diff --git a/changelogs/unreleased/10403-kaovilai b/changelogs/unreleased/10403-kaovilai new file mode 100644 index 000000000..e87620ce6 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/pkg/nodeagent/node_agent.go b/pkg/nodeagent/node_agent.go index b449a91f4..6dcdbf89b 100644 --- a/pkg/nodeagent/node_agent.go +++ b/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") } diff --git a/pkg/nodeagent/node_agent_test.go b/pkg/nodeagent/node_agent_test.go index 9bba67ec4..4a406dd39 100644 --- a/pkg/nodeagent/node_agent_test.go +++ b/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",