diff --git a/internal/controller/bindcluster_controller.go b/internal/controller/bindcluster_controller.go index b4c4279..a9b6204 100644 --- a/internal/controller/bindcluster_controller.go +++ b/internal/controller/bindcluster_controller.go @@ -472,7 +472,7 @@ func (r *BindClusterReconciler) reconfigPods(ctx context.Context, c *bindv1alpha return requeueShort } applied := map[string]string{} - var waiting, failed []string + var waiting, failed, execErrs []string for i := range pods.Items { pod := &pods.Items[i] conf := secondaryConf @@ -483,18 +483,30 @@ func (r *BindClusterReconciler) reconfigPods(ctx context.Context, c *bindv1alpha if have, ok := c.Status.AppliedConfig[pod.Name]; ok { applied[pod.Name] = have } - if applied[pod.Name] == want || !podReady(pod) { + if applied[pod.Name] == want { + continue + } + if !podReady(pod) { + waiting = append(waiting, pod.Name) continue } mounted, err := r.Exec.Exec(ctx, c.Namespace, pod.Name, []string{"cat", bind.NamedConfRun, bind.KeysConfPath}, "") - if err != nil || sha256Hex(mounted) != want { + if err != nil { + execErrs = append(execErrs, fmt.Sprintf("%s: %v", pod.Name, err)) + continue + } + if sha256Hex(mounted) != want { waiting = append(waiting, pod.Name) continue } if err := r.Exec.Reconfig(ctx, c.Namespace, pod.Name); err != nil { - // rndc only reports "failed: "; checkconf names the file and line. - detail, _ := r.Exec.Exec(ctx, c.Namespace, pod.Name, []string{bind.CheckconfBin, bind.NamedConfRun}, "") - failed = append(failed, strings.TrimSpace(fmt.Sprintf("%s: %v %s", pod.Name, err, detail))) + // rndc only reports "failed: "; checkconf names the file and line (on stderr, so in the error). + out, cerr := r.Exec.Exec(ctx, c.Namespace, pod.Name, []string{bind.CheckconfBin, bind.NamedConfRun}, "") + msg := fmt.Sprintf("%s: %v", pod.Name, err) + if cerr != nil { + msg += "; " + strings.TrimSpace(out+" "+cerr.Error()) + } + failed = append(failed, msg) continue } applied[pod.Name] = want @@ -505,12 +517,16 @@ func (r *BindClusterReconciler) reconfigPods(ctx context.Context, c *bindv1alpha var requeue time.Duration switch { case len(failed) > 0: - cond.Status, cond.Reason, cond.Message = metav1.ConditionFalse, "ReconfigFailed", strings.Join(failed, "; ") - requeue = requeueLong - log.FromContext(ctx).Info("rndc reconfig failed", "cluster", c.Name, "errors", cond.Message) + cond.Status, cond.Reason, cond.Message = metav1.ConditionFalse, "ReconfigFailed", strings.Join(append(failed, execErrs...), "; ") + case len(execErrs) > 0: + cond.Status, cond.Reason, cond.Message = metav1.ConditionFalse, "ExecFailed", strings.Join(execErrs, "; ") case len(waiting) > 0: cond.Status, cond.Reason, cond.Message = metav1.ConditionFalse, "Propagating", "awaiting config on "+strings.Join(waiting, ", ") } + if len(failed)+len(execErrs) > 0 { + requeue = requeueLong + log.FromContext(ctx).Info("config apply failed", "cluster", c.Name, "reason", cond.Reason, "errors", cond.Message) + } if len(waiting) > 0 { requeue = requeueShort } diff --git a/internal/controller/reconfig_test.go b/internal/controller/reconfig_test.go index ec39ee6..a486831 100644 --- a/internal/controller/reconfig_test.go +++ b/internal/controller/reconfig_test.go @@ -3,6 +3,7 @@ package controller import ( "context" "errors" + "fmt" "strings" "testing" @@ -12,7 +13,9 @@ import ( "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" clientgoscheme "k8s.io/client-go/kubernetes/scheme" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client/fake" bindv1alpha1 "git.unkin.net/unkin/bind-operator/api/v1alpha1" @@ -31,21 +34,26 @@ func testScheme(t *testing.T) *runtime.Scheme { return s } -// fakeExec serves `cat` from mounted (per pod) and counts reconfigs. +// fakeExec mirrors bind.Executor: stdout is returned, stderr only inside the +// error. It serves `cat` from mounted (per pod) and counts reconfigs. type fakeExec struct { - mounted map[string]string - reconfigErr error - checkconf string - reconfigs map[string]int + mounted map[string]string + catErr map[string]error + reconfigErr error + checkconfStderr string + reconfigs map[string]int } func (f *fakeExec) Exec(_ context.Context, _, pod string, cmd []string, _ string) (string, error) { if len(cmd) == 2 && cmd[0] == bind.CheckconfBin { - return f.checkconf, errors.New("exit 1") + return "", fmt.Errorf("exec %v: command terminated with exit code 1 (stderr: %s)", cmd, f.checkconfStderr) } if len(cmd) != 3 || cmd[0] != "cat" || cmd[1] != bind.NamedConfRun || cmd[2] != bind.KeysConfPath { return "", errors.New("unexpected command") } + if err := f.catErr[pod]; err != nil { + return "", err + } return f.mounted[pod], nil } @@ -167,10 +175,10 @@ func TestReconfigPodsSurfacesFailure(t *testing.T) { cluster := &bindv1alpha1.BindCluster{ObjectMeta: metav1.ObjectMeta{Name: "c", Namespace: "ns"}} cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(readyPod("c", "c-0")).Build() ex := &fakeExec{ - mounted: map[string]string{"c-0": "bad;"}, - reconfigErr: errors.New("rndc: 'reconfig' failed: unexpected token"), - checkconf: "/run/named/named.conf:6: expected IP address near 'not-an-ip'", - reconfigs: map[string]int{}, + mounted: map[string]string{"c-0": "bad;"}, + reconfigErr: errors.New("rndc: 'reconfig' failed: unexpected token"), + reconfigs: map[string]int{}, + checkconfStderr: "/run/named/named.conf:6: expected IP address near 'not-an-ip'", } r := &BindClusterReconciler{Client: cl, Scheme: scheme, Exec: ex} @@ -188,3 +196,126 @@ func TestReconfigPodsSurfacesFailure(t *testing.T) { t.Fatal("failed reconfig must not be recorded as applied") } } + +func notReadyPod(cluster, name string) *corev1.Pod { + p := readyPod(cluster, name) + p.Status.Conditions[0].Status = corev1.ConditionFalse + return p +} + +func TestReconfigPodsSurfacesExecErrorAndBacksOff(t *testing.T) { + scheme := testScheme(t) + cluster := &bindv1alpha1.BindCluster{ObjectMeta: metav1.ObjectMeta{Name: "c", Namespace: "ns"}} + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(readyPod("c", "c-0")).Build() + ex := &fakeExec{ + catErr: map[string]error{"c-0": errors.New(`pods "c-0" is forbidden: cannot create resource "pods/exec"`)}, + reconfigs: map[string]int{}, + } + r := &BindClusterReconciler{Client: cl, Scheme: scheme, Exec: ex} + + if d := r.reconfigPods(context.Background(), cluster, "p;", "s;", ""); d != requeueLong { + t.Fatalf("exec error should back off with requeueLong, got %v", d) + } + c := meta.FindStatusCondition(cluster.Status.Conditions, "ConfigApplied") + if c == nil || c.Status != metav1.ConditionFalse || c.Reason != "ExecFailed" || !strings.Contains(c.Message, "forbidden") { + t.Fatalf("want ExecFailed condition carrying the cause, got %+v", c) + } + if ex.reconfigs["c-0"] != 0 { + t.Fatal("reconfig ran after a failed mount check") + } +} + +func TestReconfigPodsNotReadyStalePodBlocksApplied(t *testing.T) { + scheme := testScheme(t) + cluster := &bindv1alpha1.BindCluster{ObjectMeta: metav1.ObjectMeta{Name: "c", Namespace: "ns"}} + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(readyPod("c", "c-0"), notReadyPod("c", "c-1")).Build() + const primary, secondary = "p;", "s;" + ex := &fakeExec{ + mounted: map[string]string{"c-0": primary}, + catErr: map[string]error{"c-1": errors.New("must not exec into a not-ready pod")}, + reconfigs: map[string]int{}, + } + r := &BindClusterReconciler{Client: cl, Scheme: scheme, Exec: ex} + + if d := r.reconfigPods(context.Background(), cluster, primary, secondary, ""); d != requeueShort { + t.Fatalf("not-ready stale pod should requeue short, got %v", d) + } + c := meta.FindStatusCondition(cluster.Status.Conditions, "ConfigApplied") + if c == nil || c.Status != metav1.ConditionFalse || c.Reason != "Propagating" || !strings.Contains(c.Message, "c-1") { + t.Fatalf("want Propagating naming c-1, got %+v", c) + } + if ex.reconfigs["c-0"] != 1 || ex.reconfigs["c-1"] != 0 { + t.Fatalf("want only the ready pod reconfigured, got %v", ex.reconfigs) + } +} + +func TestReconfigPodsNewPodReconfigsOnceThenRecorded(t *testing.T) { + scheme := testScheme(t) + const primary, secondary = "p;", "s;" + cluster := &bindv1alpha1.BindCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "c", Namespace: "ns"}, + Status: bindv1alpha1.BindClusterStatus{AppliedConfig: map[string]string{"c-0": sha256Hex(primary)}}, + } + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(readyPod("c", "c-0"), readyPod("c", "c-1")).Build() + ex := &fakeExec{mounted: map[string]string{"c-0": primary, "c-1": secondary}, reconfigs: map[string]int{}} + r := &BindClusterReconciler{Client: cl, Scheme: scheme, Exec: ex} + + for range 3 { + if d := r.reconfigPods(context.Background(), cluster, primary, secondary, ""); d != 0 { + t.Fatalf("converged cluster should not requeue, got %v", d) + } + } + if ex.reconfigs["c-0"] != 0 || ex.reconfigs["c-1"] != 1 { + t.Fatalf("want exactly one reconfig for the new pod only, got %v", ex.reconfigs) + } + if cluster.Status.AppliedConfig["c-1"] != sha256Hex(secondary) { + t.Fatalf("new pod not recorded: %v", cluster.Status.AppliedConfig) + } +} + +func TestReconcilePersistsAppliedConfig(t *testing.T) { + scheme := testScheme(t) + cluster := &bindv1alpha1.BindCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "c", Namespace: "ns"}, + Spec: bindv1alpha1.BindClusterSpec{Replicas: 1}, + } + cl := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(cluster, readyPod("c", "c-0")). + WithStatusSubresource(&bindv1alpha1.BindCluster{}). + Build() + ex := &fakeExec{mounted: map[string]string{}, reconfigs: map[string]int{}} + r := &BindClusterReconciler{Client: cl, Scheme: scheme, Exec: ex} + ctx := context.Background() + req := ctrl.Request{NamespacedName: types.NamespacedName{Namespace: "ns", Name: "c"}} + + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatal(err) + } + var cm corev1.ConfigMap + if err := cl.Get(ctx, types.NamespacedName{Namespace: "ns", Name: configMapName("c")}, &cm); err != nil { + t.Fatal(err) + } + var keys corev1.Secret + if err := cl.Get(ctx, types.NamespacedName{Namespace: "ns", Name: keysSecretName("c")}, &keys); err != nil { + t.Fatal(err) + } + mounted := cm.Data["named.conf.primary"] + string(keys.Data["keys.conf"]) + ex.mounted["c-0"] = mounted + + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatal(err) + } + var got bindv1alpha1.BindCluster + if err := cl.Get(ctx, req.NamespacedName, &got); err != nil { + t.Fatal(err) + } + if got.Status.AppliedConfig["c-0"] != sha256Hex(mounted) { + t.Fatalf("appliedConfig not persisted: %v", got.Status.AppliedConfig) + } + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatal(err) + } + if ex.reconfigs["c-0"] != 1 { + t.Fatalf("persisted appliedConfig should prevent a second reconfig, got %d", ex.reconfigs["c-0"]) + } +}