cloud node controller: refactor tests to not depend on controller/testutils #89320
Conversation
k8s-ci-robot
added
release-note-none
|
[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 |
k8s-ci-robot
added
the
approved
|
/assign @cheftako |
k8s-ci-robot
added
priority/important-longterm
k8s-ci-robot
added
the
sig/apps
|
|
||
| // 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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
Sounds good. I assume we have a tracking bug?
There was a problem hiding this comment.
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>
andrewsykim
force-pushed
the
node-controller-fake-client
branch
from
3fc9e96
to
f34b32f
Compare
k8s-ci-robot
added
the
lgtm
|
/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.: