-
Notifications
You must be signed in to change notification settings - Fork 40.2k
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
KEP-4292: Add e2e test for custom profile in kubectl debug #127187
Conversation
/triage accepted |
30a6a9d
to
4420f7f
Compare
4420f7f
to
1749e29
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.
mostly lgtm, except for that env var removal
/hold
to make sure it's brought back
/label tide/merge-method-squash |
I think, this is ready to merge; |
test/e2e/kubectl/debug.go
Outdated
framework.ExpectNoError(err) | ||
output := e2ekubectl.RunKubectlOrDie(ns, "get", "pod", debugPodName, "-o", "jsonpath={.spec.ephemeralContainers[*]}") | ||
ginkgo.By("verifying NET_ADMIN is added") | ||
if !strings.Contains(output, "NET_ADMIN") { |
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.
Change all three conditions to use this form:
gomega.Expect(output).To(gomega.MatchError(gomega.ContainSubstring("NET_ADMIN")))
which on error will provide more explainable output, see here for reference.
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.
Used in this form;
gomega.Expect(output).To(gomega.ContainSubstring("NET_ADMIN"))
customProfileFile := "custom.yaml" | ||
framework.ExpectNoError(err) | ||
defer os.Remove(tmpDir) //nolint:errcheck | ||
framework.ExpectNoError(os.WriteFile(filepath.Join(tmpDir, customProfileFile), []byte(` |
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.
Please make sure to add an explanation to the framework.ExpectNoError(err error, explain ...interface{})
invocation, that is then helpful when debugging problems. See my other comment for reference.
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.
Added. Hope it is explanatory.
test/e2e/kubectl/debug.go
Outdated
"sleep 3600") | ||
|
||
ginkgo.By("verifying the container debugger is running") | ||
err = wait.PollUntilContextTimeout(ctx, 3*time.Second, 2*time.Minute, false, func(ctx context.Context) (bool, error) { |
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.
How about using WaitForPodContainerStarted
from k8s.io/kubernetes/test/e2e/framework/pod
?
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.
After your suggestion, I've found a better one WaitForPodsWithLabelRunning
since I'm focusing on the running
status
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
/approve
LGTM label has been added. Git tree hash: 0fb80ec427eba62e3ac8942145a07e1567a013f1
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, soltysh 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 |
…s#127187) * Remove KUBECTL_DEBUG_CUSTOM_PROFILE env var * Add e2e test for custom profile in kubectl debug * Keep feature flag until 1.33 * Update comment * Simplify tests by relying on test framework functionality * Rename import alias to better to pass verify-import-alias
…s#127187) * Remove KUBECTL_DEBUG_CUSTOM_PROFILE env var * Add e2e test for custom profile in kubectl debug * Keep feature flag until 1.33 * Update comment * Simplify tests by relying on test framework functionality * Rename import alias to better to pass verify-import-alias
…s#127187) * Remove KUBECTL_DEBUG_CUSTOM_PROFILE env var * Add e2e test for custom profile in kubectl debug * Keep feature flag until 1.33 * Update comment * Simplify tests by relying on test framework functionality * Rename import alias to better to pass verify-import-alias
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds e2e for custom profiling in kubectl debug as promised in enhancement as prerequisite for promotion to stable.
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: