Add comments and logs for checking kserve deployment mode

Signed-off-by: Xieshen Zhang <xiezhang@redhat.com>

Author: xieshenzh

internal/controller/platform/kserve/nimservice_test.go

@@ -410,10 +410,10 @@ var _ = Describe("NIMServiceReconciler for a KServe platform", func() {
 
 		kserveDeployment := &appsv1.Deployment{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:      "kserve-controller-manager",
+				Name:      utils.KServeControllerName,
 				Namespace: "default",
 				Labels: map[string]string{
-					"app.kubernetes.io/name": "kserve-controller-manager",
+					"app.kubernetes.io/name": utils.KServeControllerName,
 				},
 			},
 			Spec: appsv1.DeploymentSpec{
@@ -507,7 +507,7 @@ var _ = Describe("NIMServiceReconciler for a KServe platform", func() {
 		By("delete the KServe Deployment")
 		kserveDeployment := &appsv1.Deployment{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:      "kserve-controller-manager",
+				Name:      utils.KServeControllerName,
 				Namespace: "default",
 			},
 		}

internal/utils/kserve.go

@@ -31,6 +31,11 @@ import (
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/log"
+)
+
+const (
+	KServeControllerName = "kserve-controller-manager"
 )
 
 func IsKServeStandardDeploymentMode(deploymentMode kserveconstants.DeploymentModeType) bool {
@@ -43,6 +48,8 @@ func IsKServeKnativeDeploymentMode(deploymentMode kserveconstants.DeploymentMode
 
 func GetKServeDeploymentMode(ctx context.Context, k8sClient client.Client,
 	podAnnotations map[string]string, isvcNamespacedName *types.NamespacedName) (kserveconstants.DeploymentModeType, error) {
+	logger := log.FromContext(ctx).WithName("KServe").WithName("Deployment Mode")
+
 	var annotations map[string]string
 	var deployConfig *kservev1beta1.DeployConfig
 	var statusDeploymentMode string
@@ -74,6 +81,9 @@ func GetKServeDeploymentMode(ctx context.Context, k8sClient client.Client,
 			if err != nil {
 				return "", err
 			}
+		} else {
+			logger.V(1).Info("ConfigMap inferenceservice-config is not found in the cluster, skipping the " +
+				"default deployment mode check from the ConfigMap")
 		}
 
 		// Try to get the deployment mode from the InferenceService status if it exists.
@@ -87,15 +97,21 @@ func GetKServeDeploymentMode(ctx context.Context, k8sClient client.Client,
 				if !k8serrors.IsNotFound(err) {
 					return "", err
 				}
+				logger.V(1).Info("InferenceService not found, deployment mode from status is not available",
+					"InferenceService", isvcNamespacedName.String())
 			} else {
 				statusDeploymentMode = isvc.Status.DeploymentMode
 			}
+		} else {
+			logger.V(1).Info("InferenceService name is not set, skipping deployment mode check from InferenceService status")
 		}
 	} else {
+		logger.V(1).Info("k8sClient is nil, skipping default deployment mode retrieval from the inferenceservice-config ConfigMap, " +
+			"skipping deployment mode check from InferenceService status, using the annotations only")
 		annotations = podAnnotations
 	}
 
-	deploymentMode := getDeploymentMode(statusDeploymentMode, annotations, deployConfig)
+	deploymentMode := getDeploymentMode(ctx, statusDeploymentMode, annotations, deployConfig)
 	return deploymentMode, nil
 }
 
@@ -113,10 +129,14 @@ case 2: serving.kserve.org/deploymentMode is set
 ODH 3.0 supports "RawDeployment", "Serverless", and doesn't accept "Standard", "Knative"
 Ref: https://github.com/opendatahub-io/kserve/blob/cf2920b4276d97fc2d2d700efe4879749a56b418/pkg/controller/v1beta1/inferenceservice/utils/utils.go#L220
 */
-func getDeploymentMode(statusDeploymentMode string, annotations map[string]string,
+func getDeploymentMode(ctx context.Context, statusDeploymentMode string, annotations map[string]string,
 	deployConfig *kservev1beta1.DeployConfig) kserveconstants.DeploymentModeType {
+	logger := log.FromContext(ctx).WithName("KServe").WithName("Deployment Mode")
+
 	// First priority is the deploymentMode recorded in the status
 	if len(statusDeploymentMode) != 0 {
+		logger.Info("using deployment mode from InferenceService status",
+			"Deployment Mode", statusDeploymentMode)
 		return kserveconstants.DeploymentModeType(statusDeploymentMode)
 	}
 
@@ -132,16 +152,30 @@ func getDeploymentMode(statusDeploymentMode string, annotations map[string]strin
 			deploymentMode == string(kserveconstants.ModelMeshDeployment) ||
 			deploymentMode == string(kserveconstants.LegacyRawDeployment) ||
 			deploymentMode == string(kserveconstants.LegacyServerless)) {
+			logger.Info("using deployment mode from annotations",
+				"Deployment Mode", deploymentMode)
 			return kserveconstants.DeploymentModeType(deploymentMode)
 		}
+
+		if ok {
+			logger.V(1).Info("warning: deployment mode annotation found but value is invalid",
+				"Deployment Mode", deploymentMode)
+		} else {
+			logger.V(1).Info("warning: deployment mode annotation not found in annotations")
+		}
 	}
 
 	if deployConfig != nil {
 		// Finally, if an InferenceService is being created and does not explicitly specify a DeploymentMode
+		logger.Info("using the default deployment mode from the inferenceservice-config ConfigMap",
+			"Deployment Mode", deployConfig.DefaultDeploymentMode)
 		return kserveconstants.DeploymentModeType(deployConfig.DefaultDeploymentMode)
 	}
 
 	// If InferenceServicesConfig doesn't exist, use the default deployment mode
+	// For ODH, InferenceServicesConfig is always bundled, and this line should not be reached
+	logger.Info("deployment mode is not found in any configurations, using default deployment mode",
+		"Deployment Mode", kserveconstants.DefaultDeployment)
 	return kserveconstants.DefaultDeployment
 }
 
@@ -183,12 +217,13 @@ func newDeployConfig(isvcConfigMap *corev1.ConfigMap) (*kservev1beta1.DeployConf
 
 func getISVCConfigMap(ctx context.Context, k8sClient client.Client) (*corev1.ConfigMap, error) {
 	var namespace string
+	// Find the namespace of the KServe controller
 	deploymentList := &appsv1.DeploymentList{}
-	if err := k8sClient.List(ctx, deploymentList, client.HasLabels{"app.kubernetes.io/name: kserve-controller-manager"}); err != nil {
+	if err := k8sClient.List(ctx, deploymentList, client.MatchingLabels{"app.kubernetes.io/name": KServeControllerName}); err != nil {
 		return nil, err
 	}
 	for _, deployment := range deploymentList.Items {
-		if deployment.GetName() == "kserve-controller-manager" {
+		if deployment.GetName() == KServeControllerName {
 			namespace = deployment.Namespace
 			break
 		}
@@ -198,6 +233,7 @@ func getISVCConfigMap(ctx context.Context, k8sClient client.Client) (*corev1.Con
 		return nil, fmt.Errorf("failed to find the namespace of KServe Deployment")
 	}
 
+	// Get the inferenceservice-config ConfigMap in the namespace
 	isvcConfigMap := &corev1.ConfigMap{}
 	err := k8sClient.Get(ctx,
 		types.NamespacedName{

internal/utils/kserve_test.go

@@ -54,10 +54,10 @@ var _ = Describe("KServe Utilities", func() {
 		createKServeDeployment := func() *appsv1.Deployment {
 			return &appsv1.Deployment{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:      "kserve-controller-manager",
+					Name:      KServeControllerName,
 					Namespace: namespace,
 					Labels: map[string]string{
-						"app.kubernetes.io/name": "kserve-controller-manager",
+						"app.kubernetes.io/name": KServeControllerName,
 					},
 				},
 				Spec: appsv1.DeploymentSpec{
@@ -101,10 +101,10 @@ var _ = Describe("KServe Utilities", func() {
 
 			kserveDeployment = &appsv1.Deployment{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:      "kserve-controller-manager",
+					Name:      KServeControllerName,
 					Namespace: namespace,
 					Labels: map[string]string{
-						"app.kubernetes.io/name": "kserve-controller-manager",
+						"app.kubernetes.io/name": KServeControllerName,
 					},
 				},
 				Spec: appsv1.DeploymentSpec{

internal/webhook/apps/v1alpha1/nimservice_webhook.go

@@ -100,8 +100,7 @@ func (v *NIMServiceCustomValidator) ValidateCreate(_ context.Context, obj runtim
 	fldPath := field.NewPath("nimservice").Child("spec")
 
 	// Perform comprehensive spec validation via helper.
-	namespacedName := client.ObjectKeyFromObject(nimservice)
-	warningList, errList := validateNIMServiceSpec(&nimservice.Spec, fldPath, v.k8sVersion, v.k8sClient, &namespacedName)
+	warningList, errList := validateNIMServiceSpec(nimservice, fldPath, v.k8sVersion, v.k8sClient)
 
 	if len(errList) > 0 {
 		return warningList, errList.ToAggregate()
@@ -123,9 +122,8 @@ func (v *NIMServiceCustomValidator) ValidateUpdate(_ context.Context, oldObj, ne
 	nimservicelog.V(4).Info("Validation for NIMService upon update", "name", newNIMService.GetName())
 
 	fldPath := field.NewPath("nimservice").Child("spec")
-	namespacedName := client.ObjectKeyFromObject(newNIMService)
 	// Start with structural validation to ensure the updated object is well formed.
-	warningList, errList := validateNIMServiceSpec(&newNIMService.Spec, fldPath, v.k8sVersion, v.k8sClient, &namespacedName)
+	warningList, errList := validateNIMServiceSpec(newNIMService, fldPath, v.k8sVersion, v.k8sClient)
 
 	wList, eList := validateMultiNodeImmutability(oldNIMService, newNIMService, field.NewPath("spec").Child("multiNode"))
 	warningList = append(warningList, wList...)

internal/webhook/apps/v1alpha1/nimservice_webhook_validation_helper.go

@@ -67,8 +67,10 @@ var validDRAResourceQuantitySelectorOps = []appsv1alpha1.DRAResourceQuantitySele
 // object. It is intended to be invoked by both ValidateCreate and ValidateUpdate to
 // ensure the resource is well-formed before any other validation (e.g. immutability)
 // is performed.
-func validateNIMServiceSpec(spec *appsv1alpha1.NIMServiceSpec, fldPath *field.Path, kubeVersion string,
-	k8sClient client.Client, namespacedName *types.NamespacedName) (admission.Warnings, field.ErrorList) {
+func validateNIMServiceSpec(nimservice *appsv1alpha1.NIMService, fldPath *field.Path, kubeVersion string,
+	k8sClient client.Client) (admission.Warnings, field.ErrorList) {
+	spec := &nimservice.Spec
+
 	warningList, errList := validateImageConfiguration(&spec.Image, fldPath.Child("image"))
 
 	// TODO abstract all validation functions with a single signature validateFunc(*appsv1alpha1.NIMServiceSpec, *field.Path) (admission.Warnings, field.ErrorList)
@@ -104,7 +106,8 @@ func validateNIMServiceSpec(spec *appsv1alpha1.NIMServiceSpec, fldPath *field.Pa
 	warningList = append(warningList, w...)
 	errList = append(errList, err...)
 
-	w, err = validateKServeConfiguration(spec, fldPath, namespacedName, k8sClient)
+	namespacedName := client.ObjectKeyFromObject(nimservice)
+	w, err = validateKServeConfiguration(spec, fldPath, &namespacedName, k8sClient)
 	warningList = append(warningList, w...)
 	errList = append(errList, err...)
 
@@ -513,9 +516,7 @@ func validateKServeConfiguration(spec *appsv1alpha1.NIMServiceSpec, fldPath *fie
 	errList := field.ErrorList{}
 	warningList := admission.Warnings{}
 
-	platformIsKServe := spec.InferencePlatform == appsv1alpha1.PlatformTypeKServe
-
-	if platformIsKServe {
+	if spec.InferencePlatform == appsv1alpha1.PlatformTypeKServe {
 		// Get the deployment mode
 		mode, err := utils.GetKServeDeploymentMode(context.TODO(), k8sClient, spec.Annotations, namespacedName)
 		if err != nil {