Merge pull request #30913 from Random-Liu/fix-readiness-check

Automatic merge from submit-queue

Node E2E: Make readiness check handling process exits with 0 exit code.

As is mentioned by @mtaufen:
 "there is a problem with the way service `start` is currently implemented in test/e2e_node/e2e_service.go. If the Kubelet exits with status 0 before the health check completes, cmdErrorChan will be closed and, as a result, nil will be read from that channel, and you will return a nil error from `start`."

This PR changes the logic to:
1) If the err channel returns an error, return the error
2) If the err channel returns a nil, ignore it and continue checking readiness.
3) If the err channel is closed before readiness check succeeds, replace it with `blockCh` and continue checking readiness.

@mtaufen 
/cc @kubernetes/sig-node

test/e2e_node/e2e_service.go

@@ -433,10 +433,27 @@ func (s *server) String() string {
 // TODO(random-liu): Move this to util
 func readinessCheck(urls []string, errCh <-chan error) error {
 	endTime := time.Now().Add(*serverStartTimeout)
+	blockCh := make(chan error)
+	defer close(blockCh)
 	for endTime.After(time.Now()) {
 		select {
-		case err := <-errCh:
+		// We *always* want to run the health check if there is no error on the channel.
+		// With systemd, reads from errCh report nil because cmd.Run() waits
+		// on systemd-run, rather than the service process. systemd-run quickly
+		// exits with status 0, causing the channel to be closed with no error. In
+		// this case, you want to wait for the health check to complete, rather
+		// than returning from readinessCheck as soon as the channel is closed.
+		case err, ok := <-errCh:
+			if ok { // The channel is not closed, this is a real error
+				if err != nil { // If there is an error, return it
 					return err
+				}
+				// If not, keep checking readiness.
+			} else { // The channel is closed, this is only a zero value.
+				// Replace the errCh with blockCh to avoid busy loop,
+				// and keep checking readiness.
+				errCh = blockCh
+			}
 		case <-time.After(time.Second):
 			ready := true
 			for _, url := range urls {