Allow AWS and ClusterAPI providers to ignore topology.ebs.csi.aws.com/zone label #4458
Conversation
This change adds the aforementioned label to the list of ignored labels in the AWS nodegroupset processor. This change is being made in response to the addition of this label by the aws-ebs-csi-driver. This label will eventually be deprecated by the driver, but its use will prevent AWS users from properly balancing similar nodes. Also adds unit test for the AWS processor. ref: kubernetes#3230 ref: kubernetes-sigs/aws-ebs-csi-driver#729
This allows the ClusterAPI provider to ignore the `topology.ebs.csi.aws.com/zone` label by adding a custom nodegroupset processor. It also adds unit tests to exercise the new processor.
k8s-ci-robot
added
cncf-cla: yes
|
/assign @MaciekPytel ptal, we talked about this during the last sig meeting (8 nov 2021) |
|
Code looks clean and in-keeping with existing patterns within the area. I have been talking to @elmiko through the process of diagnosing this bug and I think this should solve the issue /lgtm |
k8s-ci-robot
added
the
lgtm
|
This looks good to me, thanks for adding the AWS processor test and making it so thorough! Will leave the final say to Maciek. /lgtm |
| label string | ||
| value1 string | ||
| value2 string | ||
| removeOneLabel bool |
There was a problem hiding this comment.
nit: I would consider adding negative cases as well. Right now a comparator that strips all labels (or just always return true) would pass the tests.
I don't think it's at all blocking for this PR (it's already very much net positive on test coverage), just something to maybe consider later on.
|
/lgtm This mainly impacts AWS and Cluster API, but there is a clear approval from both providers above, so merging this. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elmiko, MaciekPytel 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
This change adds 2 commits that will allow the AWS and ClusterAPI providers to ignore the
topology.ebs.csi.aws.com/zonelabel. This label is used by the AWS EBS CSI driver, and will cause the--balance-similar-node-groupsflag to fail on these platforms when comparing node groups that are in different availability zones. It adds the label to the existing AWS nodegroupset processor and creates a new processor for ClusterAPI. Also adds unit tests to exercise the processors.fixes: #3230