feat(#90): cluster doctor — node-fit + image-pull checks (WS3 follow-up)

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,185 @@ 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/<release>-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())
+	}
+
+	// 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
+ 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 {
+ nodeGPU = true
+ }
+ }
+ if nodeCPUMem {
+ cpuMemFits = true
+ }
+ if nodeCPUMem && nodeGPU {
+ fullFits = 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 && !fullFits:
+ return Result{
+ Name: name,
+ Status: StatusWarn,
+ 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)}
+	}
+}
+
+// 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.

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,19 +319,21 @@ 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{
  Namespace: ns,
  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,155 @@ 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("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}}
+ 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)
+ }
+	})
+}