Skip to content

feat: add SlurmQos CRD for declarative QOS management#158

Open
faganihajizada wants to merge 1 commit intoSlinkyProject:mainfrom
faganihajizada:feat/qos
Open

feat: add SlurmQos CRD for declarative QOS management#158
faganihajizada wants to merge 1 commit intoSlinkyProject:mainfrom
faganihajizada:feat/qos

Conversation

@faganihajizada
Copy link
Copy Markdown
Contributor

@faganihajizada faganihajizada commented Mar 25, 2026

Summary

Add declarative Slurm QOS management via a new SlurmQos CRD with controller and webhook. QOS policies are reconciled against the Slurm REST API (slurmdb endpoints), writing directly to the accounting database: no scontrol reconfigure required.

This supports the strategic shift from partition-centric to QOS-centric scheduling: fewer partitions, fine-grained preemption hierarchies, live policy updates, and lower scheduler load.

New CRD: SlurmQos

  • controllerRef scopes to a Controller CR (requires accounting enabled)
  • description for human-readable QOS description
  • config as runtime.RawExtension
  • metadata.name maps directly to the Slurm QOS name

Controller (internal/controller/slurmqos/):

  • Reconciles SlurmQos CRs via ClientMap -> slurmrestd -> slurmdbd
  • Finalizer-based deletion cleanup
  • Status with ready, id (Slurm-assigned), and Synced condition

Webhook (internal/webhook/slurmqos_webhook.go):

  • Validates controllerRef, QOS name format, JSON config, accounting requirement
  • Immutable controllerRef on update
  • Warns on "normal" QOS (Slurm creates one by default)

Helm chart:

  • qos map in values.yaml (gated by accounting.enabled)
  • Template renders SlurmQos CRs per entry
  • 7 unit tests

Depends on: SlinkyProject/slurm-client#2

Breaking Changes

N/A

Testing Notes

  • helm unittest helm/slurm/ --file tests/qos_test.yaml
  • Webhook tests: `go test ./internal/webhook/ -run SlurmQos
  • E2E verified on kind cluster with accounting enabled:
    • Create: QOS appears in Slurm with correct priority, flags, preempt mode
    • Update: kubectl patch reflected in Slurm within seconds
    • Delete: finalizer removes QOS from Slurm, then CR is garbage collected
    • Accounting guard: Synced=False with clear message when accountingRef is nil
  • make generate && make manifests

Additional Context

  • The config field uses runtime.RawExtension which is new to this operator's CRDs.
    Justified because QOS config maps to arbitrary JSON for the REST API (unlike partition config which is a flat key=value string). This is the standard K8s approach for embedding opaque JSON in CRDs.
  • QOS names are bare (not prefixed with chart fullname) because metadata.name must match the exact Slurm QOS name. The controllerRef provides cluster scoping.
  • Account-QOS associations (accounts={"nvidia": "denied"}) are out of scope: they require a separate SlurmAssociation CRD as a follow-up.
  • Flag values must use V0044 API enum format (e.g. DENY_LIMIT, not DenyOnLimit).

Known Limitations

  • Account-QOS associations are out of scope. QOS definitions that require account restrictions (e.g. accounts={"account_name": "denied"}) cannot be managed via the V0044Qos REST API. These require sacctmgr associations. Follow-up: a SlurmAssociation CRD using the same architecture pattern. The slurm-client already has generated OpenAPI types for associations. I will create separate PR.
  • Dynamic TRES limits are set manually per cluster. For example, the normal QOS needs max_tres_pu.nodes = 0.5 * cluster_node_count. Users must compute and set the absolute value at deploy time. The Helm values comments document the formula.

@faganihajizada
Copy link
Copy Markdown
Contributor Author

SlinkyProject/slurm-client#2 merge and release is needed before merging this PR

@SkylerMalinowski SkylerMalinowski self-requested a review March 25, 2026 22:58
@SkylerMalinowski
Copy link
Copy Markdown
Contributor

Accounting to Controller is 1:N. QoS is global to all Controller CR using the same Accounting CR. Therefore, architecturally the SlurmQos CRD should use an accountingRef, not a controllerRef. This means a Slurm client must be mapped to Accounting CR in addition to Controller CR.

Another issue that is not handled in the code is ownership of the QoS in Slurm. If I were to define 2 SlurmQoS both targeting the same QoS by name (e.g. normal), what stops the endless update/overwrite. Who wins and how? How is rejection handled?

I see why a raw v44 QoS is transparently passed from CR to client, but that is certainly a precarious thing to do. If the operator changes Slurm API version, the raw QoS may silently break if the Slurm API changed subtly. And the operator does not let the admin specify Slurm API version.

Comment on lines +68 to +88
slurmQos, err := buildV0044Qos(qosCopy)
if err != nil {
statusErr := r.syncStatus(ctx, qosCopy, false, nil, fmt.Sprintf("failed to build QOS payload: %v", err))
if statusErr != nil {
logger.Error(statusErr, "failed to update status")
}
return err
}

body := slurmapi.V0044OpenapiSlurmdbdQosResp{
Qos: []slurmapi.V0044Qos{slurmQos},
}
qosObj := &slurmtypes.V0044Qos{}
qosObj.Name = slurmQos.Name
if err := slurmClient.Create(ctx, qosObj, body); err != nil {
statusErr := r.syncStatus(ctx, qosCopy, false, nil, fmt.Sprintf("failed to create/update QOS: %v", err))
if statusErr != nil {
logger.Error(statusErr, "failed to update status")
}
return err
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All Slurm API handling should be abstracted through the slurmcontrol concept.

@SkylerMalinowski SkylerMalinowski self-assigned this Mar 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants