fix --wait's failure to work on coredns pods

[ComradeProgrammer]

FIX #19288
Before: minikube start --wait=all may end when coredns is not ready
After: minikube start --wait=all will be able to wait until coredns is completly ready

The situation mentioned in #19288 was actually introduced by the HA-cluster PR. By default coredns has a deployment consists 2 coredns pods. However in pkg/minikube/node/start.go:158 it was manyally scaled down to 1. This happens before we start to wait for those essential nodes(minikube waits for nodes at line 236).

When minikube waits for system pods, there were 2 checks which will check the system pods's status:

• WaitExtra (pkg/minikube/bootstrapper/bsutil/kverify/pod_ready.go) will list all pods with the given labels, and check whether they are ready
• ExpectAppsRunning will list all the pods in kube-system namespace (pkg/minikube/bootstrapper/bsutil/kverify/system_pods.go:91 , in function ExpectAppsRunning), and check whether there are at least 1 running pod for some essential labels. But the bug is that it only check the running state, and do not check the ready state

After the HA-cluster PR was introduced, when minikube run WaitExtra funtion(the 1st check), one of the coredns pod's status can be Succeed. WaitExtra don't recognize this state and will print an error and break the checking loop. This logic is written at pkg/minikube/bootstrapper/bsutil/kverify/pod_ready.go line 99 and line 69.

The error I see is

