Skip to content
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

Jump to bottom

declare unsupported vSphere versions for in-tree plugin #111255

Merged
merged 2 commits into from Jul 27, 2022
Merged

declare unsupported vSphere versions for in-tree plugin #111255

merged 2 commits into from Jul 27, 2022

Conversation

divyenpatel
Copy link
Member

@divyenpatel divyenpatel commented Jul 19, 2022 •
edited

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?

ACTION REQUIRED: 

vSphere releases less than 7.0u2 are not supported for in-tree vSphere volume as of Kubernetes v1.25. Please consider upgrading vSphere (both ESXi and vCenter)  to 7.0u2 or above.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

cc: @xing-yang

@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/documentation Categorizes issue or PR as related to documentation. kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 19, 2022
@k8s-ci-robot
Copy link
Contributor

@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 triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/cloudprovider labels Jul 19, 2022
@k8s-ci-robot k8s-ci-robot added the sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. label Jul 19, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Jul 19, 2022
@xing-yang
Copy link
Contributor

/assign @gnufied @msau42

@xing-yang
Copy link
Contributor

/assign

@xing-yang
Copy link
Contributor

/retest

@xing-yang
Copy link
Contributor

In the release note, can you clarify that this is for the in-tree vSphere volume plugin?

@divyenpatel
Copy link
Member Author

In the release note, can you clarify that this is for the in-tree vSphere volume plugin?

vSphere releases less than 7.0u2 are not supported for in-tree vSphere volume as of Kubernetes v1.25. Please consider upgrading vSphere (both ESXi and vCenter) to 7.0u2 or above.

Is this looking good?

gnufied
gnufied reviewed Jul 20, 2022
View reviewed changes
staging/src/k8s.io/legacy-cloud-providers/vsphere/vclib/connection.go Outdated
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)
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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"?

Copy link
Member Author

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)

@gnufied
Copy link
Member

gnufied commented Jul 20, 2022

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 20, 2022
@xing-yang xing-yang mentioned this pull request Jul 24, 2022
Merged
@xing-yang
Copy link
Contributor

/assign @Jiawei0227 @jingxu97 @jsafrane

@divyenpatel
Copy link
Member Author

@gnufied if the updated message looks ok. can you remove the hold from this PR?

@gnufied
Copy link
Member

gnufied commented Jul 26, 2022

/hold cancel
lgtm

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 26, 2022
@jingxu97
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 26, 2022
@xing-yang
Copy link
Contributor

/lgtm
/approve

@divyenpatel
Copy link
Member Author

/assign @dims

@dims
Copy link
Member

dims commented Jul 26, 2022

/approve
/lgtm

@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 26, 2022
@divyenpatel
Copy link
Member Author

/test pull-kubernetes-verify-govet-levee

@k8s-triage-robot
Copy link

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:

  • The PR does have any do-not-merge/* labels
  • The PR does not have the needs-ok-to-test label
  • The PR is mergeable (does not have a needs-rebase label)
  • The PR is approved (has cncf-cla: yes, lgtm, approved labels)
  • The PR is failing tests required for merge

You can:

/retest

4 similar comments
@k8s-triage-robot
Copy link

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:

  • The PR does have any do-not-merge/* labels
  • The PR does not have the needs-ok-to-test label
  • The PR is mergeable (does not have a needs-rebase label)
  • The PR is approved (has cncf-cla: yes, lgtm, approved labels)
  • The PR is failing tests required for merge

You can:

/retest

@k8s-triage-robot
Copy link

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:

  • The PR does have any do-not-merge/* labels
  • The PR does not have the needs-ok-to-test label
  • The PR is mergeable (does not have a needs-rebase label)
  • The PR is approved (has cncf-cla: yes, lgtm, approved labels)
  • The PR is failing tests required for merge

You can:

/retest

@k8s-triage-robot
Copy link

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:

  • The PR does have any do-not-merge/* labels
  • The PR does not have the needs-ok-to-test label
  • The PR is mergeable (does not have a needs-rebase label)
  • The PR is approved (has cncf-cla: yes, lgtm, approved labels)
  • The PR is failing tests required for merge

You can:

/retest

@k8s-triage-robot
Copy link

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:

  • The PR does have any do-not-merge/* labels
  • The PR does not have the needs-ok-to-test label
  • The PR is mergeable (does not have a needs-rebase label)
  • The PR is approved (has cncf-cla: yes, lgtm, approved labels)
  • The PR is failing tests required for merge

You can:

/retest

@xing-yang
Copy link
Contributor

/milestone v1.25

@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone Jul 27, 2022
@k8s-triage-robot
Copy link

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:

  • The PR does have any do-not-merge/* labels
  • The PR does not have the needs-ok-to-test label
  • The PR is mergeable (does not have a needs-rebase label)
  • The PR is approved (has cncf-cla: yes, lgtm, approved labels)
  • The PR is failing tests required for merge

You can:

/retest

@k8s-ci-robot k8s-ci-robot merged commit 1fe71e7 into kubernetes:master Jul 27, 2022
@xing-yang xing-yang mentioned this pull request Jul 29, 2022
Merged
@xing-yang
Copy link
Contributor

@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.

@sftim
Copy link
Contributor

sftim commented Aug 1, 2022

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 ?

@xing-yang
Copy link
Contributor

@sftim What would you suggest to be copied into the detailed release notes?

@sftim
Copy link
Contributor

sftim commented Aug 2, 2022

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:

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 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.

@xing-yang
Copy link
Contributor

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:

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 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnufied gnufied gnufied left review comments

@jingxu97 jingxu97 jingxu97 left review comments

@cheftako cheftako Awaiting requested review from cheftako

@liggitt liggitt Awaiting requested review from liggitt

Assignees

@gnufied gnufied

@dims dims

@xing-yang xing-yang

@jsafrane jsafrane

@Jiawei0227 Jiawei0227

@jingxu97 jingxu97

@msau42 msau42

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cloudprovider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.25
Development

Successfully merging this pull request may close these issues.

None yet