Deployement and readiness
Closed this issue · 14 comments
Hello @baracoder
Great script! It would be nice if the script could also wait for the pods to be ready (based on the readinessProbe). The Desired and Current number of pods are only valid if you want to ensure the pod is launched. In real life, what you need to know is if the pod has successfully initialized.
Thanks,
Samuel
Hi @timoreimann I beleive the issue is still valid.
When we patch a deployment, the new replica set is created, but the previous pods are still Active/Available until the new pod readinessProbe succeed
kubectl patch deployment apps-test-deployment -p '{"spec":{"template":{"spec":{"containers":[{"name":"'"apps-test-deployment"'","image":"'"$NEW_IMAGE_HASH"'"}]}}}}'
$ ./bin/scripts/wait-for-deployment.sh apps-test-deployment
Waiting for specified generation: 120 for deployment apps-test-deployment to be observed
Observed expected generation: 120
Expected replicas: 1
Observed expected number of availabe replicas: 1
Deployment of service apps-test-deployment successful. All 3 ready
Status :
So, if you check only for the right number of Ready pods, you also include in the count the pods that have the previous image.
Maybe the solution in my case would be to add a parameter to wait for a specific image ?
Hi @timoreimann I submited a PR that contains the intended behaviour. Feel free to review it, and fix issues and improve it.
@sadortun thanks for your PR! I'll try to review it ASAP.
I'm a bit baffled we need this though. Would have expected the existing values we get from the API to suffice. Seems like I'm wrong, however. :-)
Just to double check: did you use the latest version of the script to test the behavior? The log output style seems to indicate you're behind one or two commits maybe.
Yes, i was on master@3b37c8df1ffa31b52b442c77922b316cba444545
That's strange because I can't see the log line
Waiting for specified generation:
you posted in the current master anymore. You didn't add it in a local copy of yours, did you?
FWIW, kubectl rollout status does something very similar these days to this script. I think it was added after I created the script.
The script does not rely on kubectl, however.
Looking at how kubectl implements the rollout status command, there's no indication of the image being factored in somehow. What I think the code does better and the script is missing is a verification that no surplus replicas are running: Those are from the previous deployment that we want to wait for getting terminated prior to signaling completion of the deployment.
If what I suspect is true, we could change the script logic (and hence the PR) to more closely behave like kubectl. The other alternative is to rely on kubectl directly and call rollout status on it. This would also bring in other safety checks that only kubectl is doing right now, such as validating the spec revision. The script's job would then be limited to providing a timeout limit, something which I believe kubectl isn't doing.
As already mentioned, however, this would create a dependency on kubectl. Not sure if that's something people want to prevent at all costs. It doesn't seem worth it in my perspective given kubectl's general usefulness.
Have you tested with Replica Sets or Replications Controllers ?
Looking at the Replica set status the Rollout status seems to indicate if the deployment has updated the Replica Sets, but not if the Pods inside the Replica Sets are ready.
The kubectl rollout status does not produce the behaviour i expect.
$ kubectl set image deployment/apps-test-deployment apps-test-deployment=myImage:version_hash
deployment "apps-test-deployment" image updated
$ kubectl rollout status deployment/apps-test-deployment && kubectl get deployment && kubectl get rs && kubectl get pods --selector=app=apps-test-deployment -o jsonpath='{range .items[*]}{range @.status.conditions[*]}{@.type}={@.status};{end}{end}'
# kubectl rollout status deployment/apps-test-deployment
deployment apps-test-deployment successfully rolled out
# kubectl get deployment
NAME DESIRED CURRENT UP-TO-DATE AVAILABLE AGE
apps-test-deployment 1 2 1 1 27d
# kubectl get rs
NAME DESIRED CURRENT READY AGE
apps-test-deployment-1675818725 1 1 1 11m
apps-test-deployment-4218812270 1 1 0 6s
# kubectl get pods --selector=app=apps-test-deployment -o jsonpath='{range .items[*]}{range @.status.conditions[*]}{@.type}={@.status};{end}{end}'
Initialized=True;Ready=True;PodScheduled=True;
Initialized=True;Ready=False;PodScheduled=True;As you can see, rollout status indicate evertthing is done.
- The previous pod apps-test-deployment-1675818725 is still alive
- The new pod apps-test-deployment-4218812270 is Not ready
I had not tested it but did so now. :-)
I can confirm the behavior you're experiencing: kubectl rollout status does not take the ready state into account. The problem is that all the fine logic I have been describing hasn't made it to the latest stable release yet: The commit in question will be part of the yet-unreleased Kubernetes 1.5.0. Until then, the current 1.4.6 implementation only cares about the observed generation and the number of updated replicas equaling the number of specified replicas. Apparently, a replica counts as being updated as soon as it has been initialized.
So we can't rely on kubectl, at least not for now. It still looks like we can achieve what we want by double-checking on the number of current/available/updated replicas, just like kubectl does. I created PR #12 for that purpose. Would you mind taking a look and letting me know how you think it compares to your approach?
Thanks!
Tested your PR, looking much better now ! I feel much better knowing i was not the only one having this issue ! I was not able to find other bug reports about this strange behaviour !
$ ./bin/scripts/wait-for-deployment.sh apps-test-deployment 666
Waiting for deployment with timeout 666 seconds
Expected generation for deployment apps-test-deployment: 153
Observed expected generation: 153
Specified replicas: 1
current/updated/available replicas: 2/1/1, waiting
current/updated/available replicas: 2/1/1, waiting
current/updated/available replicas: 2/1/1, waiting
.......
current/updated/available replicas: 2/1/1, waiting
Deployment apps-test-deployment successful. All 1 replicas are ready.I think that the wrong problem might be solved here. (and in kubectl rolling update)
- #12 gives feedback on : Are all pending rolling updates finished yet ?
- #11 gives feedback on : Does the current rolling update finished yet ?
Also, the logic that came from kubctl, seems a little odd. Not checking what is actually getting deployed seems strange. For example if a few user push to CI/CD, the first user deploy script might wait for the others users builds to deploy before getting the "All Good" status....
@sadortun the latest kubectl rollout status version also allows to specify a custom revision. That way, you may pinpoint the command to a particular deployment.
I'll try to incorporate the revision in the PR as well.
@sadortun unfortunately, I'm a bit short on time right now, so I couldn't integrate the revision part yet. Instead of stretching out #12 much longer, I have decided to merge it; incorporating the revision is something we can always do as a follow-up. Feel free to submit another PR if you like. :-)
(Note to implementer: The revision annotation has the following structure: deployment.kubernetes.io/revision=<revision> .)
I still think that your PR #11 can be valuable: Specifying the image instead of a revision is possibly more user-friendly, less (human-)error-prone, and easier to integrate in CI/CD systems. I don't think we need the extra readiness checking anymore though, so if you decide to adjust the logic accordingly, I'd be happy to review once more.
All of this being sad, I think that kubectl rollout status has really improved a lot and could possibly replace the script with the next release. So just waiting for 1.5 to come by and bring the majority of nice features we've been bashing into the script (pun intended) is another option I'd consider totally worthwhile.
Thanks again!
Perfect ! Your implementation is simpler, and more elegant ! Not 100% perfect, but good enough ! We will deal later with the edge cases 👍