Auto-MNNVL: Update PCS's webhook support auto-mnnvl #370
Conversation
Mutating webhook (on create): - Automatically adds grove.io/auto-mnnvl: "true" annotation when MNNVL is globally enabled and PCS requests GPUs - Respects existing annotation values (allows opt-out with "false") Validating webhook (on create): - Rejects PCS with auto-mnnvl: "true" if MNNVL feature is disabled Validating webhook (on update): - Enforces annotation immutability after creation General: - Add flexible WithContainer/WithInitContainer builder methods - Add NewGPUContainer/NewContainer helper functions - Add WithAnnotations to PodCliqueSetBuilder - Remove deprecated test-envtest from the make test
shmuel-runai
requested review from
Ronkahn21,
gflarity,
sanjaychatterjee,
shayasoolin and
unmarshall
as code owners
| // If annotation already exists (user explicitly set it), don't override | ||
| if pcs.Annotations != nil { | ||
| if _, exists := pcs.Annotations[AnnotationAutoMNNVL]; exists { | ||
| return false |
|
|
||
| // Check if PCS has GPU requirements | ||
| if !hasGPURequirement(pcs) { | ||
| return false |
There was a problem hiding this comment.
add a debug log here (I don't expect it to happen frequently)
| if pcs.Annotations == nil { | ||
| pcs.Annotations = make(map[string]string) | ||
| } | ||
| pcs.Annotations[AnnotationAutoMNNVL] = "true" |
There was a problem hiding this comment.
repeating a comment from previous PR - just in case - are we using the values of true/false or perhaps something like enabled/required/auto/etc.?
| ) | ||
|
|
||
| // hasGPURequirement checks if any container in any clique of the PCS requests nvidia.com/gpu. | ||
| func hasGPURequirement(pcs *grovecorev1alpha1.PodCliqueSet) bool { |
There was a problem hiding this comment.
we should raise the question about DRA-provided gpu requests, where the claim name is provided but the actual claim details are separated to a ResourceClaimTemplate resource.
| // Validate MNNVL annotation: reject if annotation="true" but feature is disabled | ||
| if err := mnnvl.ValidateAutoMNNVLOnCreate(pcs, h.networkConfig.AutoMNNVLEnabled); err != nil { | ||
| allErrs = append(allErrs, field.Invalid( | ||
| field.NewPath("metadata", "annotations", mnnvl.AnnotationAutoMNNVL), |
There was a problem hiding this comment.
those details (e.g. annotation name) better be encapsulated within ValidateAutoMNNVLOnCreate and not repeated here. it's fragile.
|
|
||
| // Apply MNNVL auto-annotation if conditions are met | ||
| if mnnvl.MutateAutoMNNVL(pcs, h.networkConfig.AutoMNNVLEnabled) { | ||
| h.logger.Info("Added auto-mnnvl annotation", "PodCliqueSet", k8sutils.CreateObjectKeyForCreateWebhooks(pcs, req)) |
There was a problem hiding this comment.
consider moving this log into MutateAutoMNNVL. nit but the fact that mutating means adding an annotation is implementation-specific and is part of MutateAutoMNNVL's scope, not here.
2 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Mutating webhook (on create):
Validating webhook (on create):
Validating webhook (on update):
General:
Which issue(s) this PR fixes:
GREP-270
Special notes for your reviewer:
Does this PR introduce a API change?
NONE