Cleanup Status check code (#481)

* Refactor IsPodReady to `CheckForReadyPods` and add go-docs * Add unittest for `CheckForReadyPods` * Remove `ListPods` * Refactor `CheckNetwork` and `CheckDNS` to only return error (nil->success)
canonical · Jun 17, 2024 · 0f86604 · 0f86604
1 parent 44d0f53
commit 0f86604
Show file tree

Hide file tree

Showing 7 changed files with 189 additions and 92 deletions.
diff --git a/src/k8s/cmd/k8s/k8s_x_wait_for.go b/src/k8s/cmd/k8s/k8s_x_wait_for.go
@@ -21,12 +21,11 @@ func newXWaitForCmd(env cmdutil.ExecutionEnvironment) *cobra.Command {
 			ctx, cancel := context.WithTimeout(cmd.Context(), opts.timeout)
 			defer cancel()
 			if err := control.WaitUntilReady(ctx, func() (bool, error) {
-				ok, err := features.StatusChecks.CheckDNS(cmd.Context(), env.Snap)
-				if ok {
-					return true, nil
+				err := features.StatusChecks.CheckDNS(cmd.Context(), env.Snap)
+				if err != nil {
+					cmd.PrintErrf("DNS not ready yet: %v\n", err.Error())
 				}
-				cmd.PrintErrf("DNS not ready yet: %v\n", err.Error())
-				return false, nil
+				return err == nil, nil
 			}); err != nil {
 				cmd.PrintErrf("Error: DNS did not become ready: %v\n", err)
 				env.Exit(1)
@@ -42,12 +41,11 @@ func newXWaitForCmd(env cmdutil.ExecutionEnvironment) *cobra.Command {
 			ctx, cancel := context.WithTimeout(cmd.Context(), opts.timeout)
 			defer cancel()
 			if err := control.WaitUntilReady(ctx, func() (bool, error) {
-				ok, err := features.StatusChecks.CheckNetwork(cmd.Context(), env.Snap)
-				if ok {
-					return true, nil
+				err := features.StatusChecks.CheckNetwork(cmd.Context(), env.Snap)
+				if err != nil {
+					cmd.PrintErrf("network not ready yet: %v\n", err.Error())
 				}
-				cmd.PrintErrf("network not ready yet: %v\n", err.Error())
-				return false, nil
+				return err == nil, nil
 			}); err != nil {
 				cmd.PrintErrf("Error: network did not become ready: %v\n", err)
 				env.Exit(1)

diff --git a/src/k8s/pkg/client/kubernetes/pods.go b/src/k8s/pkg/client/kubernetes/pods.go
@@ -3,41 +3,52 @@ package kubernetes
 import (
 	"context"
 	"fmt"
-	"strings"
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-// IsPodReady checks if a pod is ready.
-func (c *Client) IsPodReady(ctx context.Context, name, namespace string, listOptions metav1.ListOptions) (bool, error) {
+// CheckForReadyPods checks if all pods in the specified namespace are ready.
+// It returns an error if any of the pods are not ready.
+// The listOptions specify additional options for listing the pods, e.g. labels.
+// It returns an error if it fails to list the pods or if there are no pods in the namespace.
+// If any of the pods are not ready, it returns an error with the names of the not ready pods.
+// If all pods are ready, it returns nil.
+func (c *Client) CheckForReadyPods(ctx context.Context, namespace string, listOptions metav1.ListOptions) error {
 	pods, err := c.CoreV1().Pods(namespace).List(ctx, listOptions)
 	if err != nil {
-		return false, fmt.Errorf("failed to list pods: %w", err)
+		return fmt.Errorf("failed to list pods: %w", err)
+	}
+	if len(pods.Items) == 0 {
+		return fmt.Errorf("no pods in %v namespace on the cluster", namespace)
 	}
 
+	notReadyPods := []string{}
 	for _, pod := range pods.Items {
-		if strings.Contains(pod.Name, name) {
-			if pod.Status.Phase != corev1.PodRunning {
-				return false, nil
-			}
-
-			for _, condition := range pod.Status.Conditions {
-				if condition.Type == corev1.PodReady && condition.Status == corev1.ConditionTrue {
-					return true, nil
-				}
-			}
+		if !podIsReady(pod) {
+			notReadyPods = append(notReadyPods, pod.Name)
 		}
 	}
 
-	return false, nil
+	if len(notReadyPods) > 0 {
+		return fmt.Errorf("pods %v not ready", notReadyPods)
+	}
+	return nil
 }
 
-// ListPods lists all pods in a namespace.
-func (c *Client) ListPods(ctx context.Context, namespace string, listOptions metav1.ListOptions) ([]corev1.Pod, error) {
-	pods, err := c.CoreV1().Pods(namespace).List(ctx, listOptions)
-	if err != nil {
-		return nil, fmt.Errorf("failed to list pods: %w", err)
+// podIsReady checks if a pod is in the ready state.
+// It returns true if the pod is running (Condition "Ready" = true).
+// Otherwise, it returns false.
+func podIsReady(pod corev1.Pod) bool {
+	if pod.Status.Phase != corev1.PodRunning {
+		return false
 	}
-	return pods.Items, nil
+
+	for _, condition := range pod.Status.Conditions {
+		if condition.Type == corev1.PodReady && condition.Status == corev1.ConditionTrue {
+			return true
+		}
+	}
+
+	return false
 }
diff --git a/src/k8s/pkg/client/kubernetes/pods_test.go b/src/k8s/pkg/client/kubernetes/pods_test.go
@@ -0,0 +1,107 @@
+package kubernetes
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	. "github.com/onsi/gomega"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/kubernetes/fake"
+	k8stesting "k8s.io/client-go/testing"
+)
+
+func TestCheckForReadyPods(t *testing.T) {
+	testCases := []struct {
+		name          string
+		namespace     string
+		listOptions   metav1.ListOptions
+		podList       *corev1.PodList
+		listError     error
+		expectedError string
+	}{
+		{
+			name:          "No pods",
+			namespace:     "test-namespace",
+			podList:       &corev1.PodList{},
+			expectedError: "no pods in test-namespace namespace on the cluster",
+		},
+		{
+			name:      "All pods ready",
+			namespace: "test-namespace",
+			podList: &corev1.PodList{
+				Items: []corev1.Pod{
+					{
+						ObjectMeta: metav1.ObjectMeta{Name: "pod1"},
+						Status: corev1.PodStatus{
+							Phase: corev1.PodRunning,
+							Conditions: []corev1.PodCondition{
+								{Type: corev1.PodReady, Status: corev1.ConditionTrue},
+							},
+						},
+					},
+				},
+			},
+			expectedError: "",
+		},
+		{
+			name:      "Some pods not ready",
+			namespace: "test-namespace",
+			podList: &corev1.PodList{
+				Items: []corev1.Pod{
+					{
+						ObjectMeta: metav1.ObjectMeta{Name: "pod1"},
+						Status: corev1.PodStatus{
+							Phase: corev1.PodRunning,
+							Conditions: []corev1.PodCondition{
+								{Type: corev1.PodReady, Status: corev1.ConditionTrue},
+							},
+						},
+					},
+					{
+						ObjectMeta: metav1.ObjectMeta{Name: "pod2"},
+						Status: corev1.PodStatus{
+							Phase: corev1.PodPending,
+						},
+					},
+				},
+			},
+			expectedError: "pods [pod2] not ready",
+		},
+		{
+			name:          "Error listing pods",
+			namespace:     "test-namespace",
+			listError:     fmt.Errorf("list error"),
+			expectedError: "failed to list pods: list error",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			clientset := fake.NewSimpleClientset()
+			client := &Client{
+				Interface: clientset,
+			}
+
+			// Setup fake client responses
+			clientset.PrependReactor("list", "pods", func(action k8stesting.Action) (bool, runtime.Object, error) {
+				if tc.listError != nil {
+					return true, nil, tc.listError
+				}
+				return true, tc.podList, nil
+			})
+
+			err := client.CheckForReadyPods(context.Background(), tc.namespace, tc.listOptions)
+
+			if tc.expectedError == "" {
+				g.Expect(err).Should(BeNil())
+			} else {
+				g.Expect(err).Should(MatchError(tc.expectedError))
+			}
+		})
+	}
+}
diff --git a/src/k8s/pkg/k8sd/features/calico/status.go b/src/k8s/pkg/k8sd/features/calico/status.go
@@ -6,30 +6,15 @@ import (
 
 	"github.com/canonical/k8s/pkg/snap"
 
-	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-func podIsReady(pod v1.Pod) bool {
-	if pod.Status.Phase != v1.PodRunning {
-		return false
-	}
-
-	for _, condition := range pod.Status.Conditions {
-		if condition.Type == v1.PodReady && condition.Status == v1.ConditionTrue {
-			return true
-		}
-	}
-
-	return false
-}
-
 // CheckNetwork checks the status of the Calico pods in the Kubernetes cluster.
 // We verify that the tigera-operator and calico-node pods are Ready and in Running state.
-func CheckNetwork(ctx context.Context, snap snap.Snap) (bool, error) {
+func CheckNetwork(ctx context.Context, snap snap.Snap) error {
 	client, err := snap.KubernetesClient("calico-system")
 	if err != nil {
-		return false, fmt.Errorf("failed to create kubernetes client: %w", err)
+		return fmt.Errorf("failed to create kubernetes client: %w", err)
 	}
 
 	for _, check := range []struct {
@@ -42,22 +27,12 @@ func CheckNetwork(ctx context.Context, snap snap.Snap) (bool, error) {
 		// check that calico-node pods are ready
 		{name: "calico-node", namespace: "calico-system", labels: map[string]string{"app.kubernetes.io/name": "calico-node"}},
 	} {
-		pods, err := client.ListPods(ctx, check.namespace, metav1.ListOptions{
+		if err := client.CheckForReadyPods(ctx, check.namespace, metav1.ListOptions{
 			LabelSelector: metav1.FormatLabelSelector(&metav1.LabelSelector{MatchLabels: check.labels}),
-		})
-		if err != nil {
-			return false, fmt.Errorf("failed to get %v pods: %w", check.name, err)
-		}
-		if len(pods) == 0 {
-			return false, fmt.Errorf("no %v pods exist on the cluster", check.name)
-		}
-
-		for _, pod := range pods {
-			if !podIsReady(pod) {
-				return false, fmt.Errorf("%v pod %q not ready", check.name, pod.Name)
-			}
+		}); err != nil {
+			return fmt.Errorf("%v pods not yet ready: %w", check.name, err)
 		}
 	}
 
-	return true, nil
+	return nil
 }
diff --git a/src/k8s/pkg/k8sd/features/cilium/status.go b/src/k8s/pkg/k8sd/features/cilium/status.go
@@ -9,26 +9,26 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-func CheckNetwork(ctx context.Context, snap snap.Snap) (bool, error) {
+func CheckNetwork(ctx context.Context, snap snap.Snap) error {
 	client, err := snap.KubernetesClient("kube-system")
 	if err != nil {
-		return false, fmt.Errorf("failed to create kubernetes client: %w", err)
+		return fmt.Errorf("failed to create kubernetes client: %w", err)
 	}
 
-	ciliumPods := map[string]string{
-		"cilium-operator": "io.cilium/app=operator",
-		"cilium":          "k8s-app=cilium",
-	}
-
-	for ciliumPod, selector := range ciliumPods {
-		isReady, err := client.IsPodReady(ctx, ciliumPod, "kube-system", metav1.ListOptions{LabelSelector: selector})
-		if err != nil {
-			return false, fmt.Errorf("failed to check if pod %q is ready: %w", ciliumPod, err)
-		}
-		if !isReady {
-			return false, fmt.Errorf("cilium pod %q is not yet ready", ciliumPod)
+	for _, check := range []struct {
+		name      string
+		namespace string
+		labels    map[string]string
+	}{
+		{name: "cilium-operator", namespace: "kube-system", labels: map[string]string{"io.cilium/app": "operator"}},
+		{name: "cilium", namespace: "kube-system", labels: map[string]string{"k8s-app": "cilium"}},
+	} {
+		if err := client.CheckForReadyPods(ctx, check.namespace, metav1.ListOptions{
+			LabelSelector: metav1.FormatLabelSelector(&metav1.LabelSelector{MatchLabels: check.labels}),
+		}); err != nil {
+			return fmt.Errorf("%v pods not yet ready: %w", check.name, err)
 		}
 	}
 
-	return true, nil
+	return nil
 }
diff --git a/src/k8s/pkg/k8sd/features/coredns/status.go b/src/k8s/pkg/k8sd/features/coredns/status.go
@@ -10,19 +10,25 @@ import (
 )
 
 // CheckDNS checks the CoreDNS deployment in the cluster.
-func CheckDNS(ctx context.Context, snap snap.Snap) (bool, error) {
+func CheckDNS(ctx context.Context, snap snap.Snap) error {
 	client, err := snap.KubernetesClient("kube-system")
 	if err != nil {
-		return false, fmt.Errorf("failed to create kubernetes client: %w", err)
+		return fmt.Errorf("failed to create kubernetes client: %w", err)
 	}
 
-	isReady, err := client.IsPodReady(ctx, "coredns", "kube-system", metav1.ListOptions{LabelSelector: "app.kubernetes.io/name=coredns"})
-	if err != nil {
-		return false, fmt.Errorf("failed to wait for CoreDNS pod to be ready: %w", err)
-	}
-	if !isReady {
-		return false, fmt.Errorf("coredns pod not ready yet")
+	for _, check := range []struct {
+		name      string
+		namespace string
+		labels    map[string]string
+	}{
+		{name: "coredns", namespace: "kube-system", labels: map[string]string{"app.kubernetes.io/name": "coredns"}},
+	} {
+		if err := client.CheckForReadyPods(ctx, check.namespace, metav1.ListOptions{
+			LabelSelector: metav1.FormatLabelSelector(&metav1.LabelSelector{MatchLabels: check.labels}),
+		}); err != nil {
+			return fmt.Errorf("%v pods not yet ready: %w", check.name, err)
+		}
 	}
 
-	return isReady, nil
+	return nil
 }
diff --git a/src/k8s/pkg/k8sd/features/status.go b/src/k8s/pkg/k8sd/features/status.go
@@ -7,19 +7,19 @@ import (
 )
 
 type StatusInterface interface {
-	CheckDNS(context.Context, snap.Snap) (bool, error)
-	CheckNetwork(context.Context, snap.Snap) (bool, error)
+	CheckDNS(context.Context, snap.Snap) error
+	CheckNetwork(context.Context, snap.Snap) error
 }
 
 type statusChecks struct {
-	checkDNS     func(context.Context, snap.Snap) (bool, error)
-	checkNetwork func(context.Context, snap.Snap) (bool, error)
+	checkDNS     func(context.Context, snap.Snap) error
+	checkNetwork func(context.Context, snap.Snap) error
 }
 
-func (s *statusChecks) CheckDNS(ctx context.Context, snap snap.Snap) (bool, error) {
+func (s *statusChecks) CheckDNS(ctx context.Context, snap snap.Snap) error {
 	return s.checkDNS(ctx, snap)
 }
 
-func (s *statusChecks) CheckNetwork(ctx context.Context, snap snap.Snap) (bool, error) {
+func (s *statusChecks) CheckNetwork(ctx context.Context, snap snap.Snap) error {
 	return s.checkNetwork(ctx, snap)
 }