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

Don't fail when OpenStack config contains unknown directives #109709

Merged

Conversation

mdbooth
Copy link
Contributor

@mdbooth mdbooth commented Apr 28, 2022

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR changes the parsing of the OpenStack cloud config to be non-strict. This is in line with the behaviour of, for example:

although some other providers remain strict.

The driver for this is the cross-over period from legacy to external CCM. For reasons I admit I do not fully understand, KCM must continue for some time to use the legacy cloud provider for the in-tree volume driver even though new volumes are created exclusively using CSI.

Prior to upgrade the user's cloud config is obviously compatible with the legacy cloud provider. After upgrade we would like to allow them to add directives which enable new functionality from OpenStack CCM. This is not possible while we are continuing to also pass this config to KCM, at least not without a level of pre-parsing which presents its own maintenance issues.

This change makes KCM ignore directives it does not understand. Rather than ignoring this entirely, as seems to be normal, we emit a friendly warning which should hopefully allow the user to confidently ignore it if it does not apply.

@jsafrane I wonder if you could expand on the reasons KCM still needs in-tree volume support?

Special notes for your reviewer:

Does this PR introduce a user-facing change?

When using the OpenStack legacy cloud provider, kubelet and KCM will ignore unknown configuration directives rather than failing to start.

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


@k8s-ci-robot
Copy link
Contributor

Please note that we're already in Test Freeze for the release-1.24 branch. This means every merged PR has to be cherry-picked into the release branch to be part of the upcoming v1.24.0 release.

@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. 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 Apr 28, 2022
@k8s-ci-robot
Copy link
Contributor

@mdbooth: 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 Apr 28, 2022
@k8s-ci-robot k8s-ci-robot added sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 28, 2022
@jsafrane
Copy link
Member

jsafrane commented May 3, 2022

I wonder if you could expand on the reasons KCM still needs in-tree volume support?

CSI migration is GA in Kubernetes 1.24, all 1.24 nodes should not need in-tree OpenStack cloud provider. However, according to Kubernetes version skew policy, v1.24 KCM still needs to support nodes with v1.22 kubelet and thus it still needs to have in-tree cinder volume plugin + OpenStack cloud provider to be able to attach/detach Cinder volumes to them. IMO the cloud provider should not be used for anything else that volume attach/detach to these old nodes, the rest will be done by the external CCM.

We can remove the in-tree cloud provider in Kubernetes v1.26 - KCM there can see only 1.26, 1.25 and 1.24 kubelets and all of them don't have in-tree cinder volume plugin.

@jsafrane
Copy link
Member

jsafrane commented May 3, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 3, 2022
@jsafrane
Copy link
Member

jsafrane commented May 3, 2022

/assign @andrewsykim @nckturner
for approval.

This does not add a new feature to the in-tree OpenStack cloud provider, it allows adding new features to CCM while sharing the same config between KCM and CCM.

@dims
Copy link
Member

dims commented Jun 3, 2022

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, mdbooth

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 Jun 3, 2022
@k8s-ci-robot k8s-ci-robot merged commit b578d61 into kubernetes:master Jun 3, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/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 Denotes a PR that will be considered when it comes time to generate release notes. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants