From 52abb16f77f1ace4e44c246fe126919e12cdd5d4 Mon Sep 17 00:00:00 2001 From: William Rizzo Date: Sun, 9 Aug 2026 12:57:59 +0000 Subject: [PATCH] fix: say so when an upgraded cluster is serving an older CRD MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit v0.1.1 adds two CRD fields, and `helm upgrade` never updates a chart's crds/. The apiserver then silently drops what the old schema does not know: a pool asking for `updatePolicy.type: RollingUpdate` would have been accepted and discarded — the feature reporting Manual and never replacing a cell — and status.cells[].templateHash writes would have vanished, so drift read as up-to-date forever (an empty hash counts as current, deliberately). Nothing anywhere said a word. The manager now embeds the CRD it was built against, compares property paths with the served schema at startup, and names the exact fields being dropped plus the kubectl command to fix it. `make manifests` syncs the embedded copy and CI diffs it, so the baseline cannot drift. Unknown is never a verdict: an unreadable CRD reports "not compared" with the missing grant named, at default verbosity — a check that quietly does not run is the failure it exists to catch. Non-fatal throughout; refusing to start would be a worse answer than naming the gap. The `installCRDs` value is deleted. It was referenced by nothing: the CRD ships in crds/, which Helm installs unconditionally, so setting it false silently did nothing. Moving the CRD into templates/ to make the flag real would break upgrades of existing releases (Helm refuses to adopt a resource it does not own), so the honest fix is to remove the knob and document the manual step. docs/upgrading.md is new, NOTES.txt prints the command on upgrade only, and the release build stamps the version so the printed URL points at that release rather than main. Validated on the dev cluster by replacing the served CRD with v0.1.0's: missing: [v1alpha1.spec.updatePolicy, v1alpha1.status.cells[].templateHash] fix: kubectl apply -f https://.../v0.1.1/config/crd/bases/cells.kubeswift.io_gpucellpools.yaml then restoring it: "CRD schema matches this build". Two output defects were found and fixed by that run — the fix command reconstructed the filename from the CRD's name and 404'd, and a nil error logged a stacktrace that read as a crash. Signed-off-by: William Rizzo --- .github/workflows/ci.yaml | 6 + .github/workflows/release.yaml | 2 + Dockerfile | 8 +- Makefile | 5 + charts/gpucellpool/rules.yaml | 6 + charts/gpucellpool/templates/NOTES.txt | 12 + charts/gpucellpool/values.yaml | 9 +- cmd/manager/main.go | 49 + config/rbac/role.yaml | 6 + docs/README.md | 1 + docs/upgrading.md | 86 ++ go.mod | 2 +- internal/controller/controller.go | 4 + .../crd/cells.kubeswift.io_gpucellpools.yaml | 973 ++++++++++++++++++ internal/crdcheck/crdcheck.go | 170 +++ internal/crdcheck/crdcheck_test.go | 182 ++++ 16 files changed, 1515 insertions(+), 6 deletions(-) create mode 100644 docs/upgrading.md create mode 100644 internal/crdcheck/crd/cells.kubeswift.io_gpucellpools.yaml create mode 100644 internal/crdcheck/crdcheck.go create mode 100644 internal/crdcheck/crdcheck_test.go diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 8d35214..f7fbec7 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -100,6 +100,12 @@ jobs: - name: CRD copy in the chart matches config/ run: diff -u config/crd/bases/cells.kubeswift.io_gpucellpools.yaml charts/gpucellpool/crds/cells.kubeswift.io_gpucellpools.yaml + # The manager embeds the same CRD to detect a cluster serving an older schema. + # A stale embed would make it compare against the wrong baseline and report + # either nothing or a phantom drift. + - name: CRD copy embedded in the manager matches config/ + run: diff -u config/crd/bases/cells.kubeswift.io_gpucellpools.yaml internal/crdcheck/crd/cells.kubeswift.io_gpucellpools.yaml + image: name: Image builds runs-on: ubuntu-latest diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index 7727cbd..292756a 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -50,6 +50,8 @@ jobs: with: context: . push: true + build-args: | + VERSION=${{ github.ref_name }} platforms: linux/amd64,linux/arm64 tags: | ${{ env.IMAGE }}:${{ github.ref_name }} diff --git a/Dockerfile b/Dockerfile index 62b9635..2a75be3 100644 --- a/Dockerfile +++ b/Dockerfile @@ -14,8 +14,14 @@ COPY api/ api/ COPY internal/ internal/ # CGO off and a static binary, so the image below needs no libc at all. +# VERSION is stamped into the binary so the CRD-drift report can print a fix +# command pointing at THIS release's manifest rather than at main. Unset in a local +# build, which is why the default in crdcheck is "main". +ARG VERSION=main RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} \ - go build -a -trimpath -ldflags="-s -w" -o manager ./cmd/manager + go build -a -trimpath \ + -ldflags="-s -w -X github.com/kubeswift-io/gpucellpool/internal/crdcheck.Version=${VERSION}" \ + -o manager ./cmd/manager # Distroless static, non-root. This operator reconciles two API servers and # nothing else: no shell, no package manager, no writable filesystem, and — unlike diff --git a/Makefile b/Makefile index acfe849..cc0cecc 100644 --- a/Makefile +++ b/Makefile @@ -40,6 +40,11 @@ manifests: controller-gen ## Generate CRDs and RBAC into config/, and sync the c # kubebuilder markers and the chart's hand-written copy drifted independently — # and the chart silently lacked the Cluster API rules it needed. sed -n '/^rules:/,$$p' config/rbac/role.yaml | tail -n +2 > charts/gpucellpool/rules.yaml + # And a third copy, embedded INTO the binary: it is what lets the manager notice + # at startup that the cluster is serving an older schema than it was built + # against — the failure mode is otherwise silent, because the apiserver just + # drops the fields it does not know. + cp config/crd/bases/*.yaml internal/crdcheck/crd/ $(MAKE) dashboards-sync .PHONY: dashboards-sync diff --git a/charts/gpucellpool/rules.yaml b/charts/gpucellpool/rules.yaml index ac0f0e8..3b9b15e 100644 --- a/charts/gpucellpool/rules.yaml +++ b/charts/gpucellpool/rules.yaml @@ -16,6 +16,12 @@ verbs: - create - patch +- apiGroups: + - apiextensions.k8s.io + resources: + - customresourcedefinitions + verbs: + - get - apiGroups: - bootstrap.cluster.x-k8s.io resources: diff --git a/charts/gpucellpool/templates/NOTES.txt b/charts/gpucellpool/templates/NOTES.txt index a609fa9..7df6694 100644 --- a/charts/gpucellpool/templates/NOTES.txt +++ b/charts/gpucellpool/templates/NOTES.txt @@ -25,6 +25,18 @@ Watch both layers: A cell only becomes Ready when the VM runs, its Node registers Ready, AND HAMi advertises the GPU. If it sits in AwaitingGPUCapacity, the message names why. +{{- if .Release.IsUpgrade }} + +APPLY THE CRD. `helm upgrade` never updates a chart's crds/, and the apiserver +then SILENTLY DROPS any field the older schema does not know -- a spec you write +is accepted and ignored. Run: + + kubectl apply -f https://raw.githubusercontent.com/kubeswift-io/gpucellpool/{{ printf "v%s" .Chart.AppVersion }}/config/crd/bases/cells.kubeswift.io_gpucellpools.yaml + +The manager checks this at startup and logs exactly which fields are missing, so +`kubectl -n {{ .Release.Namespace }} logs deploy/{{ include "gpucellpool.fullname" . }} | head` will tell you +whether you needed it. +{{- end }} {{- if not .Values.webhook.enabled }} WARNING: the validating webhook is disabled. The guestTemplate denylist is a diff --git a/charts/gpucellpool/values.yaml b/charts/gpucellpool/values.yaml index 5c14a67..66386f5 100644 --- a/charts/gpucellpool/values.yaml +++ b/charts/gpucellpool/values.yaml @@ -69,10 +69,11 @@ monitoring: enabled: true additionalLabels: {} -# Install the CRDs. Note that `helm upgrade` never updates files in crds/, so a -# CRD change needs `kubectl apply -f charts/gpucellpool/crds/` — the same caveat -# KubeSwift has. -installCRDs: true +# There is deliberately no installCRDs toggle. The CRD ships in crds/, which Helm +# installs unconditionally and never updates, so a flag claiming to control it +# would be a no-op — as one here silently was. Upgrades therefore need the CRD +# applied by hand; NOTES.txt prints the command, docs/upgrading.md explains why, +# and the manager logs the specific fields being dropped if it is skipped. podAnnotations: {} podLabels: {} diff --git a/cmd/manager/main.go b/cmd/manager/main.go index cd85eff..7afc47a 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -7,14 +7,18 @@ package main import ( + "context" "crypto/tls" "flag" + "fmt" "os" "time" + apiextensionsclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/healthz" "sigs.k8s.io/controller-runtime/pkg/log/zap" @@ -22,6 +26,7 @@ import ( cellsv1alpha1 "github.com/kubeswift-io/gpucellpool/api/v1alpha1" "github.com/kubeswift-io/gpucellpool/internal/controller" + "github.com/kubeswift-io/gpucellpool/internal/crdcheck" webhookv1alpha1 "github.com/kubeswift-io/gpucellpool/internal/webhook/v1alpha1" "github.com/kubeswift-io/gpucellpool/internal/workload" ) @@ -99,9 +104,53 @@ func main() { os.Exit(1) } + // Say so if the cluster is serving an older CRD than this binary was built + // against. `helm upgrade` never updates a chart's crds/, and the apiserver then + // silently drops every field the old schema lacks — the operator writes them, + // they vanish, and nothing reports a problem. Not fatal: the rest of the + // operator still works, and refusing to start would be a worse answer than + // naming the gap. + if err := reportCRDDrift(context.Background(), mgr.GetConfig()); err != nil { + setupLog.Info("could not compare the served CRD schema", "error", err.Error()) + } + setupLog.Info("starting manager") if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil { setupLog.Error(err, "problem running manager") os.Exit(1) } } + +func reportCRDDrift(ctx context.Context, cfg *rest.Config) error { + cs, err := apiextensionsclient.NewForConfig(cfg) + if err != nil { + return err + } + results, err := crdcheck.Verify(ctx, cs.ApiextensionsV1().CustomResourceDefinitions()) + if err != nil { + return err + } + for _, r := range results { + switch { + case !r.Checked: + // At default verbosity, not V(1): a check that quietly does not run is + // the same silent failure it exists to catch. On an upgraded release the + // commonest cause is the operator's ClusterRole predating the + // apiextensions grant, which reads as Forbidden here. + setupLog.Info("CRD schema NOT compared — a stale schema would go unnoticed", + "crd", r.Name, "reason", r.Err.Error(), "needs", "apiextensions.k8s.io/customresourcedefinitions: get") + case len(r.Missing) > 0: + // A real error value, not nil: a nil error logs a stacktrace pointing at + // this function, which reads as a crash rather than the configuration + // problem it is. + setupLog.Error(fmt.Errorf("%d field(s) dropped by a stale CRD", len(r.Missing)), + "the cluster is serving an OLDER CRD than this operator was built against; "+ + "the apiserver will SILENTLY DROP the fields below, so features that depend on them "+ + "will appear to be accepted and then do nothing. helm upgrade does not update CRDs — apply it yourself", + "crd", r.Name, "missingFields", r.Missing, "fix", crdcheck.FixCommand(r.File)) + default: + setupLog.Info("CRD schema matches this build", "crd", r.Name) + } + } + return nil +} diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index b8a6835..c3312c0 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -22,6 +22,12 @@ rules: verbs: - create - patch +- apiGroups: + - apiextensions.k8s.io + resources: + - customresourcedefinitions + verbs: + - get - apiGroups: - bootstrap.cluster.x-k8s.io resources: diff --git a/docs/README.md b/docs/README.md index 9d102bb..b09832c 100644 --- a/docs/README.md +++ b/docs/README.md @@ -15,6 +15,7 @@ Start here if you are installing or operating a pool. | [security](security.md) | why creating a pool is node-root-equivalent authority, the webhook, the two RBAC scopes | | [cell-image](cell-image.md) | building and publishing a cell image with `hack/build-cell-image.sh` | | [clusterapi-cells](clusterapi-cells.md) | `provisioner: ClusterAPI` — cells as Cluster API Machines | +| [upgrading](upgrading.md) | the CRD step `helm upgrade` will not do for you, and what it silently breaks if skipped | | [limitations](limitations.md) | what is not implemented, not validated on hardware, or deliberately manual | | [observability](observability.md) | the twelve `gpucell_*` metrics, the dashboard, and the alert pack | | [runbook](runbook.md) | what to check when a pool is not doing what you expect | diff --git a/docs/upgrading.md b/docs/upgrading.md new file mode 100644 index 0000000..ae89669 --- /dev/null +++ b/docs/upgrading.md @@ -0,0 +1,86 @@ +# Upgrading + +## The one thing that is not automatic: the CRD + +`helm upgrade` does **not** update a chart's `crds/`. Helm installs those files +once and never touches them again. Nothing warns you, and the upgrade succeeds. + +What happens next is the problem. The apiserver validates against the schema it is +serving — the old one — and **silently drops** every field that schema does not +know. Not rejects: drops. You apply a pool spec, the apiserver stores a version of +it with your new fields removed, and reports success. + +So after every `helm upgrade`, apply the CRD: + +```bash +kubectl apply -f https://raw.githubusercontent.com/kubeswift-io/gpucellpool/vX.Y.Z/config/crd/bases/cells.kubeswift.io_gpucellpools.yaml +``` + +Order does not matter much — before or after the upgrade is fine — but it must +happen. `helm upgrade --install` does not do it, `--force` does not do it, and +reinstalling the release does not do it either (the CRD already exists). + +### You do not have to remember + +The manager compares the schema the cluster is serving with the one it was built +against, at startup, and names the exact fields being dropped: + +``` +ERROR the cluster is serving an OLDER CRD than this operator was built against; + the apiserver will SILENTLY DROP the fields below, so features that depend + on them will appear to be accepted and then do nothing. helm upgrade does + not update CRDs — apply it yourself + {"crd": "gpucellpools.cells.kubeswift.io", + "missingFields": ["v1alpha1.spec.updatePolicy"], + "fix": "kubectl apply -f https://.../cells.kubeswift.io_gpucellpools.yaml"} +``` + +Check it after any upgrade: + +```bash +kubectl -n gpucellpool-system logs deploy/gpucellpool | grep -i 'CRD schema' +``` + +`CRD schema matches this build` means there is nothing to do. If the operator's +credential cannot read CRDs the line says `CRD schema not compared` — the check is +read-only and optional (`apiextensions.k8s.io/customresourcedefinitions: get`), and +its absence degrades the check rather than the operator. + +### What v0.1.0 → v0.1.1 drops if you skip it + +Both fields `updatePolicy` added: + +| Field | Consequence of the old schema | +|---|---| +| `spec.updatePolicy` | `type: RollingUpdate` is accepted and discarded. The pool reports `Manual` and never replaces a stale cell — the feature looks enabled and does nothing | +| `status.cells[].templateHash` | the operator's writes vanish, so `Updated` compares against an empty hash. An empty hash counts as current by design, so **drift reports as up-to-date forever** | + +## The rest of an upgrade + +```bash +helm upgrade gpucellpool oci://ghcr.io/kubeswift-io/charts/gpucellpool \ + --version X.Y.Z -n gpucellpool-system \ + -f <(helm get values gpucellpool -n gpucellpool-system -o yaml) +``` + +Two notes on that command: + +- Pass the old values through a file rather than using `--reuse-values`, which + skips defaulting for value blocks a new chart version added and can leave nil + where the templates expect a map. +- `helm get values -o yaml` output is the values document itself, with no header + line to strip. + +Running pools are not disturbed by an operator upgrade: cells are separate objects +in the infrastructure cluster, and the manager rebuilds its view from their live +state on the first reconcile. There is no state in the manager to migrate. + +The webhook's self-signed certificate survives upgrades — the chart reuses the +existing Secret rather than minting a new one, so the `caBundle` and the serving +cert cannot drift apart. + +## Downgrading + +Downgrade the release, and leave the CRD alone. A newer CRD serving an older +operator is safe: the extra fields are simply unused. Removing fields from a CRD +that stored objects still carry is not. diff --git a/go.mod b/go.mod index b7e2ad0..abc05dd 100644 --- a/go.mod +++ b/go.mod @@ -5,6 +5,7 @@ go 1.25.0 require ( github.com/prometheus/client_golang v1.23.2 k8s.io/api v0.35.4 + k8s.io/apiextensions-apiserver v0.35.0 k8s.io/apimachinery v0.35.4 k8s.io/client-go v0.35.4 sigs.k8s.io/controller-runtime v0.23.3 @@ -56,7 +57,6 @@ require ( gopkg.in/evanphx/json-patch.v4 v4.13.0 // indirect gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect - k8s.io/apiextensions-apiserver v0.35.0 // indirect k8s.io/klog/v2 v2.130.1 // indirect k8s.io/kube-openapi v0.0.0-20250910181357-589584f1c912 // indirect k8s.io/utils v0.0.0-20251002143259-bc988d571ff4 // indirect diff --git a/internal/controller/controller.go b/internal/controller/controller.go index 688e63e..e114120 100644 --- a/internal/controller/controller.go +++ b/internal/controller/controller.go @@ -76,6 +76,10 @@ type GPUCellPoolReconciler struct { // describe gpucellpool`. The core grant stays for clients that still read there. // +kubebuilder:rbac:groups="",resources=events,verbs=create;patch // +kubebuilder:rbac:groups=events.k8s.io,resources=events,verbs=create;patch +// Read-only, and only its own CRD: the manager compares the served schema with the +// one it was built against at startup, because helm upgrade never updates a chart's +// crds/ and the apiserver then drops the new fields without a word (internal/crdcheck). +// +kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=get // Reconcile implements the loop in docs/design/gpucellpool-reconciliation.md §4. func (r *GPUCellPoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { diff --git a/internal/crdcheck/crd/cells.kubeswift.io_gpucellpools.yaml b/internal/crdcheck/crd/cells.kubeswift.io_gpucellpools.yaml new file mode 100644 index 0000000..b84a12a --- /dev/null +++ b/internal/crdcheck/crd/cells.kubeswift.io_gpucellpools.yaml @@ -0,0 +1,973 @@ +--- +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + annotations: + controller-gen.kubebuilder.io/version: v0.17.2 + name: gpucellpools.cells.kubeswift.io +spec: + group: cells.kubeswift.io + names: + kind: GPUCellPool + listKind: GPUCellPoolList + plural: gpucellpools + shortNames: + - cellpool + singular: gpucellpool + scope: Namespaced + versions: + - additionalPrinterColumns: + - jsonPath: .spec.replicas + name: Desired + type: integer + - jsonPath: .status.readyCells + name: Ready + type: integer + - jsonPath: .status.physicalCapacity.gpus + name: GPUs + type: integer + - jsonPath: .status.workloadCapacity.gpuMemory.available + name: GPUMemFree + type: string + - jsonPath: .status.conditions[?(@.type=="Ready")].status + name: Available + type: string + - jsonPath: .metadata.creationTimestamp + name: Age + type: date + name: v1alpha1 + schema: + openAPIV3Schema: + description: GPUCellPool is a pool of VM-isolated, fractionally-shared GPU + worker nodes. + properties: + apiVersion: + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources + type: string + kind: + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds + type: string + metadata: + type: object + spec: + description: |- + GPUCellPoolSpec is the desired state of a pool of GPU cells. + + A cell is one KubeSwift SwiftGuest VM holding one whole passthrough GPU, joined + to a workload Kubernetes cluster as a node whose GPU HAMi shares. The spec is + split deliberately: spec.cell is the SHAPE of one cell, spec.replicas is POOL + POLICY. They never mix (design principle 9.3). + properties: + autoscaling: + description: |- + Autoscaling, when enabled, lets unsatisfiable GPU demand in the WORKLOAD + cluster create cells, and lets idle cells be removed again (see + AutoscalingSpec). + properties: + enabled: + description: |- + Enabled turns demand-driven scale-up on. When false (the default) the pool + holds exactly spec.replicas. + type: boolean + maxReplicas: + description: |- + MaxReplicas is the ceiling. Required when enabled: an unbounded pool that + misreads demand can consume every GPU in the cluster. + format: int32 + minimum: 1 + type: integer + minReplicas: + description: |- + MinReplicas is the floor the pool never drops below. Defaults to + spec.replicas at the time autoscaling is enabled. + format: int32 + minimum: 0 + type: integer + scaleDown: + default: Manual + description: |- + ScaleDown selects who shrinks the pool. + Manual (default): change spec.replicas or minReplicas; the drain path runs. + Auto: the pool removes an IDLE cell when demand is gone. + + Auto never removes a cell that holds workloads — idleness is read from the + capacity provider, and the drain gate re-checks before deletion. It is + deliberately slower and more conservative than scale-up: a GPU cell costs + minutes to recreate, so thrashing one is worse than holding it a while. + enum: + - Manual + - Auto + type: string + scaleDownStabilizationWindow: + default: 30m + description: |- + ScaleDownStabilizationWindow is how long demand must stay absent, and how + long since the last scale action, before a cell is removed. Longer than the + scale-up window on purpose: removing a cell that is about to be wanted again + costs a full boot, and the whole point of a pool is to have capacity ready. + type: string + stabilizationWindow: + default: 10m + description: |- + StabilizationWindow is how long to wait after a scale-up before scaling up + again. A cell takes minutes to become Ready (measured: 4m45s from a baked + image, about three minutes of which is cloning the root disk), and demand does + not clear until it does — so without this window one burst of pending pods + creates a cell per reconcile. + type: string + type: object + bootstrap: + description: |- + Bootstrap describes how a cell becomes a worker of the workload cluster. + + Optional because it is genuinely unused when the workload cluster's own Cluster + API bootstrap provider supplies the join data (cell.clusterAPI. + bootstrapConfigTemplateRef). Requiring it there forced an empty `bootstrap: {}` + into the manifest to satisfy the schema — a field you must write and nothing + reads. The webhook still requires joinSecretRef whenever it IS the join path. + properties: + hostname: + default: CellName + description: |- + Hostname controls the guest hostname. CellName (default) makes the guest + hostname, and therefore the workload Node name, equal to the cell name — + which is how cells are correlated across the two clusters. + enum: + - CellName + - None + type: string + joinSecretKey: + default: user-data + description: JoinSecretKey is the key inside JoinSecretRef holding + the user-data. + type: string + joinSecretRef: + description: |- + JoinSecretRef holds the cloud-init user-data that joins the workload + cluster, under key "user-data" unless JoinSecretKey says otherwise. + Required for the Opaque provider. + + The credential is only ever referenced: the controller renders a per-cell + Secret and points the SwiftSeedProfile at it, so no token is written into + any custom resource. + properties: + name: + default: "" + description: |- + Name of the referent. + This field is effectively required, but due to backwards compatibility is + allowed to be empty. Instances of this type with an empty value here are + almost certainly wrong. + More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + type: string + type: object + x-kubernetes-map-type: atomic + provider: + default: Opaque + description: Provider selects the join-credential mechanism. Opaque + is the only value. + enum: + - Opaque + type: string + readyTimeout: + default: 15m + description: |- + ReadyTimeout bounds Booting+Joining. A cell that has not produced a Ready + workload Node within it is marked Failed with a reason, never left silent. + type: string + type: object + capacity: + description: Capacity selects and configures the inner capacity provider + (HAMi). + properties: + hami: + description: HAMi configures the HAMi provider. + properties: + deviceClassName: + description: |- + DeviceClassName is the HAMi DeviceClass whose ResourceSlices carry the + cells' capacity. Required in DRA mode. + type: string + expectedDevicesPerCell: + description: |- + ExpectedDevicesPerCell is how many GPU devices HAMi must advertise on a + cell's Node before that cell counts as Ready. Defaults to cell.gpu.count. + format: int32 + minimum: 1 + type: integer + mode: + default: DevicePlugin + description: Mode selects which HAMi accounting model to read. + enum: + - DevicePlugin + - DRA + type: string + type: object + provider: + default: HAMi + description: Provider is the inner GPU capacity provider. + enum: + - HAMi + type: string + readyTimeout: + default: 5m + description: |- + ReadyTimeout bounds how long a joined Node may go without advertising the + expected GPU before the cell is marked Failed. + type: string + type: object + cell: + description: 'Cell describes one cell: the VM shape and its GPU.' + properties: + clusterAPI: + description: |- + ClusterAPI configures the ClusterAPI provisioner. Required when + provisioner is ClusterAPI, and rejected otherwise. + properties: + bootstrapConfigTemplateRef: + description: |- + BootstrapConfigTemplateRef names a CAPI bootstrap config TEMPLATE (e.g. a + KubeadmConfigTemplate) that the operator instantiates once per cell, the way + a MachineSet does. This is the reason to use CAPI at all: the join + credential, CA hashes and cluster config come from the cluster's own + bootstrap provider rather than from a secret somebody pasted. + + Leave it unset to use spec.bootstrap instead — the per-cell rendered Secret + is then handed to the Machine as bootstrap.dataSecretName. That works, but + you own keeping the join data valid. + properties: + apiGroup: + description: APIGroup is e.g. "bootstrap.cluster.x-k8s.io". + minLength: 1 + type: string + kind: + description: Kind is e.g. "KubeadmConfigTemplate". + minLength: 1 + type: string + name: + description: Name is the template object's name in the + pool's namespace. + minLength: 1 + type: string + required: + - apiGroup + - kind + - name + type: object + clusterName: + description: ClusterName is the CAPI Cluster, in the pool's + namespace, that cells join. + maxLength: 63 + minLength: 1 + type: string + version: + description: |- + Version is the Kubernetes version stamped on each Machine + (Machine.spec.version), which bootstrap providers use to pick the kubelet. + type: string + required: + - clusterName + type: object + gpu: + description: GPU is the cell's physical GPU request. + properties: + backend: + default: DRA + description: Backend selects the KubeSwift GPU allocation + backend. + enum: + - DRA + - Native + type: string + count: + default: 1 + description: |- + Count is the number of whole GPUs per cell. Only 1 is supported in + v1alpha1; multi-GPU cells need NVLink/topology work. + enum: + - 1 + format: int32 + type: integer + dra: + description: DRA configures the DRA backend. Required when + backend is DRA. + properties: + hugepages: + description: Hugepages sizes the GPU memory hugepage backing + ("1Gi", "2Mi", or empty). + enum: + - "" + - 1Gi + - 2Mi + type: string + requestName: + default: gpu + description: |- + RequestName is the device-request name inside the claim to read the + allocation result back from. + type: string + resourceClaimName: + description: |- + ResourceClaimName references one pre-created, shared ResourceClaim. A + VFIO device can back only ONE running VM, so this is only correct for a + single-cell pool. + type: string + resourceClaimTemplateName: + description: ResourceClaimTemplateName mints a per-cell + ResourceClaim. Recommended. + type: string + tier: + default: pcie + description: |- + Tier selects hypervisor and firmware in KubeSwift. Only pcie (Cloud + Hypervisor) is supported for cells: hgx-shared needs QEMU plus a host + Fabric Manager, and hgx-full is rejected by KubeSwift at allocation. + enum: + - pcie + type: string + type: object + native: + description: |- + Native configures the native SwiftGPU backend. Required when backend is + Native. + properties: + gpuProfileRef: + description: GPUProfileRef references a SwiftGPUProfile + in the pool's namespace. + properties: + name: + default: "" + description: |- + Name of the referent. + This field is effectively required, but due to backwards compatibility is + allowed to be empty. Instances of this type with an empty value here are + almost certainly wrong. + More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + type: string + type: object + x-kubernetes-map-type: atomic + required: + - gpuProfileRef + type: object + type: object + guestTemplate: + description: |- + GuestTemplate is a verbatim KubeSwift SwiftGuestSpec, passed through + unmodified apart from the fields this operator owns. It is opaque on + purpose: mirroring KubeSwift's API surface here would guarantee drift, and + every KubeSwift VM feature (storage, interfaces, data disks, topology + spread) is available for free. + + Operator-owned fields (rejected if set here; written by the controller): + seedProfileRef, gpuProfileRef, gpuResourceClaim, nodeName, migration, + runPolicy. + + Denied fields (incompatible with a GPU cell): kernelRef, + cloneFromSnapshot, osType: windows, vhostUserDevices, filesystems. + + guestClassRef and imageRef are required — a GPU cell is a disk boot. + type: object + x-kubernetes-preserve-unknown-fields: true + nodeIPFrom: + description: |- + NodeIPFrom names the guestTemplate interface whose address the kubelet + registers as --node-ip. Empty means the primary interface. + + A cell almost always needs a ROUTABLE interface here, not KubeSwift's + node-local nat primary: the workload apiserver must be able to dial the + kubelet for logs, exec, port-forward and metrics. + type: string + provisioner: + default: SwiftGuest + description: |- + Provisioner creates the outer objects for a cell. + SwiftGuest: create a KubeSwift SwiftGuest directly. + ClusterAPI: create one Cluster API Machine + KubeSwiftMachine per cell, so + the cell is a member of a CAPI-managed cluster. An additional + mode, never a replacement: CAPI can only add Machines to a + cluster it already manages. + enum: + - SwiftGuest + - ClusterAPI + type: string + spreadPolicy: + default: Spread + description: |- + SpreadPolicy is shorthand for hostname topology spread of the cell VMs + across outer nodes. Spread is the default: two cells on one host share a + failure domain and, usually, a NUMA/PCIe path. + enum: + - Pack + - Spread + type: string + required: + - gpu + - guestTemplate + type: object + deletion: + description: Deletion controls teardown behaviour for the pool. + properties: + drainTimeout: + default: 10m + description: |- + DrainTimeout bounds draining during EXPLICIT pool deletion, after which + teardown proceeds anyway (leaving VMs and GPUs pinned forever after a + delete request is worse). Implicit scale-down never forces. + type: string + policy: + default: Drain + description: Policy controls whether cells are drained before + deletion. + enum: + - Drain + - Force + type: string + type: object + replicas: + default: 1 + description: |- + Replicas is the desired number of cells. Scaled via the scale subresource, + so `kubectl scale` and an HPA both work. When spec.autoscaling is enabled it + overrides this: the pool then aims for status.desiredReplicas, and replicas + acts as the default floor. + format: int32 + minimum: 0 + type: integer + updatePolicy: + description: UpdatePolicy decides what happens to existing cells when + spec.cell changes. + properties: + type: + default: Manual + description: Type is Manual (default) or RollingUpdate. + enum: + - Manual + - RollingUpdate + type: string + type: object + workloadCluster: + description: WorkloadCluster is the cluster the cells join and where + HAMi runs. + properties: + key: + default: value + description: Key is the Secret key holding the kubeconfig. + type: string + kubeconfigSecretRef: + description: |- + KubeconfigSecretRef references a Secret in the pool's namespace holding a + kubeconfig for the workload cluster. Scope the credential: see + docs/design/gpucellpool-reconciliation.md for the minimum RBAC. + properties: + name: + default: "" + description: |- + Name of the referent. + This field is effectively required, but due to backwards compatibility is + allowed to be empty. Instances of this type with an empty value here are + almost certainly wrong. + More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + type: string + type: object + x-kubernetes-map-type: atomic + node: + description: |- + Node describes labels, annotations and taints applied to each cell's + workload Node. + properties: + annotations: + additionalProperties: + type: string + description: Annotations are applied to the cell's Node. + type: object + labels: + additionalProperties: + type: string + description: |- + Labels are applied to the cell's Node. The capacity provider's own gate + (for HAMi: gpu=on) is declared HERE, by the user: this operator applies + labels it does not interpret. + type: object + taints: + description: |- + Taints are applied to the cell's Node. Note that the capacity provider's + own DaemonSets must tolerate them or the GPU is never advertised. + items: + description: |- + The node this Taint is attached to has the "effect" on + any pod that does not tolerate the Taint. + properties: + effect: + description: |- + Required. The effect of the taint on pods + that do not tolerate the taint. + Valid effects are NoSchedule, PreferNoSchedule and NoExecute. + type: string + key: + description: Required. The taint key to be applied to + a node. + type: string + timeAdded: + description: TimeAdded represents the time at which + the taint was added. + format: date-time + type: string + value: + description: The taint value corresponding to the taint + key. + type: string + required: + - effect + - key + type: object + type: array + type: object + required: + - kubeconfigSecretRef + type: object + required: + - cell + - replicas + - workloadCluster + type: object + status: + description: GPUCellPoolStatus is the observed state of a GPUCellPool. + properties: + cellDeviceShape: + description: |- + CellDeviceShape is the remembered shape of one cell's GPU. It outlives the + cells themselves so that a pool which scaled to zero can still judge whether + a fresh cell would satisfy a pending request. + properties: + corePercent: + description: |- + CorePercent is one device's compute, as the percentage the provider + accounts in (100 = a whole device). + format: int64 + type: integer + lastObserved: + description: |- + LastObserved is when this shape was last confirmed against a live cell. A + shape older than the pool's cells is still used — it is the best knowledge + available — but the timestamp says how stale it is. + format: date-time + type: string + memoryMiB: + description: MemoryMiB is one device's GPU memory. + format: int64 + type: integer + model: + description: Model is the GPU model the provider reported, for + operator recognition. + type: string + required: + - corePercent + - memoryMiB + type: object + cells: + description: |- + Cells is per-cell state, one entry per owned cell. It is a projection of + live cluster objects, so it is fully recoverable after a restart. + items: + description: CellStatus is the observed state of one cell, spanning + both clusters. + properties: + capacityDevices: + description: |- + CapacityDevices is how many GPU devices the capacity provider advertises + on this cell's Node. + format: int32 + type: integer + devices: + description: Devices are the PCI addresses passed into the cell + VM. + items: + type: string + type: array + failureCount: + description: |- + FailureCount is how many times this index has failed; it drives the + replacement backoff and the exhaustion guard. + format: int32 + type: integer + guestUID: + description: |- + GuestUID is the outer guest's UID. It distinguishes this incarnation of + the cell from a previous one, which is how a stale workload Node left by + a replaced cell is detected instead of being adopted. + type: string + hostNode: + description: HostNode is the outer Kubernetes node whose physical + GPU this cell holds. + type: string + index: + description: Index is the cell's stable ordinal within the pool. + format: int32 + type: integer + lastTransitionTime: + description: LastTransitionTime is when Phase last changed. + format: date-time + type: string + message: + description: Message explains a non-Ready phase in operator-actionable + terms. + type: string + name: + description: |- + Name is the cell name: -. It is also the SwiftGuest name, + the guest hostname and the workload Node name. + type: string + nodeName: + description: NodeName is the cell's workload Node (equal to + Name once it registers). + type: string + nodeReady: + description: NodeReady reports the workload Node's Ready condition. + type: boolean + phase: + description: Phase is the cell's state (see CellPhase). + enum: + - Pending + - AllocatingGPU + - GuestProvisioning + - Booting + - Joining + - AwaitingGPUCapacity + - Ready + - Draining + - Deleting + - Failed + type: string + readyOnce: + description: |- + ReadyOnce records that this cell reached Ready at least once, so a later + Ready after a regression is not mistaken for a startup. + type: boolean + templateHash: + description: |- + TemplateHash is the cell template this cell was CREATED from. When it differs + from the pool's current template the cell is running an older shape, which is + what the Updated condition reports — and, under + updatePolicy.type: RollingUpdate, what gets it replaced. + type: string + required: + - index + - name + - phase + type: object + type: array + x-kubernetes-list-map-keys: + - name + x-kubernetes-list-type: map + conditions: + description: |- + Conditions separate the layers on purpose, so a single False localises the + fault: see the Condition* constants. + items: + description: Condition contains details for one aspect of the current + state of this API Resource. + properties: + lastTransitionTime: + description: |- + lastTransitionTime is the last time the condition transitioned from one status to another. + This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable. + format: date-time + type: string + message: + description: |- + message is a human readable message indicating details about the transition. + This may be an empty string. + maxLength: 32768 + type: string + observedGeneration: + description: |- + observedGeneration represents the .metadata.generation that the condition was set based upon. + For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date + with respect to the current state of the instance. + format: int64 + minimum: 0 + type: integer + reason: + description: |- + reason contains a programmatic identifier indicating the reason for the condition's last transition. + Producers of specific condition types may define expected values and meanings for this field, + and whether the values are considered a guaranteed API. + The value should be a CamelCase string. + This field may not be empty. + maxLength: 1024 + minLength: 1 + pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ + type: string + status: + description: status of the condition, one of True, False, Unknown. + enum: + - "True" + - "False" + - Unknown + type: string + type: + description: type of condition in CamelCase or in foo.example.com/CamelCase. + maxLength: 316 + pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ + type: string + required: + - lastTransitionTime + - message + - reason + - status + - type + type: object + type: array + x-kubernetes-list-map-keys: + - type + x-kubernetes-list-type: map + creatingCells: + format: int32 + type: integer + demand: + description: |- + Demand is the last GPU-demand reading from the workload cluster, present + only when autoscaling is enabled. + properties: + lastObserved: + description: LastObserved is when this reading was taken. + format: date-time + type: string + pendingRequests: + description: PendingRequests is how many GPU requests cannot currently + be placed. + format: int32 + type: integer + satisfiableByOneCell: + description: |- + SatisfiableByOneCell is how many of them a fresh cell of this pool's shape + would actually satisfy. Scaling on anything else creates cells that do not + help, so this — not PendingRequests — is what drives the decision. + format: int32 + type: integer + required: + - pendingRequests + - satisfiableByOneCell + type: object + demandFreeSince: + description: |- + DemandFreeSince is when GPU demand last went to zero. Scale-down requires + demand to have been absent for the whole window, not merely absent at this + instant — a pool that shrinks between two bursts is worse than one that waits. + format: date-time + type: string + desiredReplicas: + description: |- + DesiredReplicas is what the scaling policy asked for. It equals + spec.replicas unless autoscaling is enabled. + format: int32 + type: integer + drainingCells: + format: int32 + type: integer + failedCells: + format: int32 + type: integer + lastScaleDownTime: + description: LastScaleDownTime gates the (longer) scale-down window. + format: date-time + type: string + lastScaleUpTime: + description: LastScaleUpTime gates the stabilization window. + format: date-time + type: string + observedGeneration: + description: ObservedGeneration is the .metadata.generation this status + reflects. + format: int64 + type: integer + physicalCapacity: + description: PhysicalCapacity is outer capacity (whole GPUs). + properties: + freeGPUsInCluster: + description: |- + FreeGPUsInCluster is how many further devices the pool could claim, from + the outer inventory. Nil means unknown (never report unknown as zero). + format: int32 + type: integer + gpus: + description: GPUs is the number of physical GPUs this pool's cells + hold. + format: int32 + type: integer + model: + description: |- + Model is the GPU model the pool holds, as reported by the outer + inventory. + type: string + required: + - gpus + type: object + readyCells: + description: ReadyCells is the number of cells where both layers agree. + format: int32 + type: integer + replicas: + description: Replicas is the number of cells the pool owns (scale + subresource). + format: int32 + type: integer + workloadCapacity: + description: WorkloadCapacity is inner capacity (HAMi fractions). + properties: + byModel: + description: ByModel is per-model capacity and is always populated. + items: + description: ModelCapacity is per-GPU-model capacity within + the pool. + properties: + compute: + description: |- + ComputeCapacity is total/allocated/available GPU compute, in percent of a + device (HAMi's unit). Percentages of DIFFERENT GPU models are not + commensurable, so pool-wide aggregates are only published for a homogeneous + pool; ByModel is always authoritative. + properties: + allocated: + format: int32 + type: integer + available: + format: int32 + type: integer + total: + format: int32 + type: integer + required: + - allocated + - available + - total + type: object + devices: + format: int32 + type: integer + memory: + description: MemoryCapacity is total/allocated/available + GPU memory. + properties: + allocated: + anyOf: + - type: integer + - type: string + pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ + x-kubernetes-int-or-string: true + available: + anyOf: + - type: integer + - type: string + pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ + x-kubernetes-int-or-string: true + total: + anyOf: + - type: integer + - type: string + pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ + x-kubernetes-int-or-string: true + required: + - allocated + - available + - total + type: object + model: + type: string + required: + - devices + - model + type: object + type: array + gpuCompute: + description: |- + GPUCompute is pool-wide GPU compute in percent. Only published while + Homogeneous is true. + properties: + allocated: + format: int32 + type: integer + available: + format: int32 + type: integer + total: + format: int32 + type: integer + required: + - allocated + - available + - total + type: object + gpuDevices: + description: |- + GPUDevices is the number of GPU devices the provider advertises across + the pool's ready cells. + format: int32 + type: integer + gpuMemory: + description: |- + GPUMemory is pool-wide GPU memory. Always valid: bytes are comparable + across models. + properties: + allocated: + anyOf: + - type: integer + - type: string + pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ + x-kubernetes-int-or-string: true + available: + anyOf: + - type: integer + - type: string + pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ + x-kubernetes-int-or-string: true + total: + anyOf: + - type: integer + - type: string + pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ + x-kubernetes-int-or-string: true + required: + - allocated + - available + - total + type: object + homogeneous: + description: |- + Homogeneous is true when every advertised device is the same model. When + false, the pool-wide Compute aggregate is omitted (see ComputeCapacity). + type: boolean + lastObserved: + description: |- + LastObserved is when these numbers were last computed successfully. A + failed read retains the previous values and this timestamp rather than + reporting zero. + format: date-time + type: string + mode: + type: string + provider: + description: Provider and Mode record which accounting model produced + these numbers. + type: string + type: object + type: object + required: + - spec + type: object + served: true + storage: true + subresources: + scale: + specReplicasPath: .spec.replicas + statusReplicasPath: .status.replicas + status: {} diff --git a/internal/crdcheck/crdcheck.go b/internal/crdcheck/crdcheck.go new file mode 100644 index 0000000..29f3c26 --- /dev/null +++ b/internal/crdcheck/crdcheck.go @@ -0,0 +1,170 @@ +// Package crdcheck compares the CRD schema this binary was built against with the +// one the cluster is actually serving. +// +// It exists because of how Helm treats CRDs: files in a chart's `crds/` directory +// are installed once and NEVER updated by `helm upgrade`. Upgrading a release +// therefore leaves the old schema in place, and the apiserver then silently drops +// every field the old schema does not know — no error, no event, no rejection. The +// operator writes them, the apiserver discards them, and everything reports success. +// +// Measured for real between v0.1.0 and v0.1.1: `spec.updatePolicy` and +// `status.cells[].templateHash` were both new, so on an upgraded release +// `updatePolicy.type: RollingUpdate` would have been accepted and ignored, and +// template drift would have read as up-to-date forever (an empty hash counts as +// current, deliberately — see internal/controller/rollout.go). +// +// The comparison is on the SET of property paths, not on descriptions or defaults: +// dropped fields are exactly what this is about, and anything finer would fail on +// harmless controller-gen churn. +package crdcheck + +import ( + "context" + "embed" + "fmt" + "path" + "sort" + + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/yaml" +) + +// crdFS holds the CRDs generated from this tree's api/ types, copied in by +// `make manifests`. CI diffs the copy, so it cannot drift from config/crd/bases. +// +//go:embed crd/*.yaml +var crdFS embed.FS + +// Getter is the slice of the apiextensions client this package needs. Narrow on +// purpose: the check is read-only and must never be able to modify a CRD. +type Getter interface { + Get(ctx context.Context, name string, opts metav1.GetOptions) (*apiextensionsv1.CustomResourceDefinition, error) +} + +// Result reports one CRD's comparison. +type Result struct { + // Name is the CRD's name. + Name string + // File is the manifest's filename in config/crd/bases. Carried through rather + // than derived from Name: controller-gen names files _.yaml while + // a CRD is named ., so reconstructing one from the other produced + // a fix command pointing at a file that does not exist. + File string + // Missing are property paths this binary expects and the cluster does not serve, + // sorted. Non-empty means fields are being silently dropped. + Missing []string + // Checked is false when the comparison could not be made — the CRD is absent, or + // unreadable with the operator's RBAC. Not an error: the operator's job is not + // conditional on being able to introspect its own CRD. + Checked bool + // Err explains a false Checked. + Err error +} + +// Verify compares every embedded CRD with the served one. +func Verify(ctx context.Context, client Getter) ([]Result, error) { + entries, err := crdFS.ReadDir("crd") + if err != nil { + return nil, fmt.Errorf("reading embedded CRDs: %w", err) + } + var out []Result + for _, e := range entries { + want, err := load(path.Join("crd", e.Name())) + if err != nil { + return nil, err + } + res := Result{Name: want.Name, File: e.Name()} + got, err := client.Get(ctx, want.Name, metav1.GetOptions{}) + switch { + case apierrors.IsNotFound(err): + res.Err = fmt.Errorf("not installed") + case err != nil: + res.Err = err + default: + res.Checked = true + res.Missing = missingPaths(want, got) + } + out = append(out, res) + } + return out, nil +} + +// FixCommand is what an operator should run to repair a stale schema. It is the +// whole point of reporting this: Helm will not do it for them. +// +// file is Result.File — the manifest's real name, not something derived from the +// CRD's name. +func FixCommand(file string) string { + return "kubectl apply -f https://raw.githubusercontent.com/kubeswift-io/gpucellpool/" + + Version + "/config/crd/bases/" + file +} + +// Version is the release whose CRD this binary embeds, stamped at build time +// (-ldflags "-X .../crdcheck.Version=vX.Y.Z"). It defaults to main so an +// unstamped dev build still prints a URL that resolves. +var Version = "main" + +func load(name string) (*apiextensionsv1.CustomResourceDefinition, error) { + raw, err := crdFS.ReadFile(name) + if err != nil { + return nil, fmt.Errorf("reading %s: %w", name, err) + } + var crd apiextensionsv1.CustomResourceDefinition + if err := yaml.Unmarshal(raw, &crd); err != nil { + return nil, fmt.Errorf("parsing %s: %w", name, err) + } + return &crd, nil +} + +// missingPaths returns the property paths present in want and absent from got, +// per served version. +func missingPaths(want, got *apiextensionsv1.CustomResourceDefinition) []string { + served := map[string]*apiextensionsv1.JSONSchemaProps{} + for i := range got.Spec.Versions { + v := &got.Spec.Versions[i] + if v.Schema != nil { + served[v.Name] = v.Schema.OpenAPIV3Schema + } + } + + var missing []string + for i := range want.Spec.Versions { + v := &want.Spec.Versions[i] + if v.Schema == nil || v.Schema.OpenAPIV3Schema == nil { + continue + } + schema, ok := served[v.Name] + if !ok { + // The whole version is unserved, which is worse than a missing field and + // worth naming as one line rather than every path beneath it. + missing = append(missing, v.Name+" (version not served)") + continue + } + missing = append(missing, walk(v.Name, v.Schema.OpenAPIV3Schema, schema)...) + } + sort.Strings(missing) + return missing +} + +// walk compares two schemas property by property, descending into objects and +// array items. A property the cluster does not have is recorded and not descended +// into: reporting the parent is enough to act on, and listing its whole subtree +// would bury the signal. +func walk(prefix string, want, got *apiextensionsv1.JSONSchemaProps) []string { + var missing []string + for name := range want.Properties { + wantChild := want.Properties[name] + gotChild, ok := got.Properties[name] + if !ok { + missing = append(missing, prefix+"."+name) + continue + } + missing = append(missing, walk(prefix+"."+name, &wantChild, &gotChild)...) + } + if want.Items != nil && want.Items.Schema != nil && got.Items != nil && got.Items.Schema != nil { + missing = append(missing, walk(prefix+"[]", want.Items.Schema, got.Items.Schema)...) + } + return missing +} diff --git a/internal/crdcheck/crdcheck_test.go b/internal/crdcheck/crdcheck_test.go new file mode 100644 index 0000000..68aba4f --- /dev/null +++ b/internal/crdcheck/crdcheck_test.go @@ -0,0 +1,182 @@ +package crdcheck + +import ( + "context" + "errors" + "strings" + "testing" + + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +type stubGetter struct { + crd *apiextensionsv1.CustomResourceDefinition + err error +} + +func (s stubGetter) Get(context.Context, string, metav1.GetOptions) (*apiextensionsv1.CustomResourceDefinition, error) { + return s.crd, s.err +} + +// embedded returns the CRD this binary was built against, which is also the +// baseline every case below mutates. +func embedded(t *testing.T) *apiextensionsv1.CustomResourceDefinition { + t.Helper() + entries, err := crdFS.ReadDir("crd") + if err != nil || len(entries) == 0 { + t.Fatalf("no CRD embedded (did `make manifests` run?): %v", err) + } + crd, err := load("crd/" + entries[0].Name()) + if err != nil { + t.Fatalf("load: %v", err) + } + return crd +} + +// TestTheEmbeddedCRDIsUsable guards the copy itself: an empty or unparseable embed +// would make every other check here vacuously pass. +func TestTheEmbeddedCRDIsUsable(t *testing.T) { + crd := embedded(t) + if crd.Name != "gpucellpools.cells.kubeswift.io" { + t.Errorf("embedded CRD name = %q", crd.Name) + } + if len(crd.Spec.Versions) == 0 || crd.Spec.Versions[0].Schema == nil { + t.Fatal("embedded CRD carries no schema") + } + // The two fields whose absence made this package necessary. + props := crd.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties + if _, ok := props["spec"].Properties["updatePolicy"]; !ok { + t.Error("spec.updatePolicy missing from the embedded CRD") + } +} + +func TestAServedSchemaEqualToTheBuildIsClean(t *testing.T) { + res, err := Verify(context.Background(), stubGetter{crd: embedded(t)}) + if err != nil { + t.Fatalf("Verify: %v", err) + } + if len(res) != 1 || !res[0].Checked || len(res[0].Missing) != 0 { + t.Errorf("got %+v, want one clean result", res) + } +} + +// TestADroppedFieldIsNamed is the v0.1.0 → v0.1.1 case: the served CRD predates +// spec.updatePolicy, so the apiserver would accept `RollingUpdate` and discard it. +func TestADroppedFieldIsNamed(t *testing.T) { + old := embedded(t).DeepCopy() + spec := old.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["spec"] + delete(spec.Properties, "updatePolicy") + old.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["spec"] = spec + + res, err := Verify(context.Background(), stubGetter{crd: old}) + if err != nil { + t.Fatalf("Verify: %v", err) + } + if !res[0].Checked { + t.Fatal("comparison not made") + } + if len(res[0].Missing) != 1 || !strings.HasSuffix(res[0].Missing[0], ".spec.updatePolicy") { + t.Errorf("Missing = %v, want exactly the dropped field", res[0].Missing) + } +} + +// TestADroppedFieldInsideAnArrayIsNamed covers status.cells[].templateHash — the +// other field the first upgrade would have dropped. It lives under array items, so +// a walk that did not descend into them would have reported nothing. +func TestADroppedFieldInsideAnArrayIsNamed(t *testing.T) { + old := embedded(t).DeepCopy() + root := old.Spec.Versions[0].Schema.OpenAPIV3Schema + status := root.Properties["status"] + cells := status.Properties["cells"] + item := cells.Items.Schema + if _, ok := item.Properties["templateHash"]; !ok { + t.Skip("status.cells[].templateHash is no longer in the schema") + } + delete(item.Properties, "templateHash") + + res, err := Verify(context.Background(), stubGetter{crd: old}) + if err != nil { + t.Fatalf("Verify: %v", err) + } + if len(res[0].Missing) != 1 || !strings.Contains(res[0].Missing[0], "templateHash") { + t.Errorf("Missing = %v, want the field under cells[]", res[0].Missing) + } +} + +// TestAMissingParentIsReportedOnce: reporting a dropped object's whole subtree would +// bury the one line an operator needs to act on. +func TestAMissingParentIsReportedOnce(t *testing.T) { + old := embedded(t).DeepCopy() + root := old.Spec.Versions[0].Schema.OpenAPIV3Schema + delete(root.Properties, "status") + + res, err := Verify(context.Background(), stubGetter{crd: old}) + if err != nil { + t.Fatalf("Verify: %v", err) + } + if len(res[0].Missing) != 1 || !strings.HasSuffix(res[0].Missing[0], ".status") { + t.Errorf("Missing = %v, want just the parent", res[0].Missing) + } +} + +func TestAnUnservedVersionIsReported(t *testing.T) { + old := embedded(t).DeepCopy() + old.Spec.Versions[0].Name = "v1beta9" + + res, err := Verify(context.Background(), stubGetter{crd: old}) + if err != nil { + t.Fatalf("Verify: %v", err) + } + if len(res[0].Missing) != 1 || !strings.Contains(res[0].Missing[0], "not served") { + t.Errorf("Missing = %v, want the unserved version named", res[0].Missing) + } +} + +// TestAnUnreadableCRDIsNotAVerdict: the operator's work is not conditional on being +// able to introspect its own CRD, so a Forbidden or a missing CRD must report +// "not checked" rather than "everything is missing". +func TestAnUnreadableCRDIsNotAVerdict(t *testing.T) { + cases := map[string]error{ + "forbidden": apierrors.NewForbidden( + schema.GroupResource{Group: "apiextensions.k8s.io", Resource: "customresourcedefinitions"}, + "gpucellpools.cells.kubeswift.io", errors.New("no rbac")), + "absent": apierrors.NewNotFound( + schema.GroupResource{Resource: "customresourcedefinitions"}, "gpucellpools.cells.kubeswift.io"), + } + for name, err := range cases { + t.Run(name, func(t *testing.T) { + res, vErr := Verify(context.Background(), stubGetter{err: err}) + if vErr != nil { + t.Fatalf("Verify returned an error instead of a result: %v", vErr) + } + if res[0].Checked { + t.Error("claimed to have compared a CRD it could not read") + } + if len(res[0].Missing) != 0 { + t.Errorf("Missing = %v, want none: unknown is not missing", res[0].Missing) + } + if res[0].Err == nil { + t.Error("no reason recorded") + } + }) + } +} + +// TestFixCommandPointsAtAFileThatExists: the first version derived the filename +// from the CRD's name, and controller-gen does not name files that way — the +// printed command 404'd. +func TestFixCommandPointsAtAFileThatExists(t *testing.T) { + res, err := Verify(context.Background(), stubGetter{crd: embedded(t)}) + if err != nil { + t.Fatalf("Verify: %v", err) + } + if _, err := crdFS.ReadFile("crd/" + res[0].File); err != nil { + t.Fatalf("Result.File %q is not the embedded manifest: %v", res[0].File, err) + } + if got := FixCommand(res[0].File); !strings.HasSuffix(got, "/config/crd/bases/"+res[0].File) { + t.Errorf("FixCommand = %q, want it to end in the real manifest path", got) + } +}