-
Notifications
You must be signed in to change notification settings - Fork 38.8k
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 node controller: refactor tests to not depend on controller/testutils #89320
cloud node controller: refactor tests to not depend on controller/testutils #89320
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewsykim 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 |
/assign @cheftako |
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.
Sorry the diff is so hard to read :(
|
||
// This test checks that a node with the external cloud provider taint is cloudprovider initialized and | ||
// the GCE route condition is added if cloudprovider is GCE | ||
func TestGCECondition(t *testing.T) { |
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 was kept separate since it timestamped fields can't go in the test table.
ObjectMeta: metav1.ObjectMeta{ | ||
Name: "node0", | ||
CreationTimestamp: metav1.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC), | ||
Labels: map[string]string{}, | ||
Labels: map[string]string{ | ||
"failure-domain.beta.kubernetes.io/region": "us-west", |
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.
Not against a backward compatibility test but these tags are now deprecated, correct? Shouldn't we also have a version of this test using the GA labels?
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.
Actually is topology.kubernetes.io/region the GA version? Some of our release notes reference "failure-domain.kubernetes.io/region", which matched my memory but the code seems to indicate "topology.kubernetes.io/region".
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.
It would still be good to have a test case which does not use "failure-domain.beta.kubernetes.io".
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.
We currently add both labels for compatibility and won't be removing the beta ones until v1.21. This test checks for both.
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.
Correct me if I'm wrong, but I thought it was the cloud provider who would be adding these labels. As such it would be good to make sure that we do the right thing when we both styles of label are provided and also when only the new labels are provided.
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.
Correct me if I'm wrong, but I thought it was the cloud provider who would be adding these labels.
The cloud provider provides the zone/region values, but the upstream controller (cloud node controller) will always set both beta/GA labels. There's no way at the moment to only add the GA ones.
Effect: v1.TaintEffectNoSchedule, | ||
}, | ||
}, | ||
ProviderID: "aws://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.
At some point it would be nice to get a properly functioning sample cloud provider, with its own ID and use that. No chance of bleed over with/from an existing provider.
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 learned during this refactor that there's a few issues with FakeCloud
and as a result we are not testing certain code paths (like most of the *ByProviderID methods), I intend to clean this up in a future PR.
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.
Sounds good. I assume we have a tracking bug?
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.
Not yet. I don't understand the full scope of the issues yet, but I'll create one when I do
…tutils Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
3fc9e96
to
f34b32f
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.
/lgtm
Not sure TestGCECondition belongs here but we can deal with that a little later.
/retest |
/test pull-kubernetes-e2e-gce |
Signed-off-by: Andrew Sy Kim kim.andrewsy@gmail.com
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Removes dependency to pkg/controller/testutils in cloud node controller.
Which issue(s) this PR fixes:
Part of #81172
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: