add new extra component to --wait=all to validate a healthy cluster #10424
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @prezha. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
k8s-ci-robot
added
needs-ok-to-test
|
Can one of the admins verify this patch? |
There was a problem hiding this comment.
this is good work, and I agree one thing to consider is,
we should ensure our current "minikube start" won't add additional wait for components
but we also want to make "minikube start --wait=all" to wait for every single thing possible (as purposed in this PR, we should not only rely on pod phase status but also wait for the them be actually running)
so if possible please post in the PR descriptions the time metrics Before and After this PR for "normal start" and then if it adds any new time we can introduce a new component to the wait flag
currently the available options for --wait are
- Default (apiserver,system_pods)
- False (no wait)
- All (everything)
- or a combination of available options "apiserver,system_pods,default_sa,apps_running,node_ready,kubelet"
we can introduce a new option (strict_check) therofore --wait=all will be "apiserver,system_pods,default_sa,apps_running,node_ready,kubelet, strict_check "
note that I dont acutally purpose the name "strict_check" any other name you come up with would be good.
this will allow the user to choose the level of wait they wanna control (so if they rather a fast start but dont care much about strict check they could pass the paramters they want)
but ---wait=all should always give u ALL possible wait to verify everythying
|
@prezha Just curious how you find out this issue, do you get the clue from any log? I also debugged this before but got no luck. If you could kindly share the debugging experience, that would be a very good learning lesson for all! |
|
thank you @medyagh, i've added time metrics to the pr description and commented |
in general: looked at plenty of logs :) - mostly those from failed ci/cd tasks here, but also run locally and then examined run logs and also the containers/services themself, tried to figure out the 'normal'/expected behaviour, then backtraced through the issue and found where it could root... here is an example of a failed job: https://github.com/kubernetes/minikube/pull/10378/checks?check_run_id=1842596262 here is the command i've run locally: you can then ssh into individual containers to see beyond default last 25 lines of logs, or you can use something like: (replace here specifically the test failed on services that were not operational, so i've then backtracked where and when they've been started ( i hope this was not too confusing and somewhat helpful :) |
Yeah, Thanks @prezha for the detail explanation, it's really helpful and appreciate it! |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
kvm2 Driver Times for Minikube (PR 10424): 160.1s 135.9s 143.9s Averages Time Per Log docker Driver Times for Minikube (PR 10424): 110.9s 95.2s 101.6s Averages Time Per Log |
k8s-ci-robot
added
the
needs-rebase
prezha
force-pushed
the
fix-WaitForPod-Ready
branch
from
947cd64 to
93e98de
Compare
k8s-ci-robot
removed
the
needs-rebase
|
after consultation with @medya, i've updated pr so that these strict checks (ie, waiting for pods in |
|
kvm2 Driver Times for Minikube (PR 10424): 156.6s 158.6s 155.0s Averages Time Per Log docker Driver Times for Minikube (PR 10424): 114.2s 109.1s 96.7s Averages Time Per Log |
|
kvm2 Driver Times for Minikube (PR 10424): 71.2s 65.7s 68.3s Averages Time Per Log docker Driver Times for Minikube (PR 10424): 26.5s 24.3s 24.9s Averages Time Per Log |
There was a problem hiding this comment.
it is possible that we do not Respect the --wait flag if it is passed to an Existiing cluster (on a second stat)
maybe the code needs to be fixed in updateExistingConfigFromFlags
// updateExistingConfigFromFlags will update the existing config from the flags - used on a second start
// skipping updating existing docker env , docker opt, InsecureRegistry, registryMirror, extra-config, apiserver-ips
func updateExistingConfigFromFlags(cmd *cobra.Command, existing *config.ClusterConfig) config.ClusterConfig { //nolint to suppress cyclomatic complexity 45 of func `updateExistingConfigFromFlags` is high (> 30)
validateFlags(cmd, existing.Driver)
cc := *existing
There was a problem hiding this comment.
test is still failing
I think what is missing is need to add the new wait component to other places such as the bootstrapper
func (k *Bootstrapper) WaitForNode(
... add new component here
and also may be we need to ensure the other Route of waiting (if the cluster already exists, respects the --wait flags)
or other places in the code that we do waiting after the cluster is already running
@medya do you mean, on subsequent starts, to completely remove minikube/cmd/minikube/cmd/start_flags.go Lines 655 to 657 in 27d86a4 with cc.VerifyComponents = kverify.NoComponents
|
thanks @medyagh, i think i've already added that in the pr: |
i think a good place to intervene is: minikube/cmd/minikube/cmd/start.go Line 178 in 6c2280c here, on subsequent starts, we can simply turn off whichever checks we want... what about restarts of components during the first run - should we still respect |
@prezha but if not flag is set, and user only does "minikube start" we should not add any new things to wait on. |
|
kvm2 Driver Times for Minikube (PR 10424): 66.4s 68.6s 66.5s Averages Time Per Log docker Driver Times for Minikube (PR 10424): 27.4s 26.7s 26.1s Averages Time Per Log |
| // it appears to be immediately Ready as are all kube-system pods | ||
| // then (after ~10sec) it realises it has some changes to apply, implying also pods restarts | ||
| // so we wait for kubelet to initialise itself... | ||
| time.Sleep(10 * time.Second) |
There was a problem hiding this comment.
I dont think this is the right thing to sleep 10 seconds randomly, is there any other way to detect that kubelet is trying to initialize itself ?
There was a problem hiding this comment.
you are right @medyagh and thank you for proposing to use retry.Expo() instead
| @@ -37,16 +37,18 @@ const ( | |||
| NodeReadyKey = "node_ready" | |||
| // KubeletKey is the name used in the flags for waiting for the kubelet status to be ready | |||
| KubeletKey = "kubelet" | |||
| // OperationalKey is the name used for waiting for pods in CorePodsList to be Ready | |||
| OperationalKey = "operational" | |||
There was a problem hiding this comment.
how about we name this, something like
"extra"
There was a problem hiding this comment.
sounds good - renamed
|
kvm2 Driver Times for Minikube (PR 10424): 64.6s 68.6s 68.7s Averages Time Per Log docker Driver Times for Minikube (PR 10424): 26.2s 26.1s 28.2s Averages Time Per Log |
| // WaitForPodReadyByLabel waits for pod with label ([key:]val) in a namespace to be in Ready condition. | ||
| // If namespace is not provided, it defaults to "kube-system". | ||
| // If label key is not provided, it will try with "component" and "k8s-app". | ||
| func WaitForPodReadyByLabel(cs *kubernetes.Clientset, label, namespace string, timeout time.Duration) error { |
There was a problem hiding this comment.
this func is not used outside, make private.
medyagh
changed the title
|
kvm2 Driver Times for Minikube (PR 10424): 69.3s 67.6s 70.5s Averages Time Per Log docker Driver Times for Minikube (PR 10424): 27.9s 26.5s 26.4s Averages Time Per Log |
k8s-ci-robot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: medyagh, prezha 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 |
fixes #9936
fixes #10130
i've slightly improved
validateComponentHealthfunctional test, but in this pr i'm also proposing to change how we check if a pod is actually available -currently, we rely on pod's
Runningstatus, and according toPod phase(ref: https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/#pod-phase):
so it is prereq but not sufficient to consider it operational - we should use
Pod conditions/Readyinstead (ref: https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/#pod-conditions):i've changed accordingly how we wait for
api serverinkubeadm.WaitForNodeas well askverify.WaitForSystemPods- these changes should improve the odds forvalidateComponentHealthfunctional test not to fail, as well as overall stabilitythree additional notes:
kverify.SystemPodsList(eg, remove currently redundant 'kube-apiserver' from the list -or- amend thekverify.*Componentsand removeAPIServerWaitKeyifSystemPodsWaitKeyis defined, as it's using theSystemPodsList)cmd.waitForAPIServerProcessin docker-env, andkverify.waitForAPIServerProcessin apiserver used inkubeadmin.restartControlPlane)healthzendpoint that is deprecated (ref: https://kubernetes.io/docs/reference/using-api/health-checks/#api-endpoints-for-health):time metrics (all completed successfully - here are just resulting values for brevity)
before
after
expectedly, here the time increased for 'plain'
minikube start- explanation:default value for
waitflag iskverify.DefaultWaitList:APIServerWaitKey, SystemPodsWaitKey, so, in thekubeadmin.WaitForNodeit will triggerWaitForPodReadyByLabelfor all system components (incl. apiserver) => currently,minikube startequals tominikube start --wait=allon the other hand:
--wait=allwill force wait for all system components to become fully operational--wait=none, then wait does not happen (no additional wait time is added): should we change the default value for--waittononethen, or we should trigger this strict_check only if explicitly asked for (and also with--all)?one thing may be worth mentioning: just
--waitflag, w/o specifying=xxxdoes not work as (i've) expected - ie, it's not taking the default value (kverify.DefaultWaitList), but it's 'greedy':(and that could be explained with the fact that
=is optional anyway...)