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.

Sign up for GitHub

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

deprecating in-tree vsphere volume diskformat parameters, vsphere less than 67u3, vm hardware less than 15 and multi vCenter support #98546

Merged
merged 1 commit into from Feb 25, 2021
Merged

deprecating in-tree vsphere volume diskformat parameters, vsphere less than 67u3, vm hardware less than 15 and multi vCenter support #98546

merged 1 commit into from Feb 25, 2021

Conversation

divyenpatel
Copy link
Member

@divyenpatel divyenpatel commented Jan 28, 2021 •
edited

What type of PR is this?

/kind feature
/kind deprecation

What this PR does / why we need it:
This PR is deprecating followings for in-tree vSphere cloud provider and in-tree vSphere volume plugin

  • diskformat parameters,
  • vsphere releases less than 67u3
  • vm hardware versions less than 15 and
  • multi vCenter support

PR checks vCenter API version vSphere Cloud Provider is interacting with. If the vCenter version is less than 67u3, a deprecation warning message is printed.

klog.Warningf("vSphere version: %s is deprecated. Please consider upgrading vSphere to 6.7u3.", client.ServiceContent.About.ApiVersion)

Note: code block for vCenter version check is validated for vSphere releases mentioned in this gist - https://gist.github.com/divyenpatel/b537c949c1078e2005d42e0e2c07f38d using https://play.golang.org/p/6IR0KMPOrjk

PR checks Node VM's guest hardware version during node discovery, and it finds hardware version is less than 15, a deprecation warning message is printed.

klog.Warningf("node vm hardware version:%s is deprecated. please consider upgrading virtual machine hardware version to vmx-15 or higher", vmconfig.Version)

PR checks if vSphere Cloud Provider is configured to work with multiple vCenter servers when kubernetes nodes in the cluster are spread across multiple vCenter servers, and prints a deprecation warning message for multi vCenter support.

klog.Warningf("Multi vCenter support is deprecated. vSphere CSI Driver does not support kubernertes nodes spread across multiple vCenter servers. Please consider moving all k8s nodes to single vCenter server")

Special notes for your reviewer:
vSphere CSI driver can not support provisioning of thick/zerored thick volume using in-tree vSphere storageclass parameter diskformat after the migration is turned on.

Existing thick/zeroed thick volumes created using the in-tree vSphere volume plugin, can be migrated to CSI, but new thick/zeroed thick volumes can not be created by the vSphere CSI driver.

We have already deprecated raw vsan policy parameters hostfailurestotolerate, forceprovisioning, cachereservation, diskstripes, objectspacereservation, iopslimit as of Kubernetes 1.19 - https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.19.md#deprecation

vSphere CSI driver does not support Kubernetes nodes spread across multiple vCenter servers.

Does this PR introduce a user-facing change?:

ACTION REQUIRED: `diskformat` stroage class parameter for in-tree vSphere volume plugin is deprecated as of v1.21 release. Please consider updating storageclass and remove `diskformat` parameter. vSphere CSI Driver does not support diskformat storageclass parameter.

vSphere releases less than 67u3 are deprecated as of v1.21. Please consider upgrading vSphere to 67u3 or above. vSphere CSI Driver requires minimum vSphere 67u3.

VM Hardware version less than 15 is deprecated as of v1.21. Please consider upgrading the Node VM Hardware version to 15 or above. vSphere CSI Driver recommends Node VM's Hardware version set to at least vmx-15.

Multi vCenter support is deprecated as of v1.21. If you have a Kubernetes cluster spanning across multiple vCenter servers, please consider moving all k8s nodes to a single vCenter Server. vSphere CSI Driver does not support Kubernetes deployment spanning across multiple vCenter servers.

Support for these deprecations will be available till Kubernetes v1.24.

cc: @xing-yang @SandeepPissay @chethanv28

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. 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 Jan 28, 2021
@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. labels Jan 28, 2021
@k8s-ci-robot k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 28, 2021
@divyenpatel
Copy link
Member Author

cc: @yuga711 @gnufied @saad-ali @msau42 @misterikkit

@xing-yang
Copy link
Contributor

cc @msau42 @gnufied

@xing-yang
Copy link
Contributor

cc @yuga711

@msau42
Copy link
Member

msau42 commented Jan 28, 2021

/assign @andrewsykim
for vsphere cloud provider signoff

@divyenpatel divyenpatel mentioned this pull request Jan 28, 2021
Closed
@gnufied
Copy link
Member

gnufied commented Jan 29, 2021

Assuming not everyone reads release-notes and these changes are big enough that impact large number of users - should we consider logging a deprecating warning when a node is discovered that does not match HW version requirement?

And same thing for vsphere version too.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/cloudprovider sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 29, 2021
@divyenpatel
Copy link
Member Author

divyenpatel commented Jan 29, 2021 •
edited

Assuming not everyone reads release-notes and these changes are big enough that impact large number of users - should we consider logging a deprecating warning when a node is discovered that does not match HW version requirement?

And same thing for vsphere version too.

@gnufied @yuga711 please see the latest commit.

I am working on validating change on the vSphere releases less than 67u3 with VM hardware version less than 15.
Will update PR after I am done with testing.

@xing-yang
Copy link
Contributor

/retest

@yuga711
Copy link
Contributor

