-
Notifications
You must be signed in to change notification settings - Fork 38.9k
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
use kubectl-with-retry on pause & resume #26243
Conversation
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry. Otherwise, if this message is too spammy, please complain to ixdy. |
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry. Otherwise, if this message is too spammy, please complain to ixdy. |
1 similar comment
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry. Otherwise, if this message is too spammy, please complain to ixdy. |
ok to test |
The
|
@@ -1087,13 +1087,11 @@ __EOF__ | |||
kube::test::get_object_assert deployment "{{range.items}}{{$deployment_image_field}}:{{end}}" "${IMAGE_NGINX}:${IMAGE_NGINX}:" | |||
kube::test::if_has_string "${output_message}" "Object 'Kind' is missing" | |||
## Pause the deployment | |||
output_message=$(! kubectl rollout pause -f hack/testdata/recursive/deployment --recursive 2>&1 "${kube_flags[@]}") |
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.
Would you change the title comment of this test too (L1077)? These tests are about "--recursive" and the "rollout" commands, but not about "Rollback a deployment". It's somewhat misleading. Changing each sub-title would help too (maybe add "recursively" to the end of each sub-title, and mention that we expect error message "Object 'Kind' is missing").
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.
Sure thing
kube::test::get_object_assert deployment "{{range.items}}{{.spec.paused}}:{{end}}" "true:true:" | ||
kube::test::if_has_string "${output_message}" "Object 'Kind' is missing" |
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 believe we still want this check?
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 do, but I removed it as it was the only idea I had for a solution to attempt to triage the flake quickly by using kubectl-with-retry
instead of kubectl
.
However, in doing so, kubectl-with-retry
directs stderr to a file that is removed once it is done executing, and does not allow me to review it for this missing object as I was doing
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 can have a retry function with customized error message check, or we can pass the expected error message to that retry function. WDYT?
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.
Do we still need 2>&1 when we're using kubectl-with-retry?
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.
Maybe creating a new retry function with your own check is easier, and we can add a comment saying something like "remove this function once #20437 is fixed"
The e2e test is hitting flake #26210 |
@k8s-bot e2e test this issue: #IGNORE |
@janetkuo I've gone ahead and addressed your feedback:
PTAL |
Thanks, looks good now. Please squash and I'll apply the tag. |
@janetkuo all set, thanks! |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
GCE e2e build/test passed for commit 54e6d23. |
Automatic merge from submit-queue |
attempts to fix #25645 by using
kubectl-with-retry
onrollout {pause,resume}
(resume
is for safe measures) instead ofkubectl
directly, as is done with otherrollout {pause,resume}
tests in this same script.