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
declare unsupported vSphere versions for in-tree plugin #111255
declare unsupported vSphere versions for in-tree plugin #111255
Conversation
@divyenpatel: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/assign |
/retest |
In the release note, can you clarify that this is for the in-tree vSphere volume plugin? |
Is this looking good? |
klog.Errorf("failed to check if vCenter version:%v and api version: %s is supported. Error: %v", client.ServiceContent.About.Version, client.ServiceContent.About.ApiVersion, err) | ||
} | ||
if vcNotSupported { | ||
klog.Warningf("vCenter version is not supported. version: %s, api verson: %s Please consider upgrading vCenter and ESXi servers to 7.0u2 or higher", client.ServiceContent.About.Version, client.ServiceContent.About.ApiVersion) |
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.
Thanks for making this change. I wonder about changing the messages in the logs though.
One can still disable a beta feature gate and keep old behavior at least for one release. Users who are upgrading to 1.25 without manually disabling feature gate will obviously run into problems.
But while the feature is in beta - technically it is still possible to disable the migration (even though it is enabled by default) and hence I wonder if Warning message here should additionally check if VSphere migration feature gate is enabled or not.
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.
Regardless of whether migration is enabled or disabled, we want to send out a message that vSphere releases less than 7.0u2 are no longer supported for the in-tree vSphere plugin from k8s 1.25 release.
Here we are just printing a warning message and does not error out which means the user can continue to use the in-tree driver on vSphere releases less than 7.0u2.
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 think sending warning no matter migration is enabled or disabled seems fine. It would be good the let customers know about this. Once it is GA, they cannot disable it anymore.
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.
maybe mention some more details?
"vCenter version is not supported with CSI migration which is enabled by default and will be GA in future release"?
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.
@jingxu97 updated message as below
klog.Warningf("vCenter version (version: %q, api verson: %q) is not supported for CSI Migration. Please consider upgrading vCenter and ESXi servers to 7.0u2 or higher for migrating vSphere volumes to CSI.", client.ServiceContent.About.Version, client.ServiceContent.About.ApiVersion)
/hold |
/assign @Jiawei0227 @jingxu97 @jsafrane |
@gnufied if the updated message looks ok. can you remove the |
/hold cancel |
/lgtm |
/lgtm |
/assign @dims |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, divyenpatel, xing-yang 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 |
/test pull-kubernetes-verify-govet-levee |
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
4 similar comments
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
/milestone v1.25 |
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
@divyenpatel, can you please update the release note of this PR to the following? From Kubernetes v1.25, the in-tree vSphere volume driver will not support any vSphere release before 7.0u2. Check the v1.25 detailed release notes for more advice on how to handle this. |
This doesn't seem right: the text you're proposing @xing-yang will be copied into those detailed release notes. How about using the Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc section, and adding a hyperlink to https://kubernetes.io/docs/concepts/storage/volumes/#vspherevolume ? |
@sftim What would you suggest to be copied into the detailed release notes? |
When you set a release note on a PR, all that release note text is always copied into the detailed release notes. That was what I meant. So, someone reading that detailed release note document might encounter advice:
This is frustrating: it's like finding that the link you're trying to click on takes you to the web page you're already reading. So, we should fix that. |
I see what you meant now. Thanks for the explanation. |
What type of PR is this?
/kind cleanup
/kind documentation
/kind deprecation
What this PR does / why we need it:
Declare unsupported vSphere versions for the in-tree plugin.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
cc: @xing-yang