Merge pull request #10403 from velero-io/copilot/follow-up-pr-for-daemonset-checks

Check both daemonsets before returning non-NotFound lookup error in IsReady

changelogs/unreleased/10403-kaovilai

@@ -0,0 +1 @@
+Check both node-agent daemonsets before returning a non-NotFound lookup error in IsReady, so a transient error fetching one daemonset no longer masks the other daemonset being ready

pkg/nodeagent/node_agent.go

@@ -82,13 +82,17 @@ func KbClientIsRunningInNode(ctx context.Context, namespace string, nodeName str
 }
 
 // IsReady checks whether the node-agent daemonset has at least one ready pod
-// by inspecting the DaemonSet status.
+// by inspecting the DaemonSet status. Both the linux and windows daemonsets
+// are checked before returning any non-NotFound lookup error, so that a
+// transient error fetching one daemonset does not mask the other daemonset
+// being ready.
 func IsReady(ctx context.Context, namespace string, crClient ctrlclient.Client) error {
 	dsLinux := new(appsv1api.DaemonSet)
+	var lookupErr error
 	if err := crClient.Get(ctx, ctrlclient.ObjectKey{Namespace: namespace, Name: daemonSet}, dsLinux); err != nil {
 		dsLinux = nil
 		if !apierrors.IsNotFound(err) {
-			return errors.Wrap(err, "failed to get linux node-agent daemonset")
+			lookupErr = errors.Wrap(err, "failed to get linux node-agent daemonset")
 		}
 	}
 
@@ -96,7 +100,7 @@ func IsReady(ctx context.Context, namespace string, crClient ctrlclient.Client)
 	if err := crClient.Get(ctx, ctrlclient.ObjectKey{Namespace: namespace, Name: daemonsetWindows}, dsWindows); err != nil {
 		dsWindows = nil
 		if !apierrors.IsNotFound(err) {
-			return errors.Wrap(err, "failed to get windows node-agent daemonset")
+			lookupErr = errors.CombineErrors(lookupErr, errors.Wrap(err, "failed to get windows node-agent daemonset"))
 		}
 	}
 
@@ -108,6 +112,10 @@ func IsReady(ctx context.Context, namespace string, crClient ctrlclient.Client)
 		return nil
 	}
 
+	if lookupErr != nil {
+		return lookupErr
+	}
+
 	return errors.New("node-agent is not ready: no ready pods found")
 }
 

pkg/nodeagent/node_agent_test.go

@@ -18,6 +18,7 @@ package nodeagent
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	"github.com/cockroachdb/errors"
@@ -275,6 +276,52 @@ func TestIsReady(t *testing.T) {
 			},
 			expectErr: "failed to get windows node-agent daemonset: fake-get-error",
 		},
+		{
+			name:      "linux daemonset get error but windows ready",
+			namespace: "fake-ns",
+			kubeClientObj: []runtime.Object{
+				dsWindowsReady,
+			},
+			interceptor: &interceptor.Funcs{
+				Get: func(ctx context.Context, c ctrlclient.WithWatch, key ctrlclient.ObjectKey, obj ctrlclient.Object, opts ...ctrlclient.GetOption) error {
+					if key.Name == "node-agent" {
+						return errors.New("fake-get-error")
+					}
+					return c.Get(ctx, key, obj, opts...)
+				},
+			},
+		},
+		{
+			name:      "windows daemonset get error but linux ready",
+			namespace: "fake-ns",
+			kubeClientObj: []runtime.Object{
+				dsLinuxReady,
+			},
+			interceptor: &interceptor.Funcs{
+				Get: func(ctx context.Context, c ctrlclient.WithWatch, key ctrlclient.ObjectKey, obj ctrlclient.Object, opts ...ctrlclient.GetOption) error {
+					if key.Name == "node-agent-windows" {
+						return errors.New("fake-get-error")
+					}
+					return c.Get(ctx, key, obj, opts...)
+				},
+			},
+		},
+		{
+			name:      "linux daemonset get error and windows not ready",
+			namespace: "fake-ns",
+			kubeClientObj: []runtime.Object{
+				dsWindowsNotReady,
+			},
+			interceptor: &interceptor.Funcs{
+				Get: func(ctx context.Context, c ctrlclient.WithWatch, key ctrlclient.ObjectKey, obj ctrlclient.Object, opts ...ctrlclient.GetOption) error {
+					if key.Name == "node-agent" {
+						return errors.New("fake-get-error")
+					}
+					return c.Get(ctx, key, obj, opts...)
+				},
+			},
+			expectErr: "failed to get linux node-agent daemonset: fake-get-error",
+		},
 		{
 			name:      "linux ds exist but no ready pods",
 			namespace: "fake-ns",
@@ -362,6 +409,34 @@ func TestIsReady(t *testing.T) {
 	}
 }
 
+// TestIsReadyBothDaemonsetsGetError ensures that when both daemonset lookups
+// return a non-NotFound error, the linux error is returned as the primary
+// error while the windows error is retained as a secondary/attached error
+// rather than being silently discarded.
+func TestIsReadyBothDaemonsetsGetError(t *testing.T) {
+	scheme := runtime.NewScheme()
+	appsv1api.AddToScheme(scheme)
+
+	fakeClient := clientFake.NewClientBuilder().
+		WithScheme(scheme).
+		WithInterceptorFuncs(interceptor.Funcs{
+			Get: func(ctx context.Context, c ctrlclient.WithWatch, key ctrlclient.ObjectKey, obj ctrlclient.Object, opts ...ctrlclient.GetOption) error {
+				if key.Name == "node-agent" {
+					return errors.New("fake-linux-get-error")
+				}
+				if key.Name == "node-agent-windows" {
+					return errors.New("fake-windows-get-error")
+				}
+				return c.Get(ctx, key, obj, opts...)
+			},
+		}).
+		Build()
+
+	err := IsReady(t.Context(), "fake-ns", fakeClient)
+	require.EqualError(t, err, "failed to get linux node-agent daemonset: fake-linux-get-error")
+	assert.Contains(t, fmt.Sprintf("%+v", err), "failed to get windows node-agent daemonset: fake-windows-get-error")
+}
+
 func TestGetPodSpec(t *testing.T) {
 	podSpec := corev1api.PodSpec{
 		NodeName: "fake-node",