-
Notifications
You must be signed in to change notification settings - Fork 167
Integrate disk support labels into NodeGetInfo #2199
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
base: master
Are you sure you want to change the base?
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hajiler The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test all |
|
/assign @tonyzhc |
a2ec1ec to
6fbd415
Compare
6fbd415 to
02c19b1
Compare
02c19b1 to
60a5117
Compare
60a5117 to
e1d3d24
Compare
| Segments: map[string]string{constants.TopologyKeyZone: ns.MetadataService.GetZone()}, | ||
| } | ||
|
|
||
| node, err := k8sclient.GetNodeWithRetry(ctx, ns.MetadataService.GetName()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: You can rename err to nodeErr and avoid having to set it to nil in the if statement.
| fmt.Sprintf(constants.NodeRestrictionLabelPrefix, constants.AttachLimitOverrideLabel): "9999", | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a test case where node is nil as well, just to add coverage for the node == nil case
What type of PR is this?
What this PR does / why we need it:
This PR is a pre-requisite for the generic volumes feature. It adds disk types supported by a node to the topology information using NodeGetInfo. The PR will be followed up by using this topology information to select a compatible disk type when provisioning a volume.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Node attach limit override and the disk type labels rely on reading the Node k8s object. Instead of repeating this request, I migrated it out to NodeGetInfo and piped down the Node object. This also allowed me to add more comprehensive unit testing.
Does this PR introduce a user-facing change?: