surface exec and checkconf errors on ConfigApplied, count not-ready stale pods as waiting

2026-10-16 23:39:43 +11:00
parent 70e5b0ed55
commit 60b04e5eb1
2 changed files with 166 additions and 19 deletions
@@ -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: <reason>"; 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: <reason>"; 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
 	}
@@ -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"])
+	}
+}