[occm] Allow changing cluster-name on existing deployments #2552
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
the
size/L
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
This is a WiP, definitely needs unit tests and some more manual testing from me. |
dulek
force-pushed
the
rename-lbs
branch
2 times, most recently
from
7f29797 to
bf40e6b
Compare
I remember it's been discussed multiple times before (with --cluster-name=kubernetes) |
|
@jichenjc: I dug out #2241 and added it to this PR. Citing @zetaab from there:
This is what the PR proposes - to make it easier to migrate away from a wrong |
|
PR needs rebase. 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
added
the
needs-rebase
It's a common issue that clusters are deployed with the default `--cluster-name=kubernetes` and later on it's discovered that next deployments on the same cloud will have conflicts when trying to manage LBs of the same namespace and name. This commit aims at allowing to change the cluster-name on a running environment and handling all the renames of the LB resources and their tags.
k8s-ci-robot
removed
the
needs-rebase
By an terrible oversight, since the in-tree, OpenStack cloud provider was run without passing `--cluster-name` argument, meaning that it just used the default of "kubernetes" as the cluster name when constructing the names of the LB resources. Obviously this led to conflicts when there were multiple OCP clusters in a single OpenStack tenant and it's still a challenge for example for QE running tests on a single cloud. This issue couldn't be fixed easily, because we haven't had an upgrade story solved. Now kubernetes/cloud-provider-openstack#2552 is an attempt to make CCM rename the LBs when the cluster-name is reconfigured. This should allow us to finally solve the name conflict issues. This commit makes sure that CCMO deploys OpenStack CCM passing `--cluster-name`.
|
Hey, @stephenfin, could you take a preliminary look? |
|
And you @gryf. |
By an terrible oversight, since the in-tree, OpenStack cloud provider was run without passing `--cluster-name` argument, meaning that it just used the default of "kubernetes" as the cluster name when constructing the names of the LB resources. Obviously this led to conflicts when there were multiple OCP clusters in a single OpenStack tenant and it's still a challenge for example for QE running tests on a single cloud. This issue couldn't be fixed easily, because we haven't had an upgrade story solved. Now kubernetes/cloud-provider-openstack#2552 is an attempt to make CCM rename the LBs when the cluster-name is reconfigured. This should allow us to finally solve the name conflict issues. This commit makes sure that CCMO deploys OpenStack CCM passing `--cluster-name`.
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
Alright, I tested this through and through, it's safe. |
|
Nice work. The logic looks good as does the structure. No complaints from my end. /approve |
|
Looks good from my side as well. Good job, @dulek! |
| // lbHasOldClusterName checks if the OCCM LB prefix is present and if so, validates the cluster-name | ||
| // component value. Returns true if the cluster-name component of the loadbalancer's name doesn't match | ||
| // clusterName. | ||
| func lbHasOldClusterName(loadbalancer *loadbalancers.LoadBalancer, clusterName string) bool { |
There was a problem hiding this comment.
so basically this will assume that all "kubernetes" loadbalancers are from old clustername? This is quite risky if someone have now two kubernetes clusters named "kubernetes" in same openstack project. However, that kind of setup is already problematic but I think someone is doing that
Like:
cluster1 named "kubernetes"
cluster2 named "kubernetes"
now cluster1 will rename all loadbalancers to "cluster1". It will also rename cluster2 loadbalancers incorrectly (and will break things)
There was a problem hiding this comment.
I think this approach will still be safe when there are multiple clusters using default cluster name, assuming that there were no conflicting LBs, which would be not functional anyway. Note that renameLoadBalancer() is only called from ensureLoadBalancer() [1] meaning that it's not looking up all LBs the tenant sees in OpenStack, but rather reacts on Services that were supposed to have LBs, so renames should be constrained to a single K8s cluster.
|
/lgtm ...is what I meant 😅 |
k8s-ci-robot
added
the
lgtm
|
/approve I think @zetaab you approved this :) |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jichenjc, stephenfin 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 |
k8s-ci-robot
added
the
approved
|
/cherry-pick release-1.29 We'd like this to be backported to 1.29 as a bugfix. I'd like to hear opinions if that's feasible. |
|
@dulek: new pull request created: #2568 In response to this:
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. |
What this PR does / why we need it:
It's a common issue that clusters are deployed with the default
--cluster-name=kubernetesand later on it's discovered that next deployments on the same cloud will have conflicts when trying to manage LBs of the same namespace and name.This commit aims at allowing to change the cluster-name on a running environment and handling all the renames of the LB resources and their tags.
Which issue this PR fixes(if applicable):
fixes #2241
Special notes for reviewers:
Release note: