-
Notifications
You must be signed in to change notification settings - Fork 39.4k
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
test/integration/client: add TestExecPluginRotationViaInformer #101726
test/integration/client: add TestExecPluginRotationViaInformer #101726
Conversation
/sig auth |
@@ -18,4 +18,9 @@ set -o errexit | |||
set -o nounset | |||
set -o pipefail | |||
|
|||
if [[ -n "${EXEC_PLUGIN_OUTPUT_FILE-""}" ]]; then |
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 really wanted to collapse this shell script into a single line: cat "$EXEC_PLUGIN_OUTPUT_FILE"
. I got a little bit lazy because it would take an hour or two of refactoring the rest of the test file. I was also cognizant of trying to keep the blast radius smaller on this PR. I can certainly do the refactor, though, if you think that would be valuable.
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 think it is fine as-is.
f6706c6
to
3df7846
Compare
/assign @enj |
/triage accepted |
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.
Minor comments. Please squash into one commit.
I am a bit concerned about how long this would take to run.
} else { | ||
if !errors.Is(err, wait.ErrWaitTimeout) { |
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.
😬 so we wait for 30 seconds in this case?
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 :(. Same with waitForInformerSync
when !wantSynced
. I could shorten the timeout to like 10 seconds when !wantEvents
, but I was worried that would produce some false positives. I looked around in other integration test code and it seemed like there was some precedent of 30 second timeouts. I tried changing the timeouts to 20 seconds and the test failed locally...
test/integration/client/exec_test.go
Outdated
func waitForInformerSync(t *testing.T, informer cache.SharedIndexInformer, wantSynced bool, lastSyncResourceVersion string) { | ||
t.Helper() | ||
|
||
err := wait.PollImmediate(time.Second, time.Second*30, func() (bool, error) { | ||
return informer.HasSynced(), nil | ||
}) | ||
if wantSynced { | ||
if err != nil { | ||
t.Fatalf("wanted sync, but got error: %v", err) | ||
} | ||
} else { | ||
if !errors.Is(err, wait.ErrWaitTimeout) { | ||
if err != nil { | ||
t.Fatalf("wanted no sync, but got error: %v", err) | ||
} else { | ||
t.Fatalf("wanted no sync, but got sync") | ||
} | ||
} |
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.
Use WaitForCacheSync
?
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 was originally using WaitForCacheSync
, but switched to this way because...I can't remember? I'll switch back to WaitForCacheSync
until I remember why I did it this more complicated way...I should really comment my code more...shakes fist at my time in a previous organization where I picked up this habit. Fixed here: 3b447d4a53a.
@@ -18,4 +18,9 @@ set -o errexit | |||
set -o nounset | |||
set -o pipefail | |||
|
|||
if [[ -n "${EXEC_PLUGIN_OUTPUT_FILE-""}" ]]; then |
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 think it is fine as-is.
time.Sleep(lifetime) | ||
} | ||
|
||
func TestExecPluginRotationViaInformer(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.
t.Parallel()
? How long does this test take to run?
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 my machine, make test-integration WHAT="./test/integration/client" KUBE_TEST_ARGS="-run TestExecPluginRotat"
runs in 138.622s
(before any changes made in response to PR comments).
Is it OK for an integration test to run in parallel that makes writes to the API? I was worried that I would screw up other tests (e.g., we create a Namespace and a ConfigMap in this test). I'm all for adding a t.Parallel()
if that wouldn't mess anything else up: e9c17c84b6a.
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.
Is it OK for an integration test to run in parallel that makes writes to the API?
Each test (or maybe it is only test suite) is supposed to have it own sub section of etcd so it should be fine to run in parallel.
test/integration/client/exec_test.go
Outdated
// Wait for old token to expire. | ||
time.Sleep(lifetime) |
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 am not sure I understand what this does in the context of the test which always seems to pass in 5 seconds?
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 line pauses the test so that the exec authenticator will need to be triggered again to pick up the new token. If we didn't have this, a line like this might fail because the informer recreates its watch within the lifetime of the clientAuthorizedToken
: https://github.com/kubernetes/kubernetes/blob/3df78461ad2cdf161d09c681372b3b99412cb435/test/integration/client/exec_test.go#L642.
I suppose everywhere else we could get away without this pause since the informer would eventually pick up the new token over the course of our timeouts (e.g., https://github.com/kubernetes/kubernetes/blob/3df78461ad2cdf161d09c681372b3b99412cb435/test/integration/client/exec_test.go#L631).
I wrote this a different way - do you think this is better? This would get rid of 15s of unnecessary waiting (new test runs in 125.988s
) and potentially some confusing test code: 180e7c4aa86.
3df7846
to
e9c17c8
Compare
Signed-off-by: Andrew Keesler <akeesler@vmware.com>
e9c17c8
to
a14cd8e
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ankeesler, enj 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 |
/retest Review the full test history for this PR. Silence the bot with an |
2 similar comments
/retest Review the full test history for this PR. Silence the bot with an |
/retest Review the full test history for this PR. Silence the bot with an |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
/cc @enj @liggitt