Support Downward API node name override#2277
Conversation
k8s-ci-robot
added
the
release-note
k8s-ci-robot
added
size/M
hajiler
force-pushed
the
get-name-fix-branch
branch
from
8d54466 to
fa9f638
Compare
| } | ||
| if nsArgs.DataCacheEnabledNodePool { | ||
| if err := setupDataCache(ctx, *nodeName, nodeServer.MetadataService.GetName()); err != nil { | ||
| if err := setupDataCache(ctx, *nodeName, nodeServer.GetNodeName()); err != nil { |
There was a problem hiding this comment.
I'm not sure what the distinction is between nodeName and nodeId for data cache. @sunnylovestiramisu can you help review?
There was a problem hiding this comment.
nodeName is Kubernetes identifier and nodeId is GCE identifier. Most of the time they are same but back then probably just for robustness and decoupling.
There was a problem hiding this comment.
Maybe we should not change data cache to use the override until we can better understand if it is safe. If it is safe, I would rather merge to the two variables into one. Let's add a TODO to revisit this in the future
|
@suzezhang Please test in RH's env. |
|
/retest |
|
@hajiler: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
|
@sunnylovestiramisu @hajiler verified in RH environment. After updating the env var, the override label works as expected now in RH env. Thanks for the quick fix! |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hajiler, msau42 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
| } | ||
|
|
||
| nodeID := common.CreateNodeID(ns.MetadataService.GetProject(), ns.MetadataService.GetZone(), ns.MetadataService.GetName()) | ||
| nodeID := common.CreateNodeID(ns.MetadataService.GetProject(), ns.MetadataService.GetZone(), ns.GetNodeName()) |
There was a problem hiding this comment.
This chanage is causing the following error:
Warning FailedAttachVolume 58m (x6 over 80m) attachdetach-controller AttachVolume.Attach failed for volume "pvc-507bb954-97fa-40aa-98c1-c78e849cac0d" : rpc error: code = InvalidArgument desc = ControllerPublish not permitted on node "projects/ocpstrat-1278/zones/us-central1-c/instances/test-gcp10-wf7zf-worker-c-6kgqn.c.ocpstrat-1278.internal" due to backoff condition Warning FailedAttachVolume 4m57s (x39 over 80m) attachdetach-controller AttachVolume.Attach failed for volume "pvc-507bb954-97fa-40aa-98c1-c78e849cac0d" : rpc error: code = InvalidArgument desc = Failed to get instance: googleapi: Error 400: Invalid value for field 'instance': 'test-gcp10-wf7zf-worker-c-6kgqn.c.ocpstrat-1278.internal'. Must be a match of regex '[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?|[1-9][0-9]{0,19}', invalid
i think for nodeID it should keep getting the name from MetadataService
There was a problem hiding this comment.
It is already Get from MetadataService if you check the latest change, there is an additional commit merged.
There was a problem hiding this comment.
hey @sunnylovestiramisu ,
are you reffering to d44bea1?
because this is a different code path, the error presented above comes specifically from nodeID.
I've tried the following fix from my forked repo:
https://github.com/noamasu/gcp-compute-persistent-disk-csi-driver/commit/2516027168b73db3060a42bebc4ccf04aff32e68
and it worked for me. i just want to make sure we are not missing something here :)
There was a problem hiding this comment.
@sunnylovestiramisu I think Noam is talking about a different place for the bug.
The commit d44bea1 fixed the issue in main.go, but Noam is referring another wrong usage of GetNodeName() in node.go at line 762. It should be using the MetadataService.
6 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
Implement Downward API node name override in PDCSI driver to prioritize the Kubernetes-provided node name via --node-name flag over the GCE metadata service. This ensures accurate node identification when interacting with the Kubernetes API, particularly for NodeGetInfo operations that fetch node labels and topology information.
Which issue(s) this PR fixes:
Fixes #2276
Special notes for your reviewer:
Does this PR introduce a user-facing change?: