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
cloud provider: fix the fake cloud provider #95499
cloud provider: fix the fake cloud provider #95499
Conversation
Hi @nicolehanjing. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
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.
/ok-to-test
@@ -321,8 +321,15 @@ func (f *Cloud) InstanceMetadata(ctx context.Context, node *v1.Node) (*cloudprov | |||
f.addCall("instance-metadata-by-provider-id") | |||
f.addressesMux.Lock() | |||
defer f.addressesMux.Unlock() | |||
|
|||
providerID := node.Spec.ProviderID | |||
_, ok := f.ExtIDErr[types.NodeName(node.Name)] |
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.
This should probably check ExtID
instead of ExtIDErr
, and fake cloud provider could likely use a new field like MetadataErr
to mock errors when calling InstanceMetadata
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.
I thought if there is ExtIDErr
associated with the nodeName, that means ExtID
is an empty map, but if ExtIDErr
doesn't exist, we have ExtID available.
But you are right, maybe checking ExtID
is more straightforward
will update!
/triage accepted |
532810e
to
f17611a
Compare
f17611a
to
b6b5813
Compare
/retest |
providerID := node.Spec.ProviderID | ||
id, ok := f.ExtID[types.NodeName(node.Name)] | ||
if providerID == "" && ok { | ||
providerID = "fake://" + id |
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.
On second thought, I think we want Cloud
to have a field ProviderID
that holds the value that would be returned here. This way you can define the fake provider ID from the test instead of it being opaque. What do you think?
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.
yeah I agree, that could be much clearer, will update!
b6b5813
to
de60e0d
Compare
/retest |
@@ -702,6 +702,7 @@ func Test_syncNode(t *testing.T) { | |||
Effect: v1.TaintEffectNoSchedule, | |||
}, | |||
}, | |||
ProviderID: "fake://12345", |
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.
For reference, this was originally removed here https://github.com/kubernetes/kubernetes/pull/95342/files#r501940685 since the fake cloud provider did not support pre-existing provider IDs
@@ -436,14 +436,14 @@ func Test_syncNode(t *testing.T) { | |||
CreationTimestamp: metav1.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC), | |||
}, | |||
Spec: v1.NodeSpec{ | |||
ProviderID: "fake://12345", |
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.
This change here seems not related, can we undo this just to keep the git history clean?
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.
(move line 446 back here)
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.
ah sure, good catch!
de60e0d
to
be4cc50
Compare
@@ -321,13 +323,20 @@ func (f *Cloud) InstanceMetadata(ctx context.Context, node *v1.Node) (*cloudprov | |||
f.addCall("instance-metadata-by-provider-id") | |||
f.addressesMux.Lock() | |||
defer f.addressesMux.Unlock() | |||
|
|||
providerID := node.Spec.ProviderID | |||
_, ok := f.ExtID[types.NodeName(node.Name)] |
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.
Realiziing now that similar to ExtID, ProviderID
should probably be of type map[types.NodeName]string
. And here we should check f.ProviderID
instead of f.ExtdID
.
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.
gotcha! Will update
be4cc50
to
8c526fa
Compare
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.
One more minor comment, lgtm otherwise
@@ -702,6 +702,7 @@ func Test_syncNode(t *testing.T) { | |||
Effect: v1.TaintEffectNoSchedule, | |||
}, | |||
}, | |||
ProviderID: "fake://12345", |
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.
This test case name is "provider ID not implemented", so I think the existing node is not supposed to have a provider ID and the updated node is not supposed to have it either.
6efeb6b
to
bbc0364
Compare
bbc0364
to
5971fa6
Compare
5971fa6
to
acee077
Compare
@@ -321,13 +323,20 @@ func (f *Cloud) InstanceMetadata(ctx context.Context, node *v1.Node) (*cloudprov | |||
f.addCall("instance-metadata-by-provider-id") | |||
f.addressesMux.Lock() | |||
defer f.addressesMux.Unlock() | |||
|
|||
providerID := "" |
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.
@andrewsykim so I make this default empty providerID instead of inheriting from node.spec.providerID
PTAL :)
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.
/approve
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewsykim, nicolehanjing 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 |
fix the fake cloud provider and update the unit tests for cloud node controller
What type of PR is this?
/kind bug
What this PR does / why we need it:
Fix the fake cloud provider and update the unit tests for cloud node controller
After this merge, back port it into v1.19
To implement instancesV2 interface (kubernetes/cloud-provider-aws#131), we need to handle empty InstancesV2 providerID from getProviderID. A bug was found and fixed: #95342
The test case we're now missing in our unit tests for cloud node controller is when existingNode does not have
spec.providerID
already set, the updated node should set providerID based on the providerID returned fromInstanceMetadata
.Testing that would require the fake cloud provider to return a generated provider ID not derived from what's already set on the node
After this fix, the fake cloud provider will return ProviderID in InstanceMetadata from the node spec if it exists, otherwise it returns a generated providerID like fake:// where ext-id is fetched from the ExtID map
Which issue(s) this PR fixes:
Part of #125
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: