From f0a4e7d507dbd8f78b2c1a536dc01ef7b1be19af Mon Sep 17 00:00:00 2001 From: Benjamin Schimke Date: Mon, 10 Jun 2024 15:14:43 +0200 Subject: [PATCH] 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) --- src/k8s/cmd/k8s/k8s_x_wait_for.go | 18 ++-- src/k8s/pkg/client/kubernetes/pods.go | 53 ++++++---- src/k8s/pkg/client/kubernetes/pods_test.go | 107 ++++++++++++++++++++ src/k8s/pkg/k8sd/features/calico/status.go | 37 ++----- src/k8s/pkg/k8sd/features/cilium/status.go | 30 +++--- src/k8s/pkg/k8sd/features/coredns/status.go | 24 +++-- src/k8s/pkg/k8sd/features/status.go | 12 +-- 7 files changed, 189 insertions(+), 92 deletions(-) create mode 100644 src/k8s/pkg/client/kubernetes/pods_test.go diff --git a/src/k8s/cmd/k8s/k8s_x_wait_for.go b/src/k8s/cmd/k8s/k8s_x_wait_for.go index 8aa227c39..17f8fc1b2 100644 --- 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 index 9291ca281..62551b144 100644 --- 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 new file mode 100644 index 000000000..5f3922e1f --- /dev/null +++ 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 index bf75ca9c2..423fe7426 100644 --- 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 index ba36c51ee..45b61c79b 100644 --- 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 index 2bb260f7a..629eabe87 100644 --- 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 index 0cf6ac33a..cc20c2f45 100644 --- 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) }