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) + } +}