-
Notifications
You must be signed in to change notification settings - Fork 159
Update docs/stable overlay to reference new release #658
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update docs/stable overlay to reference new release #658
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: saad-ali The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
a88c20d
to
cbfc795
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL
cbfc795
to
02a86e8
Compare
02a86e8
to
07cd01d
Compare
@msau42 When are the new sidecar images going to be promoted? |
07cd01d
to
44700fc
Compare
PTAL |
@@ -12,8 +12,8 @@ kind: ImageTagTransformer | |||
metadata: | |||
name: imagetag-csi-provisioner-prow-head | |||
imageTag: | |||
name: gke.gcr.io/csi-provisioner | |||
newName: quay.io/k8scsi/csi-provisioner | |||
name: k8s.gcr.io/sig-storage/csi-provisioner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any alpha/dev image transformations that need to be updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Alpha only overrides the driver image, not the sidecars. And dev just uses alpha image transformers.
44700fc
to
776e647
Compare
newName: gcr.io/gke-release-staging/csi-provisioner | ||
newTag: "v2.0.3-gke.0" | ||
name: k8s.gcr.io/sig-storage/csi-provisioner | ||
newName: gcr.io/k8s-staging-sig-storage/csi-provisioner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
staging-rc could remain on k8s.gcr.io/sig-storage since it's still using released images
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not want to push it to use the latest available released staging image?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For pd driver image, I think using the staging repo makes sense.
But I'm not sure about CSI sidecars. We don't use internal pdcsi tests as an indicator for OSS CSI sidecar releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. We don't need to test unreleased versions of sidecars in this repo.
before updating stable overlay, is there a testing phase? Or we just use stable overlay for testing. |
/test pull-gcp-compute-persistent-disk-csi-driver-e2e-win2019 |
The pull job tests stable overlay with custom built pd driver image, so doesn't exactly reflect the stable overlay. The closest thing we have to pretesting stable overlay is the staging-rc overlay, so it's good to make sure they match as close as possible. |
Update docs and stable overlay to reference new release (1.1.0). Move leader election and volume in use patches to base overlay. Switch sidecar images to k8s.gcr.io/sig-storage/ and gcr.io/k8s-staging-sig-storage
776e647
to
7a715d0
Compare
PTAL |
# - "--run-controller-service=false" # disable the controller service of the CSI driver | ||
# - "--run-node-service=false" # disable the node service of the CSI driver | ||
- "--leader-election" | ||
- "--default-fstype=ext4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to set default fstype?
If the reason we need to set it due to issue related to fsgroup will be ignored if default fstype is not set, kubernetes-sigs/vsphere-csi-driver#370 (comment)
This logic should be fixed in 1.19 by kubernetes/kubernetes#92001. This means for testing k/k 1.19 and above, no need to set default-fstype here. During format and mount, the default fstype will be applied depending on OS.
So for OSS test which using the latest kubernetes, we don't need to set default value here so we can test the behavior of correctly setting fstype by driver.
For internal tests which tests earlier version of kubernetes, it also seems ok so far without setting default-fstype. From what I searched, we didn't test fsgroup behavior yet.
There is a new test added very recently for 1.20. https://github.com/kubernetes/kubernetes/blame/master/test/e2e/storage/testsuites/fsgroupchangepolicy.go
So I think from testing point, should be ok without setting default value here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we don't need it staring with Kubernetes 1.20. In 1.19 it's still an alpha feature and not enabled by default.
I think for the purposes of this PR, which is to cleanup the overlays for the already released driver, we should still keep it.
For the next driver release, we can look into removing it along with adding the CSIDriver change. It will also bump up the min version of this driver to Kubernetes 1.20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #668 to track this since this PR has merged.
/lgtm |
Cherrypick #658 from `master` to `release-1.1`
What type of PR is this?
What this PR does / why we need it:
k8s.gcr.io/sig-storage/
andgcr.io/k8s-staging-sig-storage
.Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
/assign @msau42