From 5ede07f4b3b23348168eee99928f7ac1da18f7b3 Mon Sep 17 00:00:00 2001 From: "Shiva Krishna, Merla" Date: Thu, 15 Aug 2024 13:19:10 -0700 Subject: [PATCH 1/2] Fix check for external PVC fix model store selection using below order 1. Use NIMCache instance if specified for NIMService 2. Create a new PVC if creation is enabled 3. Use an external/existing PVC if the name is provided Signed-off-by: Shiva Krishna, Merla --- api/apps/v1alpha1/nimcache_types.go | 6 +++--- api/apps/v1alpha1/nimservice_types.go | 6 +++--- api/apps/v1alpha1/zz_generated.deepcopy.go | 5 ----- internal/controller/nimcache_controller.go | 4 ++-- .../controller/platform/standalone/nimservice.go | 14 ++++++++------ .../platform/standalone/nimservice_test.go | 2 +- 6 files changed, 17 insertions(+), 20 deletions(-) diff --git a/api/apps/v1alpha1/nimcache_types.go b/api/apps/v1alpha1/nimcache_types.go index 8002b8ae..f16a9b9b 100644 --- a/api/apps/v1alpha1/nimcache_types.go +++ b/api/apps/v1alpha1/nimcache_types.go @@ -124,7 +124,7 @@ type PersistentVolumeClaim struct { // Create indicates to create a new PVC Create *bool `json:"create,omitempty"` // Name is the name of the PVC - Name *string `json:"name,omitempty"` + Name string `json:"name,omitempty"` // StorageClass to be used for PVC creation. Leave it as empty if the PVC is already created. StorageClass string `json:"storageClass,omitempty"` // Size of the NIM cache in Gi, used during PVC creation @@ -236,8 +236,8 @@ type NIMCache struct { // Prefers pvc.Name if explicitly set by the user in the NIMCache instance func (n *NIMCache) GetPVCName(pvc PersistentVolumeClaim) string { pvcName := fmt.Sprintf("%s-pvc", n.GetName()) - if pvc.Name != nil { - pvcName = *pvc.Name + if pvc.Name != "" { + pvcName = pvc.Name } return pvcName } diff --git a/api/apps/v1alpha1/nimservice_types.go b/api/apps/v1alpha1/nimservice_types.go index 3ab389a1..82a58fb4 100644 --- a/api/apps/v1alpha1/nimservice_types.go +++ b/api/apps/v1alpha1/nimservice_types.go @@ -120,8 +120,8 @@ type NIMServiceList struct { // Prefers pvc.Name if explicitly set by the user in the NIMService instance func (n *NIMService) GetPVCName(pvc PersistentVolumeClaim) string { pvcName := fmt.Sprintf("%s-pvc", n.GetName()) - if pvc.Name != nil { - pvcName = *pvc.Name + if pvc.Name != "" { + pvcName = pvc.Name } return pvcName } @@ -393,7 +393,7 @@ func (n *NIMService) GetVolumes(modelPVC PersistentVolumeClaim) []corev1.Volume Name: "model-store", VolumeSource: corev1.VolumeSource{ PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ - ClaimName: *modelPVC.Name, + ClaimName: modelPVC.Name, }, }, }, diff --git a/api/apps/v1alpha1/zz_generated.deepcopy.go b/api/apps/v1alpha1/zz_generated.deepcopy.go index 863967ee..625362b7 100644 --- a/api/apps/v1alpha1/zz_generated.deepcopy.go +++ b/api/apps/v1alpha1/zz_generated.deepcopy.go @@ -791,11 +791,6 @@ func (in *PersistentVolumeClaim) DeepCopyInto(out *PersistentVolumeClaim) { *out = new(bool) **out = **in } - if in.Name != nil { - in, out := &in.Name, &out.Name - *out = new(string) - **out = **in - } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PersistentVolumeClaim. diff --git a/internal/controller/nimcache_controller.go b/internal/controller/nimcache_controller.go index 7f71e18c..4b64ae23 100644 --- a/internal/controller/nimcache_controller.go +++ b/internal/controller/nimcache_controller.go @@ -570,8 +570,8 @@ func getJobName(nimCache *appsv1alpha1.NIMCache) string { func getPvcName(parent client.Object, pvc appsv1alpha1.PersistentVolumeClaim) string { pvcName := fmt.Sprintf("%s-pvc", parent.GetName()) - if pvc.Name != nil { - pvcName = *pvc.Name + if pvc.Name != "" { + pvcName = pvc.Name } return pvcName } diff --git a/internal/controller/platform/standalone/nimservice.go b/internal/controller/platform/standalone/nimservice.go index 79704d1a..e1700ba7 100644 --- a/internal/controller/platform/standalone/nimservice.go +++ b/internal/controller/platform/standalone/nimservice.go @@ -136,7 +136,7 @@ func (r *NIMServiceReconciler) reconcileNIMService(ctx context.Context, nimServi var modelPVC *appsv1alpha1.PersistentVolumeClaim modelProfile := "" - // If external PVC is provided, use that as model-store + // Select PVC for model store if nimService.GetNIMCacheName() != "" { // Fetch PVC for the associated NIMCache instance and mount it nimCachePVC, err := r.getNIMCachePVC(ctx, nimService) @@ -151,16 +151,18 @@ func (r *NIMServiceReconciler) reconcileNIMService(ctx context.Context, nimServi logger.Info("overriding model profile", "profile", profile) modelProfile = profile } - } else if externalPVC := nimService.GetExternalPVC(); externalPVC != nil { - modelPVC = externalPVC } else if nimService.Spec.Storage.PVC.Create != nil && *nimService.Spec.Storage.PVC.Create { + // Create a new PVC modelPVC, err = r.reconcilePVC(ctx, nimService) if err != nil { logger.Error(err, "unable to create pvc") return ctrl.Result{}, err } + } else if nimService.Spec.Storage.PVC.Name != "" { + // Use an existing PVC + modelPVC = &nimService.Spec.Storage.PVC } else { - err := fmt.Errorf("neither external PVC or NIMCache volume is provided") + err := fmt.Errorf("neither external PVC name or NIMCache volume is provided") logger.Error(err, "failed to determine PVC for model-store") return ctrl.Result{}, err } @@ -334,8 +336,8 @@ func (r *NIMServiceReconciler) getNIMCachePVC(ctx context.Context, nimService *a return nil, fmt.Errorf("missing PVC for the nimcache instance %s, nimservice %s", nimCache.GetName(), nimService.GetName()) } - if nimCache.Spec.Storage.PVC.Name == nil { - nimCache.Spec.Storage.PVC.Name = &nimCache.Status.PVC + if nimCache.Spec.Storage.PVC.Name == "" { + nimCache.Spec.Storage.PVC.Name = nimCache.Status.PVC } // Get the underlying PVC for the NIMCache instance return &nimCache.Spec.Storage.PVC, nil diff --git a/internal/controller/platform/standalone/nimservice_test.go b/internal/controller/platform/standalone/nimservice_test.go index 1796156f..ce149881 100644 --- a/internal/controller/platform/standalone/nimservice_test.go +++ b/internal/controller/platform/standalone/nimservice_test.go @@ -110,7 +110,7 @@ var _ = Describe("NIMServiceReconciler for a standalone platform", func() { Image: appsv1alpha1.Image{Repository: "nvcr.io/nvidia/nim-llm", PullPolicy: "IfNotPresent", Tag: "v0.1.0", PullSecrets: []string{"ngc-secret"}}, Storage: appsv1alpha1.Storage{ PVC: appsv1alpha1.PersistentVolumeClaim{ - Name: &pvcName, + Name: pvcName, }, }, NIMCache: appsv1alpha1.NIMCacheVolSpec{ From 1f5caa823d13230500474c4e1b8995562b268080 Mon Sep 17 00:00:00 2001 From: "Shiva Krishna, Merla" Date: Thu, 15 Aug 2024 14:48:53 -0700 Subject: [PATCH 2/2] Rebase Signed-off-by: Shiva Krishna, Merla --- api/apps/v1alpha1/nimservice_types.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/api/apps/v1alpha1/nimservice_types.go b/api/apps/v1alpha1/nimservice_types.go index 82a58fb4..f237aceb 100644 --- a/api/apps/v1alpha1/nimservice_types.go +++ b/api/apps/v1alpha1/nimservice_types.go @@ -434,11 +434,6 @@ func (n *NIMService) GetNIMCacheProfile() string { return n.Spec.NIMCache.Profile } -// GetExternalPVC returns the external PVC name to use for the NIMService deployment -func (n *NIMService) GetExternalPVC() *PersistentVolumeClaim { - return &n.Spec.Storage.PVC -} - // GetHPA returns the HPA spec for the NIMService deployment func (n *NIMService) GetHPA() HorizontalPodAutoscalerSpec { return n.Spec.Scale.HPA