Simplify the wait_for_xx bash functions #2440
Conversation
|
@chizhg: GitHub didn't allow me to request PR reviews from the following users: chizhg. Note that only knative members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
knative-prow-robot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chizhg 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 |
knative-prow-robot
added
approved
knative-prow-robot
removed
the
do-not-merge/work-in-progress
|
/cc @chaodaiG |
| fi | ||
| return 1 | ||
| echo "Waiting until all pods in namespace $1 are up" | ||
| kubectl wait pod --for=condition=Ready -n "$1" -l '!job-name' --timeout=5m || return 1 |
There was a problem hiding this comment.
Just to make sure that Ready covers Running, Completed, etc right?
There was a problem hiding this comment.
Thanks for pointing this out.
Actually the function name is not correct - other than wait_until_pods_running it also wait_until_job_pods_completed. But since the function is being used everywhere, let's also wait for the job pods to be completed to avoid regressions.
There was a problem hiding this comment.
So my $0.02, we should just use this one line: Ready and -l '!jobname'.
This doesn't properly handle Job pods today, which are what result in Completed because we need one pod to be Completed, not all (this waits for all).
|
I believe you have tested it already, so |
knative-prow-robot
added
the
do-not-merge/hold
knative-prow-robot
added
lgtm
scripts/library.sh
Outdated
| kubectl wait pod --for=condition=Ready -n "$1" -l '!job-name' --timeout=5m || return 1 | ||
| # Also wait for all the job pods to be completed. | ||
| # This is mainly for backward compatibility and should not throw out an error is there is no job in the ns. | ||
| kubectl wait job --for=condition=Complete --all -n "$1" --timeout=5m || true |
There was a problem hiding this comment.
This should be Completed (with a d), I believe.
We definitely don't want --all with Completed, maybe -ljobname (no !).
There was a problem hiding this comment.
This kubectl command I added is waiting on job instead of pod
The status of a completed job pod is like this:
conditions:
- lastProbeTime: null
lastTransitionTime: "2020-09-17T19:41:07Z"
reason: PodCompleted
status: "True"
type: Initialized
- lastProbeTime: null
lastTransitionTime: "2020-09-17T19:41:21Z"
reason: PodCompleted
status: "False"
type: Ready
- lastProbeTime: null
lastTransitionTime: "2020-09-17T19:41:21Z"
reason: PodCompleted
status: "False"
type: ContainersReady
- lastProbeTime: null
lastTransitionTime: "2020-09-17T19:41:07Z"
status: "True"
type: PodScheduled
It does not have a Completed condition, so the command will simply fail:
chizhg@chizhg-macbookpro:~$ kubectl wait pod --for=Completed -n istio-system -l 'job-name' --timeout=5m
error: unrecognized condition: "Completed"
There was a problem hiding this comment.
misread here as well, thanks for clarifying 🤦
| echo -e "\n\nERROR: timeout waiting for jobs to complete\n${jobs}" | ||
| return 1 | ||
| echo "Waiting until all batch jobs in namespace $1 run to completion." | ||
| kubectl wait job --for=condition=Complete --all -n "$1" --timeout=5m || return 1 |
There was a problem hiding this comment.
This command is waiting on job, I have tested the command on my cluster and it works:
chizhg@chizhg-macbookpro:~$ kubectl wait job --for=condition=Complete --all -n istio-system --timeout=5m
job.batch/istio-init-crd-10-1.4.6 condition met
job.batch/istio-init-crd-11-1.4.6 condition met
job.batch/istio-init-crd-14-1.4.6 condition met
job.batch/istio-security-post-install-1.4.6 condition met
|
/unhold |
knative-prow-robot
removed
the
do-not-merge/hold
What this PR does, why we need it:
kubectl wait pod --for=condition=Ready -n "$1" -l '!job-name' --timeout=5mforwait_for_pods_runningkubectl wait job --for=condition=Complete --all -n "$1"forwait_until_batch_job_completeWhich issue(s) this PR fixes:
Fixes #1611