Mis cleanup (#428)

Author: hzxuzhonghu

pkg/epp/controller/inferencemodel_reconciler.go

@@ -68,9 +68,9 @@ func (c *InferenceModelReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	logger = logger.WithValues("poolRef", infModel.Spec.PoolRef).WithValues("modelName", infModel.Spec.ModelName)
 	if !c.Datastore.ModelSetIfOlder(infModel) {
 		logger.Info("Skipping InferenceModel, existing instance has older creation timestamp")
-
+	} else {
+		logger.Info("Added/Updated InferenceModel")
 	}
-	logger.Info("Added/Updated InferenceModel")
 
 	return ctrl.Result{}, nil
 }
@@ -82,8 +82,8 @@ func (c *InferenceModelReconciler) handleModelDeleted(ctx context.Context, req t
 	// other instances referencing the same modelName if exist, and store the oldest in
 	// its place. This ensures that the InferenceModel with the oldest creation
 	// timestamp is active.
-	existing, exists := c.Datastore.ModelDelete(req)
-	if !exists {
+	existing := c.Datastore.ModelDelete(req)
+	if existing == nil {
 		// No entry exists in the first place, nothing to do.
 		return nil
 	}

pkg/epp/datastore/datastore.go

@@ -51,15 +51,15 @@ type Datastore interface {
 
 	// InferenceModel operations
 	ModelSetIfOlder(infModel *v1alpha2.InferenceModel) bool
-	ModelGet(modelName string) (*v1alpha2.InferenceModel, bool)
-	ModelDelete(namespacedName types.NamespacedName) (*v1alpha2.InferenceModel, bool)
+	ModelGet(modelName string) *v1alpha2.InferenceModel
+	ModelDelete(namespacedName types.NamespacedName) *v1alpha2.InferenceModel
 	ModelResync(ctx context.Context, ctrlClient client.Client, modelName string) (bool, error)
 	ModelGetAll() []*v1alpha2.InferenceModel
 
 	// PodMetrics operations
 	PodUpdateOrAddIfNotExist(pod *corev1.Pod) bool
 	PodUpdateMetricsIfExist(namespacedName types.NamespacedName, m *Metrics) bool
-	PodGet(namespacedName types.NamespacedName) (*PodMetrics, bool)
+	PodGet(namespacedName types.NamespacedName) *PodMetrics
 	PodDelete(namespacedName types.NamespacedName)
 	PodResyncAll(ctx context.Context, ctrlClient client.Client)
 	PodGetAll() []*PodMetrics
@@ -147,7 +147,6 @@ func (ds *datastore) PoolLabelsMatch(podLabels map[string]string) bool {
 	return poolSelector.Matches(podSet)
 }
 
-// /// InferenceModel APIs ///
 func (ds *datastore) ModelSetIfOlder(infModel *v1alpha2.InferenceModel) bool {
 	ds.poolAndModelsMu.Lock()
 	defer ds.poolAndModelsMu.Unlock()
@@ -199,23 +198,22 @@ func (ds *datastore) ModelResync(ctx context.Context, c client.Client, modelName
 	return true, nil
 }
 
-func (ds *datastore) ModelGet(modelName string) (*v1alpha2.InferenceModel, bool) {
+func (ds *datastore) ModelGet(modelName string) *v1alpha2.InferenceModel {
 	ds.poolAndModelsMu.RLock()
 	defer ds.poolAndModelsMu.RUnlock()
-	m, exists := ds.models[modelName]
-	return m, exists
+	return ds.models[modelName]
 }
 
-func (ds *datastore) ModelDelete(namespacedName types.NamespacedName) (*v1alpha2.InferenceModel, bool) {
+func (ds *datastore) ModelDelete(namespacedName types.NamespacedName) *v1alpha2.InferenceModel {
 	ds.poolAndModelsMu.Lock()
 	defer ds.poolAndModelsMu.Unlock()
 	for _, m := range ds.models {
 		if m.Name == namespacedName.Name && m.Namespace == namespacedName.Namespace {
 			delete(ds.models, m.Spec.ModelName)
-			return m, true
+			return m
 		}
 	}
-	return nil, false
+	return nil
 }
 
 func (ds *datastore) ModelGetAll() []*v1alpha2.InferenceModel {
@@ -238,12 +236,12 @@ func (ds *datastore) PodUpdateMetricsIfExist(namespacedName types.NamespacedName
 	return false
 }
 
-func (ds *datastore) PodGet(namespacedName types.NamespacedName) (*PodMetrics, bool) {
+func (ds *datastore) PodGet(namespacedName types.NamespacedName) *PodMetrics {
 	val, ok := ds.pods.Load(namespacedName)
 	if ok {
-		return val.(*PodMetrics), true
+		return val.(*PodMetrics)
 	}
-	return nil, false
+	return nil
 }
 
 func (ds *datastore) PodGetAll() []*PodMetrics {
@@ -311,7 +309,7 @@ func (ds *datastore) PodResyncAll(ctx context.Context, ctrlClient client.Client)
 		}
 	}
 
-	// Remove pods that don't exist or not ready any more.
+	// Remove pods that don't belong to the pool or not ready any more.
 	deleteFn := func(k, v any) bool {
 		pm := v.(*PodMetrics)
 		if exist := activePods[pm.NamespacedName.Name]; !exist {

pkg/epp/datastore/datastore_test.go

@@ -176,8 +176,8 @@ func TestModel(t *testing.T) {
 			name:           "Getting by model name, chat -> model2",
 			existingModels: []*v1alpha2.InferenceModel{model2chat, model1ts},
 			op: func(ds Datastore) bool {
-				gotChat, exists := ds.ModelGet(chatModel)
-				return exists && cmp.Diff(model2chat, gotChat) == ""
+				gotChat := ds.ModelGet(chatModel)
+				return gotChat != nil && cmp.Diff(model2chat, gotChat) == ""
 			},
 			wantOpResult: true,
 			wantModels:   []*v1alpha2.InferenceModel{model2chat, model1ts},
@@ -186,9 +186,9 @@ func TestModel(t *testing.T) {
 			name:           "Delete the model",
 			existingModels: []*v1alpha2.InferenceModel{model2chat, model1ts},
 			op: func(ds Datastore) bool {
-				_, existed := ds.ModelDelete(types.NamespacedName{Name: model1ts.Name, Namespace: model1ts.Namespace})
-				_, exists := ds.ModelGet(tsModel)
-				return existed && !exists
+				existing := ds.ModelDelete(types.NamespacedName{Name: model1ts.Name, Namespace: model1ts.Namespace})
+				got := ds.ModelGet(tsModel)
+				return existing != nil && got == nil
 
 			},
 			wantOpResult: true,

pkg/epp/handlers/request.go

@@ -64,8 +64,8 @@ func (s *Server) HandleRequestBody(
 	// NOTE: The nil checking for the modelObject means that we DO allow passthrough currently.
 	// This might be a security risk in the future where adapters not registered in the InferenceModel
 	// are able to be requested by using their distinct name.
-	modelObj, exist := s.datastore.ModelGet(model)
-	if !exist {
+	modelObj := s.datastore.ModelGet(model)
+	if modelObj == nil {
 		return nil, errutil.Error{Code: errutil.BadConfiguration, Msg: fmt.Sprintf("error finding a model object in InferenceModel for input %v", model)}
 	}
 	if len(modelObj.Spec.TargetModels) > 0 {

test/integration/hermetic_test.go

@@ -520,8 +520,8 @@ func BeforeSuit(t *testing.T) func() {
 	}
 
 	assert.EventuallyWithT(t, func(t *assert.CollectT) {
-		_, modelExist := serverRunner.Datastore.ModelGet("my-model")
-		synced := serverRunner.Datastore.PoolHasSynced() && modelExist
+		modelExist := serverRunner.Datastore.ModelGet("my-model")
+		synced := serverRunner.Datastore.PoolHasSynced() && modelExist != nil
 		assert.True(t, synced, "Timeout waiting for the pool and models to sync")
 	}, 10*time.Second, 10*time.Millisecond)