From a0bb0004864df02b30bd936d7dd2300c0d189396 Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Thu, 18 Jun 2026 21:23:04 +0500 Subject: [PATCH 1/2] =?UTF-8?q?feat(#90):=20cluster=20doctor=20=E2=80=94?= =?UTF-8?q?=20node-fit=20+=20image-pull=20checks=20(WS3=20follow-up)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two read-only checks added to `tracebloc cluster doctor` (follow-up to #89): - Node capacity: parses the resource requests jobs-manager stamps on spawned training jobs (RESOURCE_REQUESTS / GPU_REQUESTS env) and checks at least one Ready node can fit them — the "Pending forever, no node big enough" class. GPU is soft: a hard ✖ only on cpu/mem, and a ⚠ when a GPU is requested but no node exposes it (jobs-manager has a GPU->CPU fallback). - Image pull secret: when jobs-manager references a registry pull secret, verifies it exists and is a well-formed dockerconfigjson so private-image pulls don't ImagePullBackOff. Both read-only/best-effort, tested with client-go's fake clientset. The in-cluster egress probe (the third deferred check on #90) is intentionally a separate PR — it needs a port-forward/exec mechanism, not this read-only pattern. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/doctor/doctor.go | 175 +++++++++++++++++++++++++++++++++ internal/doctor/doctor_test.go | 139 +++++++++++++++++++++++++- 2 files changed, 312 insertions(+), 2 deletions(-) diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 6e6ee7ee..f282a1b1 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -12,6 +12,7 @@ package doctor import ( "context" + "encoding/json" "fmt" "net/http" "sort" @@ -20,6 +21,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" @@ -108,6 +110,8 @@ func Run(ctx context.Context, cs kubernetes.Interface, opts Options) []Result { checkReachable(release, relErr, ns), checkPods(ctx, cs, ns), checkPVC(ctx, cs, ns), + checkNodeFit(ctx, cs, jmEnv), + checkImagePull(ctx, cs, ns, release), checkProxy(jmEnv), checkBackendEgress(ctx, jmEnv, opts.HTTPProbe), checkRequestsProxy(ctx, cs, ns, release), @@ -320,6 +324,177 @@ func checkRequestsProxy(ctx context.Context, cs kubernetes.Interface, ns string, return Result{Name: name, Status: StatusOK, Detail: "requests-proxy ready (brokers the 'experiments' queue)"} } +// checkNodeFit verifies at least one Ready node can satisfy the resource +// requests the jobs-manager stamps on spawned training jobs (RESOURCE_REQUESTS +// / GPU_REQUESTS env) — the "Pending forever, no node big enough" class. GPU is +// soft: when a GPU is requested but no node exposes it, that's a ⚠ (jobs-manager +// has a GPU→CPU fallback), not a hard failure. +func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]string) Result { + const name = "Node capacity" + cpuReq, memReq, ok := parseCPUMem(env["RESOURCE_REQUESTS"]) + if !ok { + return Result{ + Name: name, + Status: StatusWarn, + Detail: "couldn't read RESOURCE_REQUESTS from jobs-manager — skipping node-fit", + Remedy: "kubectl set env deploy/-jobs-manager --list | grep RESOURCE_REQUESTS", + } + } + gpuName, gpuReq, gpuRequested := parseGPU(env["GPU_REQUESTS"]) + + nodes, err := cs.CoreV1().Nodes().List(ctx, metav1.ListOptions{}) + if err != nil { + return Result{ + Name: name, + Status: StatusWarn, + Detail: "could not list nodes: " + err.Error(), + Remedy: "Ensure your kubeconfig user can list nodes.", + } + } + + req := fmt.Sprintf("cpu=%s, memory=%s", cpuReq.String(), memReq.String()) + if gpuRequested { + req += fmt.Sprintf(", %s=%s", gpuName, gpuReq.String()) + } + + var cpuMemFits, gpuFits bool + for i := range nodes.Items { + n := nodes.Items[i] + if !nodeReady(n) { + continue + } + alloc := n.Status.Allocatable + if alloc.Cpu().Cmp(cpuReq) >= 0 && alloc.Memory().Cmp(memReq) >= 0 { + cpuMemFits = true + } + if gpuRequested { + if q, present := alloc[gpuName]; present && q.Cmp(gpuReq) >= 0 { + gpuFits = true + } + } + } + + switch { + case !cpuMemFits: + return Result{ + Name: name, + Status: StatusFail, + Detail: fmt.Sprintf("no Ready node can fit a training job (needs %s)", req), + Remedy: "Add/resize a node to meet the job's requests, or lower RESOURCE_REQUESTS on jobs-manager.", + } + case gpuRequested && !gpuFits: + return Result{ + Name: name, + Status: StatusWarn, + Detail: fmt.Sprintf("no node exposes %s — GPU jobs rely on the CPU fallback (needs %s)", gpuName, req), + Remedy: "If GPU training is expected, ensure a GPU node + its device plugin are present.", + } + default: + return Result{Name: name, Status: StatusOK, Detail: fmt.Sprintf("a Ready node can schedule a training job (%s)", req)} + } +} + +// checkImagePull verifies that any registry pull secret the jobs-manager +// references exists and is a well-formed dockerconfigjson — so private-image +// pulls don't ImagePullBackOff. (Bad-but-well-formed credentials can't be +// detected without an actual pull; this catches a missing/empty/malformed +// secret, the common misconfig.) +func checkImagePull(ctx context.Context, cs kubernetes.Interface, ns string, release *cluster.ParentRelease) Result { + const name = "Image pull secret" + dep := findDeployment(ctx, cs, ns, release, "jobs-manager") + if dep == nil { + return Result{ + Name: name, + Status: StatusWarn, + Detail: "couldn't read jobs-manager to resolve image pull secrets — skipping", + Remedy: "Check the parent client release is installed in " + ns + ".", + } + } + secrets := dep.Spec.Template.Spec.ImagePullSecrets + if len(secrets) == 0 { + return Result{Name: name, Status: StatusOK, Detail: "no image pull secret in use (public/digest-pinned images)"} + } + for _, ref := range secrets { + sec, err := cs.CoreV1().Secrets(ns).Get(ctx, ref.Name, metav1.GetOptions{}) + if err != nil { + return Result{ + Name: name, + Status: StatusFail, + Detail: fmt.Sprintf("image pull secret %q not found", ref.Name), + Remedy: "Private image pulls will ImagePullBackOff. Reinstall the chart with valid registry credentials.", + } + } + if sec.Type != corev1.SecretTypeDockerConfigJson { + return Result{ + Name: name, + Status: StatusFail, + Detail: fmt.Sprintf("secret %q is type %q, not %s", ref.Name, sec.Type, corev1.SecretTypeDockerConfigJson), + Remedy: "Recreate it as a docker-registry secret (kubectl create secret docker-registry).", + } + } + if data := sec.Data[corev1.DockerConfigJsonKey]; len(data) == 0 || !json.Valid(data) { + return Result{ + Name: name, + Status: StatusFail, + Detail: fmt.Sprintf("secret %q has an empty or malformed %s", ref.Name, corev1.DockerConfigJsonKey), + Remedy: "Recreate the registry secret; its .dockerconfigjson isn't valid JSON.", + } + } + } + return Result{Name: name, Status: StatusOK, Detail: fmt.Sprintf("%d image pull secret(s) present and well-formed", len(secrets))} +} + +// parseResourceSpec parses jobs-manager's "k1=v1,k2=v2" resource env into a map. +func parseResourceSpec(spec string) map[string]string { + out := map[string]string{} + for _, part := range strings.Split(spec, ",") { + kv := strings.SplitN(strings.TrimSpace(part), "=", 2) + if len(kv) == 2 && strings.TrimSpace(kv[0]) != "" { + out[strings.TrimSpace(kv[0])] = strings.TrimSpace(kv[1]) + } + } + return out +} + +// parseCPUMem extracts the cpu + memory quantities from a RESOURCE_REQUESTS +// spec; ok is false unless both are present and parseable. +func parseCPUMem(spec string) (cpu, mem resource.Quantity, ok bool) { + m := parseResourceSpec(spec) + c, cOK := m["cpu"] + mm, mOK := m["memory"] + if !cOK || !mOK { + return resource.Quantity{}, resource.Quantity{}, false + } + cpu, errC := resource.ParseQuantity(c) + mem, errM := resource.ParseQuantity(mm) + if errC != nil || errM != nil { + return resource.Quantity{}, resource.Quantity{}, false + } + return cpu, mem, true +} + +// parseGPU extracts the GPU resource name + quantity from a GPU_REQUESTS spec +// (e.g. "nvidia.com/gpu=1"). requested is false when absent, unparseable, or 0. +func parseGPU(spec string) (name corev1.ResourceName, qty resource.Quantity, requested bool) { + for k, v := range parseResourceSpec(spec) { + q, err := resource.ParseQuantity(v) + if err == nil && !q.IsZero() { + return corev1.ResourceName(k), q, true + } + } + return "", resource.Quantity{}, false +} + +// nodeReady reports whether a node's Ready condition is True. +func nodeReady(n corev1.Node) bool { + for _, c := range n.Status.Conditions { + if c.Type == corev1.NodeReady { + return c.Status == corev1.ConditionTrue + } + } + return false +} + // jobsManagerEnv reads jobs-manager's first-container plain env into a map // (valueFrom entries have no literal value and are skipped). Best-effort: // returns an empty map when the deployment can't be fetched. diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index a47fd186..7c2b5c2a 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -9,6 +9,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/fake" @@ -318,10 +319,12 @@ func TestRun_HealthyCluster(t *testing.T) { jobsManagerDep(rel, corev1.EnvVar{Name: "REQUESTS_PROXY_URL", Value: "http://requests-proxy-service:8888"}, corev1.EnvVar{Name: "CLIENT_ENV", Value: "dev"}, + corev1.EnvVar{Name: "RESOURCE_REQUESTS", Value: "cpu=2,memory=8Gi"}, ), requestsProxyDep(rel, 1), boundPVC(), runningPod("tb-jobs-manager-abc"), + node("n1", "4", "16Gi"), ) results := Run(bg(), cs, Options{ @@ -329,8 +332,8 @@ func TestRun_HealthyCluster(t *testing.T) { HTTPProbe: func(context.Context, string) error { return nil }, }) - if len(results) != 6 { - t.Fatalf("want 6 checks, got %d", len(results)) + if len(results) != 8 { + t.Fatalf("want 8 checks, got %d", len(results)) } if w := Worst(results); w != StatusOK { for _, r := range results { @@ -339,3 +342,135 @@ func TestRun_HealthyCluster(t *testing.T) { t.Fatalf("healthy cluster worst = %v, want ok", w) } } + +func node(name, cpu, mem string, gpu ...string) *corev1.Node { + alloc := corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse(cpu), + corev1.ResourceMemory: resource.MustParse(mem), + } + if len(gpu) == 2 { + alloc[corev1.ResourceName(gpu[0])] = resource.MustParse(gpu[1]) + } + return &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Status: corev1.NodeStatus{ + Allocatable: alloc, + Conditions: []corev1.NodeCondition{{Type: corev1.NodeReady, Status: corev1.ConditionTrue}}, + }, + } +} + +func TestParseCPUMem(t *testing.T) { + cpu, mem, ok := parseCPUMem("cpu=2,memory=8Gi") + if !ok || cpu.String() != "2" || mem.String() != "8Gi" { + t.Fatalf("parseCPUMem => %q %q %v", cpu.String(), mem.String(), ok) + } + if _, _, ok := parseCPUMem("cpu=2"); ok { + t.Fatalf("missing memory should be !ok") + } + if _, _, ok := parseCPUMem("cpu=abc,memory=8Gi"); ok { + t.Fatalf("unparseable cpu should be !ok") + } +} + +func TestParseGPU(t *testing.T) { + name, qty, req := parseGPU("nvidia.com/gpu=1") + if !req || string(name) != "nvidia.com/gpu" || qty.String() != "1" { + t.Fatalf("parseGPU => %q %q %v", name, qty.String(), req) + } + if _, _, req := parseGPU("nvidia.com/gpu=0"); req { + t.Fatalf("zero gpu should be !requested") + } + if _, _, req := parseGPU(""); req { + t.Fatalf("empty should be !requested") + } +} + +func TestCheckNodeFit(t *testing.T) { + full := map[string]string{"RESOURCE_REQUESTS": "cpu=2,memory=8Gi", "GPU_REQUESTS": "nvidia.com/gpu=1"} + cpuOnly := map[string]string{"RESOURCE_REQUESTS": "cpu=2,memory=8Gi"} + + t.Run("fits cpu+mem+gpu", func(t *testing.T) { + cs := fake.NewClientset(node("n1", "4", "16Gi", "nvidia.com/gpu", "2")) + if r := checkNodeFit(bg(), cs, full); r.Status != StatusOK { + t.Fatalf("=> %v (%q), want ok", r.Status, r.Detail) + } + }) + t.Run("no node big enough -> fail", func(t *testing.T) { + cs := fake.NewClientset(node("n1", "1", "2Gi")) + if r := checkNodeFit(bg(), cs, cpuOnly); r.Status != StatusFail { + t.Fatalf("=> %v (%q), want fail", r.Status, r.Detail) + } + }) + t.Run("gpu requested but none -> warn", func(t *testing.T) { + cs := fake.NewClientset(node("n1", "4", "16Gi")) // cpu/mem fit, no gpu + if r := checkNodeFit(bg(), cs, full); r.Status != StatusWarn { + t.Fatalf("=> %v (%q), want warn", r.Status, r.Detail) + } + }) + t.Run("not-ready node doesn't count -> fail", func(t *testing.T) { + n := node("n1", "8", "32Gi") + n.Status.Conditions = []corev1.NodeCondition{{Type: corev1.NodeReady, Status: corev1.ConditionFalse}} + cs := fake.NewClientset(n) + if r := checkNodeFit(bg(), cs, cpuOnly); r.Status != StatusFail { + t.Fatalf("=> %v (%q), want fail (node not ready)", r.Status, r.Detail) + } + }) + t.Run("missing RESOURCE_REQUESTS -> warn", func(t *testing.T) { + cs := fake.NewClientset(node("n1", "4", "16Gi")) + if r := checkNodeFit(bg(), cs, map[string]string{}); r.Status != StatusWarn { + t.Fatalf("=> %v (%q), want warn", r.Status, r.Detail) + } + }) +} + +func dockerSecret(name string, data []byte) *corev1.Secret { + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Type: corev1.SecretTypeDockerConfigJson, + Data: map[string][]byte{corev1.DockerConfigJsonKey: data}, + } +} + +func jmDepWithPullSecret(release, secretName string) *appsv1.Deployment { + d := jobsManagerDep(release) + if secretName != "" { + d.Spec.Template.Spec.ImagePullSecrets = []corev1.LocalObjectReference{{Name: secretName}} + } + return d +} + +func TestCheckImagePull(t *testing.T) { + rel := &cluster.ParentRelease{ReleaseName: "tb"} + + t.Run("no pull secret -> ok", func(t *testing.T) { + cs := fake.NewClientset(jmDepWithPullSecret("tb", "")) + if r := checkImagePull(bg(), cs, ns, rel); r.Status != StatusOK { + t.Fatalf("=> %v (%q), want ok", r.Status, r.Detail) + } + }) + t.Run("valid dockerconfigjson -> ok", func(t *testing.T) { + cs := fake.NewClientset( + jmDepWithPullSecret("tb", "reg"), + dockerSecret("reg", []byte(`{"auths":{}}`)), + ) + if r := checkImagePull(bg(), cs, ns, rel); r.Status != StatusOK { + t.Fatalf("=> %v (%q), want ok", r.Status, r.Detail) + } + }) + t.Run("missing secret -> fail", func(t *testing.T) { + cs := fake.NewClientset(jmDepWithPullSecret("tb", "reg")) // secret absent + if r := checkImagePull(bg(), cs, ns, rel); r.Status != StatusFail { + t.Fatalf("=> %v (%q), want fail", r.Status, r.Detail) + } + }) + t.Run("malformed dockerconfigjson -> fail", func(t *testing.T) { + cs := fake.NewClientset( + jmDepWithPullSecret("tb", "reg"), + dockerSecret("reg", []byte("not json")), + ) + if r := checkImagePull(bg(), cs, ns, rel); r.Status != StatusFail { + t.Fatalf("=> %v (%q), want fail", r.Status, r.Detail) + } + }) +} From c04ab624ee6860bdf8cd272e9bfcca9058f35aac Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Thu, 18 Jun 2026 21:26:58 +0500 Subject: [PATCH 2/2] fix(#90): node-fit must require cpu+mem+GPU on ONE node (Bugbot) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit checkNodeFit set cpuMemFits and gpuFits independently, so they could come from different nodes — reporting OK even when no single node had cpu+memory+GPU together (a GPU job would then stay Pending). It now evaluates each node as a whole: cpuMemFits (any node) drives the hard fail; fullFits (one node with cpu+mem AND the GPU) drives the ok/warn split. Adds regression tests for the cross-node and single-node-fits cases. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/doctor/doctor.go | 24 ++++++++++++++++-------- internal/doctor/doctor_test.go | 20 ++++++++++++++++++++ 2 files changed, 36 insertions(+), 8 deletions(-) diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index f282a1b1..fdcc4ecd 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -357,21 +357,29 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s req += fmt.Sprintf(", %s=%s", gpuName, gpuReq.String()) } - var cpuMemFits, gpuFits bool + // A pod gets ALL its requested resources from ONE node, so evaluate each + // node as a whole — never OR cpu/mem and GPU across different nodes, which + // would pass even when no single node can run the job (Bugbot on PR #91). + var cpuMemFits, fullFits bool for i := range nodes.Items { n := nodes.Items[i] if !nodeReady(n) { continue } alloc := n.Status.Allocatable - if alloc.Cpu().Cmp(cpuReq) >= 0 && alloc.Memory().Cmp(memReq) >= 0 { - cpuMemFits = true - } + nodeCPUMem := alloc.Cpu().Cmp(cpuReq) >= 0 && alloc.Memory().Cmp(memReq) >= 0 + nodeGPU := !gpuRequested if gpuRequested { if q, present := alloc[gpuName]; present && q.Cmp(gpuReq) >= 0 { - gpuFits = true + nodeGPU = true } } + if nodeCPUMem { + cpuMemFits = true + } + if nodeCPUMem && nodeGPU { + fullFits = true + } } switch { @@ -382,12 +390,12 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s Detail: fmt.Sprintf("no Ready node can fit a training job (needs %s)", req), Remedy: "Add/resize a node to meet the job's requests, or lower RESOURCE_REQUESTS on jobs-manager.", } - case gpuRequested && !gpuFits: + case gpuRequested && !fullFits: return Result{ Name: name, Status: StatusWarn, - Detail: fmt.Sprintf("no node exposes %s — GPU jobs rely on the CPU fallback (needs %s)", gpuName, req), - Remedy: "If GPU training is expected, ensure a GPU node + its device plugin are present.", + Detail: fmt.Sprintf("no single Ready node satisfies cpu+memory AND %s — GPU jobs rely on the CPU fallback (needs %s)", gpuName, req), + Remedy: "If GPU training is expected, ensure one node has both the compute and the GPU capacity, with its device plugin.", } default: return Result{Name: name, Status: StatusOK, Detail: fmt.Sprintf("a Ready node can schedule a training job (%s)", req)} diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index 7c2b5c2a..766fe933 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -408,6 +408,26 @@ func TestCheckNodeFit(t *testing.T) { t.Fatalf("=> %v (%q), want warn", r.Status, r.Detail) } }) + t.Run("cpu+mem and gpu on different nodes -> warn, not ok", func(t *testing.T) { + // The Bugbot #91 case: one node fits cpu/mem, a different node has the + // GPU but is too small. No single node runs a GPU job → must NOT be ok. + cs := fake.NewClientset( + node("big", "4", "16Gi"), // cpu/mem, no gpu + node("gpu", "1", "1Gi", "nvidia.com/gpu", "2"), // gpu, too small + ) + if r := checkNodeFit(bg(), cs, full); r.Status != StatusWarn { + t.Fatalf("=> %v (%q), want warn (no single node fits all)", r.Status, r.Detail) + } + }) + t.Run("single node fits cpu+mem+gpu -> ok", func(t *testing.T) { + cs := fake.NewClientset( + node("big", "4", "16Gi"), // distractor: cpu/mem only + node("full", "4", "16Gi", "nvidia.com/gpu", "1"), // satisfies everything + ) + if r := checkNodeFit(bg(), cs, full); r.Status != StatusOK { + t.Fatalf("=> %v (%q), want ok", r.Status, r.Detail) + } + }) t.Run("not-ready node doesn't count -> fail", func(t *testing.T) { n := node("n1", "8", "32Gi") n.Status.Conditions = []corev1.NodeCondition{{Type: corev1.NodeReady, Status: corev1.ConditionFalse}}