E1003 22:37:04.296076   17140 pod_ready.go:66] WaitExtra: waitPodCondition: pod "coredns-7db6d8ff4d-nljst" in "kube-system" namespace has status phase "Succeeded" (skipping!): {Phase:Succeeded Conditions:[{Type:PodReadyToStartContainers Status:False LastProbeTime:0001-01-01 00:00:00 +0000 UTC LastTransitionTime:2024-10-03 22:37:04 +0200 CEST Reason: Message:} {Type:Initialized Status:True LastProbeTime:0001-01-01 00:00:00 +0000 UTC LastTransitionTime:2024-10-03 22:36:53 +0200 CEST Reason:PodCompleted Message:} {Type:Ready Status:False LastProbeTime:0001-01-01 00:00:00 +0000 UTC LastTransitionTime:2024-10-03 22:36:53 +0200 CEST Reason:PodCompleted Message:} {Type:ContainersReady Status:False LastProbeTime:0001-01-01 00:00:00 +0000 UTC LastTransitionTime:2024-10-03 22:36:53 +0200 CEST Reason:PodCompleted Message:} {Type:PodScheduled Status:True LastProbeTime:0001-01-01 00:00:00 +0000 UTC LastTransitionTime:2024-10-03 22:36:53 +0200 CEST Reason: Message:}] Message: Reason: NominatedNodeName: HostIP:192.168.49.2 HostIPs:[{IP:192.168.49.2}] PodIP: PodIPs:[] StartTime:2024-10-03 22:36:53 +0200 CEST InitContainerStatuses:[] ContainerStatuses:[{Name:coredns State:{Waiting:nil Running:nil Terminated:&ContainerStateTerminated{ExitCode:0,Signal:0,Reason:Completed,Message:,StartedAt:2024-10-03 22:36:54 +0200 CEST,FinishedAt:2024-10-03 22:37:04 +0200 CEST,ContainerID:docker://281a8c3106510cdc16a6d3f91ad2e9f5d7aa1609fac4f0e8f7494af67cb8b5d6,}} LastTerminationState:{Waiting:nil Running:nil Terminated:nil} Ready:false RestartCount:0 Image:registry.k8s.io/coredns/coredns:v1.11.1 ImageID:docker-pullable://registry.k8s.io/coredns/coredns@sha256:1eeb4c7316bacb1d4c8ead65571cd92dd21e27359f0d4917f1a5822a73b75db1 ContainerID:docker://281a8c3106510cdc16a6d3f91ad2e9f5d7aa1609fac4f0e8f7494af67cb8b5d6 Started:0x14001e58e00 AllocatedResources:map[] Resources:nil VolumeMounts:[]}] QOSClass:Burstable EphemeralContainerStatuses:[] Resize: ResourceClaimStatuses:[]}

The when minikube arrives at ExpectAppsRunning(the 2nd check), it doesn't check the ready state, so it believes that all pods are ok. This causes the #19288

So the fix is to make ExpectAppsRunning(the 2nd check) check the ready state as well.

(The reason why I didn't make the 1st check function to recognized the Succeed state is that: if for some reason there is a job (e.g. init job for some containers) in kube-system namespace, and we change the WaitExtra's logic to reject Succeed state, there will be problems.)

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ComradeProgrammer
Once this PR has been reviewed and has the lgtm label, please assign spowelljr for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ComradeProgrammer]

/ok-to-test

[minikube-pr-bot]

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 19748) |
+----------------+----------+---------------------+
| minikube start | 70.5s    | 46.7s               |
| enable ingress | 14.7s    | 15.2s               |
+----------------+----------+---------------------+

Times for minikube start: 48.8s 48.7s 159.0s 48.5s 47.4s
Times for minikube (PR 19748) start: 46.9s 47.3s 45.5s 45.4s 48.6s

Times for minikube ingress: 14.5s 14.9s 14.4s 15.0s 14.5s
Times for minikube (PR 19748) ingress: 15.9s 14.4s 14.9s 15.4s 15.5s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 19748) |
+----------------+----------+---------------------+
| minikube start | 21.5s    | 22.0s               |
| enable ingress | 12.3s    | 13.5s               |
+----------------+----------+---------------------+

Times for minikube start: 23.9s 20.5s 20.7s 20.7s 21.7s
Times for minikube (PR 19748) start: 22.8s 22.9s 23.5s 20.2s 20.5s

Times for minikube ingress: 10.3s 12.7s 13.2s 12.2s 12.7s
Times for minikube (PR 19748) ingress: 11.7s 10.8s 12.8s 19.8s 12.3s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 19748) |
+----------------+----------+---------------------+
| minikube start | 20.8s    | 21.0s               |
| enable ingress | 26.6s    | 26.3s               |
+----------------+----------+---------------------+

Times for minikube start: 19.9s 22.5s 19.6s 22.4s 19.6s
Times for minikube (PR 19748) start: 23.2s 20.0s 20.3s 22.4s 18.9s

Times for minikube ingress: 22.8s 39.3s 24.2s 23.3s 23.3s
Times for minikube (PR 19748) ingress: 23.3s 22.7s 23.2s 23.2s 39.2s

[minikube-pr-bot]

Here are the number of top 10 failed tests in each environments with lowest flake rate.

Environment	Test Name	Flake Rate
Docker_Cloud_Shell (4 failed)	TestAddons/serial/GCPAuth/PullSecret(gopogh)	Unknown
Docker_macOS (1 failed)	TestAddons/serial/GCPAuth/PullSecret(gopogh)	Unknown
Docker_Windows (4 failed)	TestAddons/serial/GCPAuth/PullSecret(gopogh)	Unknown
Docker_Windows (4 failed)	TestStartStop/group/old-k8s-version/serial/SecondStart(gopogh)	44.64% (chart)
Docker_Linux_crio (3 failed)	TestAddons/serial/GCPAuth/PullSecret(gopogh)	Unknown
Docker_Linux_crio_arm64 (3 failed)	TestAddons/serial/GCPAuth/PullSecret(gopogh)	Unknown
Docker_Linux (1 failed)	TestAddons/serial/GCPAuth/PullSecret(gopogh)	Unknown
Docker_Linux_docker_arm64 (2 failed)	TestAddons/serial/GCPAuth/PullSecret(gopogh)	Unknown
Docker_Linux_docker_arm64 (2 failed)	TestStartStop/group/old-k8s-version/serial/SecondStart(gopogh)	10.23% (chart)
none_Linux (1 failed)	TestAddons/serial/GCPAuth/PullSecret(gopogh)	Unknown
KVM_Linux (1 failed)	TestAddons/serial/GCPAuth/PullSecret(gopogh)	Unknown

Besides the following environments also have failed tests:

• Hyper-V_Windows: 13 failed (gopogh)

• QEMU_macOS: 15 failed (gopogh)

• KVM_Linux_crio: 13 failed (gopogh)

• Docker_Linux_containerd_arm64: 1 failed (gopogh)

• Hyperkit_macOS: 26 failed (gopogh)

To see the flake rates of all tests by environment, click here.

[prezha]

hey @ComradeProgrammer thanks for looking into this

here's a bit more context that might help you:

wait for system-critical pods' Ready condition implementation was intentionally made alongside the Running checks, ie, the former would be called only if wait was explicitly requested and would add some startup delay, whereas the latter would almost always be called for quickest startup time

so, as the ExpectAppsRunning is called by WaitForAppsRunning, which, in turn, is called by WaitForNode that is always called (with the exception for the first ha control plane), by adding the Ready check to the ExpectAppsRunning, we'd effectively always wait, which is not the intention

on the other hand, only if wait is required, WaitExtra is called by WaitForNode (but before the WaitForAppsRunning) or by restartPrimaryControlPlane

also, for ha + wait, iirc the idea was that we'd be ok with waiting for at least one coredns pod to be ready, as the kube-dns service would take care of routing requests to the pod(s) that can process them

example from #19288 shows only one coredns, so it's not a ha cluster, but makes a very good point:

sometimes the list of pods is pulled before some of the pods have even been created, resulting in them not being in the waiting check

i think that the fix should be made in WaitExtra, and there we could eg, invert the logic so not to list all pods once and then loop through it waiting for each pod that's also on a system-critical list to become Ready (as we do now), but instead to wait until all system-critical pods (which is a fixed list: kverify.CorePodsLabels) became Ready, re-fetching the pod's status as needed

as for Succeed status phase, that means that "All containers in the Pod have terminated in success, and will not be restarted", so it is handled - by skipping it (ie, it will never become Ready, so no point waiting for it)