Add zone-id topology label to nodes #855
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
needs-triage
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
|
Generally lgtm, but may want to split the deps upgrade as a separate PR. At least clean up/fixup the "merge" commit. |
@olemarkus That's fair. I put up another PR to separate it out. I can remove the changes here once that's merged. |
tests/e2e/nodes.go
Outdated
| /* | ||
| Copyright 2024 The Kubernetes Authors. | ||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ |
There was a problem hiding this comment.
Formatting got weird here
There was a problem hiding this comment.
Ya, I just copied the other test file. I'll make it nicer when I rebase.
| @@ -63,6 +64,25 @@ func (c *Cloud) InstanceShutdown(ctx context.Context, node *v1.Node) (bool, erro | |||
| return c.InstanceShutdownByProviderID(ctx, providerID) | |||
| } | |||
|
|
|||
| func (c *Cloud) getAdditionalLabels(zoneName string) (map[string]string, error) { | |||
There was a problem hiding this comment.
This function could just add the zone ID label to an existing map, probably cleaner in the long run as more labels are set:
| func (c *Cloud) getAdditionalLabels(zoneName string) (map[string]string, error) { | |
| func (c *Cloud) addLabelZoneID(labels map[string]string, zoneName string) (error) { |
There was a problem hiding this comment.
Ya, I had considered that, but I preferred to keep encapsulate creating the additional labels in it's own method to keep the InstanceMetadata method as dumb as possible.
That being said, I could still pull out zone ID logic into it's own method and create one as you suggest, but I left the restructuring to when we have more than a single label.
That being said, I don't feel that strongly here if you do. One downside to having a single method for labels that calls out to other methods is that the list of inputs could potentially grow awkwardly over time.
There was a problem hiding this comment.
Why not
| func (c *Cloud) getAdditionalLabels(zoneName string) (map[string]string, error) { | |
| func (c *Cloud) getZoneIDByZoneName(zoneName string) (string, error) { |
This would make it more in line with the other functions. And since only one label is being returned anyway.
There was a problem hiding this comment.
Tx for the discussion. I did kind of a mix of all of this. I moved the zone logic out to the zones file so that this is just getting a value and setting the label.
Includes node e2e test to verify that the label is actually applied
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cartermckinnon 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
|
Remember to change the PR description. |
|
/retest |
Co-authored-by: Carter <cartermckinnon@gmail.com>
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
What type of PR is this?
/kind feature
What this PR does / why we need it:
As part of 1.30, we added the ability to add custom labels from the cloud provider to nodes, and AWS customers are looking for a way to label nodes with a zone ID, which will be consistent across account. That label is added here.\
Which issue(s) this PR fixes:
Fixes #300
Special notes for your reviewer:
Added a new e2e case for this change.
Does this PR introduce a user-facing change?: