feat: add Account and User CRDs for Slurm accounting#216
Closed
giuliocalzo wants to merge 23 commits into
Closed
Conversation
Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
Move DeletionPolicy into account_types.go and AdminLevel into user_types.go, and place the limits struct (renamed AccountingLimits -> AssociationLimits) alongside UserAssociation in a new association_types.go. Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
Add AccountingControlInterface bridging the Account and User CRs to the v0044 slurmdbd accounts/users/associations endpoints, with pure helpers that map AssociationLimits into the nested assoc payload via JSON merge. GrpJobs/GrpSubmitJobs/GrpWall and non-numeric Fairshare have no v0044 assoc target and are intentionally not mapped. Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
Validate controllerRef presence and immutability, defaultQos membership in the qos list, and (for User) non-empty associations, defaultAccount membership, and duplicate account/partition detection. Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
Register the Account and User reconcilers in the manager and their validating webhooks in the webhook server, add PROJECT entries, and regenerate CRDs, RBAC, webhook config, and Helm CRD templates. Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
Wire an events.EventRecorder into the Account and User reconcilers (mirroring the token/nodeset controllers) and emit events to the CR on: - Ready condition transitions (Synced / SyncFailed / NoClient / AccountNotReady), gated on transition to avoid per-reconcile spam. - Slurm-side deletion (Deleted / Orphaned per deletionPolicy, DeleteFailed). Sync tests now assert a Synced event is emitted on first reconcile. Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
The User CRD's spec-level CEL rule (self.associations.exists(a, a.account == self.defaultAccount)) was rejected by the API server because associations had no maxItems and the compared strings no maxLength, so the cost estimator assumed unbounded sizes and exceeded the budget. Add maxItems=128 to associations and maxLength=64 to defaultAccount and association.account, which bounds the rule cost. Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
Document the Account and User CRDs: prerequisites, example manifests, field references, association limits, the User->Account dependency gate, status conditions, emitted events, deletion policy, and webhook validation rules. Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
Replace the local-filesystem replace directive (which is unavailable inside the Docker build context and broke `go mod download`) with the giuliocalzo/slurm-client fork pinned to the feat/account-user-crds branch commit so container builds can resolve the dependency. Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
slurmdbd scopes associations to a cluster: a POST that omits the cluster returns 200 but is silently dropped, and the slurm-client read-back (keyed cluster/account/user/partition) then fails with Not Found. This left Account and User resources permanently SyncFailed. Discover the cluster name at runtime from an existing association (the root association always exists once slurmdbd is up), cache it per controllerRef, and set it on both the account-level and user associations. Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
Exercise the Account and User controllers end-to-end against slurmdbd: create the CRs, wait for Ready, verify the account/user/association land in slurmdbd via sacctmgr, and confirm finalizer-driven cleanup on deletion. Gated on the accounting install path. Also drop the non-existent "high" QOS from the sample Account manifest so it applies cleanly on a default Slurm install. Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
giuliocalzo
requested review from
SkylerMalinowski,
alanmutsch,
catblade,
vivian-hafener and
wickberg
as code owners
The applyLimits mapping wrote several limits to the wrong nested v0044 association schema paths, so they were silently dropped by slurmdbd (the POST returned 200 but the limit was not stored). The correct paths were established empirically by setting each limit via sacctmgr and reading it back through the REST API: GrpJobs -> max.jobs.per.count (was unmapped) GrpSubmitJobs -> max.jobs.per.submitted (was unmapped) GrpWall -> max.per.account.wall_clock (was unmapped) GrpTRES -> max.tres.total (was max.tres.group.active) MaxJobs/MaxSubmitJobs/MaxWallPerJob/MaxTRESPerJob were already correct. Add a comprehensive unit test exercising every AssociationLimits field and expand the sample Account/User manifests to set all fields for end-to-end validation. Sample TRES use cpu/mem (tracked) since gres/gpu is not tracked on a default install and is silently dropped. Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
Two issues surfaced while validating the Orphan deletion policy with a minimal Account/User: - slurmdbd rejects account creation when description or organization is empty (HTTP 500 "slurmdb_accounts_add() failed"), which the operator surfaced as an opaque "Internal Server Error". Default both to the account name when unset, mirroring sacctmgr's own behavior. - An unchanged upsert returns HTTP 304 Not Modified, which the client reports as an error and the controller flipped to SyncFailed even though the entity already matched. Treat Not Modified as success in ApplyAccount/ApplyUser via a new ignoreNotModified helper. Add unit coverage for the defaulting and the Not Modified tolerance. Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
Default User.defaultAccount to the first association's account (make it optional and relax the CEL rule to allow an empty value at admission). Default Account.organization to "root" instead of the account name. accountName/userName already default to metadata.name, adminLevel to None, and parentAccount/deletionPolicy to their documented values; trim the sample manifests to exercise these defaults. Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
Add cluster templates that render Account/User CRs from the new .Values.accounts and .Values.users maps (keyed by resource name), with controllerRef defaulting to the chart controller. Include helm-unittest suites and snapshots covering rendering, defaults, overrides, and multi-document output. Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds two new namespaced CRDs —
AccountandUser(slinky.slurm.net/v1beta1) — that declaratively manage Slurm accounting entities (slurmdbd accounts/users and their associations) through the operator viaslurmrestd.Account,User, and sharedAssociationLimits/UserAssociation(limits, QoS, fairshare, TRES), plusDeletionPolicy(Delete|Orphan) andAdminLevel(None|Operator|Administrator) enums. Defaulting for both kinds.internal/controller/sacctmgr): Account and User reconcilers with finalizers, status conditions, and Kubernetes event emission on state transitions. The User controller gates on Account readiness (eventual consistency) and the operator enforces the CR as source of truth (drift reversion).slurm-clientv0044 slurmdb API calls, including runtime discovery of the Slurm cluster name (required so slurmdbd does not silently drop association writes).maxItems/maxLength.PROJECT; regenerated CRDs, RBAC, webhook configs, deepcopy, and Helm chart templates.docs/usage/accounting.mdand sample manifests underhack/resources/.sacctmgr→ finalizer cleanup), gated on the accounting install path.Dependency
This builds on the new accounting types in the slurm-client library:
SlinkyProject/slurm-client#3
Until that PR merges,
go.modcarries a temporaryreplacedirective pointinggithub.com/SlinkyProject/slurm-clientat the fork branch pseudo-version. This should be dropped (and pinned to a released slurm-client version) once #3 lands.Test plan
make build/go vet ./...go test ./...(unit tests for API types, defaults, slurmcontrol mapping, controllers, webhooks)make lint(golangci-lint clean)make generate manifests)