yuga711 commented Feb 16, 2021

The deprecation message here says: vsphere releases less than 67u3. The code change and the PR discusses about deprecating vCenter versions less than 6.7u3. Can you please clarify if we are deprecating only the vCenter versions or does the deprecation covers ESXi host versions < 6.7u3 as well.

@jsafrane
Copy link
Member

I tried to check how often are the deprecation messages logged (goal: don't spam log every second), but I failed to find all the code paths and when various connections could be re-created. Therefore asking here: can you please confirm that the deprecation messages are logged only few times per kube-controller-manager or kubelet?

@divyenpatel
Copy link
Member Author

The deprecation message here says: vsphere releases less than 67u3. The code change and the PR discusses about deprecating vCenter versions less than 6.7u3. Can you please clarify if we are deprecating only the vCenter versions or does the deprecation covers ESXi host versions < 6.7u3 as well.

We need to upgrade both ESX and Center to 67u3. Updated message to be more clear - Please consider upgrading vCenter and ESXi servers to 6.7u3 or higher

@divyenpatel
Copy link
Member Author

I tried to check how often are the deprecation messages logged (goal: don't spam log every second), but I failed to find all the code paths and when various connections could be re-created. Therefore asking here: can you please confirm that the deprecation messages are logged only few times per kube-controller-manager or kubelet?

Deprecation message is printed few times only 1) while creating a connection to vCenter and 2) During node discovery.

@divyenpatel
deprecating in-tree vsphere volume diskformat sc parameter, vSphere l…
3e91ac2 
…ess than 67u3, vm hardware less than 15 and multi vCenter server
andrewsykim
andrewsykim reviewed Feb 22, 2021
View reviewed changes
Copy link
Member

@andrewsykim andrewsykim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

Approving the legacy-cloud-providers changes since this is deprecating capabilities that would otherwise block users from migrating to the CSI driver and there are no new features being added here. cc @cheftako if you want to double-check on the changes here.

Would appreciate extra eyes on the vSphere code changes from another reviewer though.

Regarding future plans -- I am not sure we need to actually ever remove support for some of these features. It can stay supported until the eventual removal of the in-tree cloud providers. Specifically, users on vSphere 6.5 should be able to continue to use the in-tree drivers assuming they know CSI migration will never be supported there. Wondering if we can improve some of the wording to reflect this more accurately.

pkg/volume/vsphere_volume/vsphere_volume_util.go Show resolved Hide resolved
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, chethanv28, divyenpatel, msau42

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 Feb 22, 2021
@gnufied
Copy link
Member

gnufied commented Feb 22, 2021

lgtm from vsphere code changes side.

@msau42
Copy link
Member

msau42 commented Feb 22, 2021

I am not sure we need to actually ever remove support for some of these features. It can stay supported until the eventual removal of the in-tree cloud providers.

The purpose of this messaging is to inform users that these features will no longer work once the intree providers are removed. I think the current messaging accomplishes that?

@yuga711
Copy link
Contributor

yuga711 commented Feb 22, 2021

lgtm for the vSphere code changes.

@divyenpatel
Copy link
Member Author

/re-test

@xing-yang
Copy link
Contributor

/retest

@xing-yang
Copy link
Contributor

Discussed at sig-storage meeting today. This is good to go.

@xing-yang
Copy link
Contributor

/lgtm
approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 25, 2021
@k8s-ci-robot
Copy link
Contributor

@divyenpatel: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kind 3e91ac2 link /test pull-kubernetes-e2e-kind

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@xing-yang
Copy link
Contributor

retest

@xing-yang
Copy link
Contributor

/retest

@k8s-ci-robot k8s-ci-robot merged commit 525e397 into kubernetes:master Feb 25, 2021
@yuga711
Copy link
Contributor

yuga711 commented Mar 19, 2021

The deprecation message here says: vsphere releases less than 67u3. The code change and the PR discusses about deprecating vCenter versions less than 6.7u3. Can you please clarify if we are deprecating only the vCenter versions or does the deprecation covers ESXi host versions < 6.7u3 as well.

We need to upgrade both ESX and Center to 67u3. Updated message to be more clear - Please consider upgrading vCenter and ESXi servers to 6.7u3 or higher

Just want to confirm, for ESXi 67u3 checks, is it enough to check if the ESXi version is 6.7 and the build number is at least 14320388? (https://kb.vmware.com/s/article/2143832)

@gnufied gnufied mentioned this pull request Jul 7, 2021
Merged
@oomichi oomichi mentioned this pull request Oct 22, 2021
Closed
@gnufied gnufied mentioned this pull request Aug 29, 2022
Closed
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

@xing-yang xing-yang xing-yang left review comments

@andrewsykim andrewsykim andrewsykim left review comments

@SandeepPissay SandeepPissay SandeepPissay left review comments

@jingxu97 jingxu97 jingxu97 left review comments

@yuga711 yuga711 yuga711 left review comments

@chethanv28 chethanv28 chethanv28 approved these changes

@jsafrane jsafrane Awaiting requested review from jsafrane

@justinsb justinsb Awaiting requested review from justinsb

Assignees

@xing-yang xing-yang

@andrewsykim andrewsykim

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/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. kind/feature Categorizes issue or PR as related to a new feature. 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. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.

None yet