Merge pull request #12690 from chlins/fix/preheat-disable-instance-precheck

fix(preheat): add precheck before disable instance

src/controller/p2p/preheat/controller.go

@@ -184,11 +184,45 @@ func (c *controller) CreateInstance(ctx context.Context, instance *providerModel
 
 // DeleteInstance implements @Controller.Delete
 func (c *controller) DeleteInstance(ctx context.Context, id int64) error {
+	// delete instance should check the instance whether be used by policies
+	policies, err := c.ListPolicies(ctx, &q.Query{
+		Keywords: map[string]interface{}{
+			"provider_id": id,
+		},
+	})
+	if err != nil {
+		return err
+	}
+
+	if len(policies) > 0 {
+		return errors.New(nil).
+			WithCode(errors.PreconditionCode).
+			WithMessage("Can't delete instance %d, %d preheat policies use it as provider", id, len(policies))
+	}
+
 	return c.iManager.Delete(ctx, id)
 }
 
 // UpdateInstance implements @Controller.Update
 func (c *controller) UpdateInstance(ctx context.Context, instance *providerModels.Instance, properties ...string) error {
+	if !instance.Enabled {
+		// update instance should check the instance whether be used by policies
+		policies, err := c.ListPolicies(ctx, &q.Query{
+			Keywords: map[string]interface{}{
+				"provider_id": instance.ID,
+			},
+		})
+		if err != nil {
+			return err
+		}
+
+		if len(policies) > 0 {
+			return errors.New(nil).
+				WithCode(errors.PreconditionCode).
+				WithMessage("Can't disable instance %d, %d preheat policies use it as provider", instance.ID, len(policies))
+		}
+	}
+
 	return c.iManager.Update(ctx, instance, properties...)
 }
 

src/controller/p2p/preheat/controllor_test.go

@@ -168,21 +168,40 @@ func (s *preheatSuite) TestCreateInstance() {
 }
 
 func (s *preheatSuite) TestDeleteInstance() {
-	err := s.controller.DeleteInstance(s.ctx, 0)
+	// instance be used should not be deleted
-	s.Error(err)
+	s.fakePolicyMgr.On("ListPolicies", s.ctx, &q.Query{Keywords: map[string]interface{}{"provider_id": int64(1)}}).Return([]*policy.Schema{
+		{
+			ProviderID: 1,
+		},
+	}, nil)
+	err := s.controller.DeleteInstance(s.ctx, int64(1))
+	s.Error(err, "instance should not be deleted")
 
-	err = s.controller.DeleteInstance(s.ctx, int64(1))
+	s.fakePolicyMgr.On("ListPolicies", s.ctx, &q.Query{Keywords: map[string]interface{}{"provider_id": int64(2)}}).Return([]*policy.Schema{}, nil)
-	s.NoError(err)
+	s.fakeInstanceMgr.On("Delete", s.ctx, int64(2)).Return(nil)
+	err = s.controller.DeleteInstance(s.ctx, int64(2))
+	s.NoError(err, "instance can be deleted")
 }
 
 func (s *preheatSuite) TestUpdateInstance() {
-	s.fakeInstanceMgr.On("Update", s.ctx, mock.Anything).Return(errors.New("no properties provided to update"))
+	// normal update
-	err := s.controller.UpdateInstance(s.ctx, nil)
+	s.fakeInstanceMgr.On("Update", s.ctx, &providerModel.Instance{ID: 1000, Enabled: true}).Return(nil)
-	s.Error(err)
+	err := s.controller.UpdateInstance(s.ctx, &providerModel.Instance{ID: 1000, Enabled: true})
+	s.NoError(err, "instance can be updated")
 
-	s.fakeInstanceMgr.On("Update", mock.Anything, mock.Anything, mock.Anything).Return(nil)
+	// disable instance should error due to with policy used
-	err = s.controller.UpdateInstance(s.ctx, &providerModel.Instance{ID: 1}, "enabled")
+	s.fakeInstanceMgr.On("Update", s.ctx, &providerModel.Instance{ID: 1001}).Return(nil)
-	s.NoError(err)
+	s.fakePolicyMgr.On("ListPolicies", s.ctx, &q.Query{Keywords: map[string]interface{}{"provider_id": int64(1001)}}).Return([]*policy.Schema{
+		{ProviderID: 1001},
+	}, nil)
+	err = s.controller.UpdateInstance(s.ctx, &providerModel.Instance{ID: 1001})
+	s.Error(err, "instance should not be disabled")
+
+	// disable instance can be deleted if no policy used
+	s.fakeInstanceMgr.On("Update", s.ctx, &providerModel.Instance{ID: 1002}).Return(nil)
+	s.fakePolicyMgr.On("ListPolicies", s.ctx, &q.Query{Keywords: map[string]interface{}{"provider_id": int64(1002)}}).Return([]*policy.Schema{}, nil)
+	err = s.controller.UpdateInstance(s.ctx, &providerModel.Instance{ID: 1002})
+	s.NoError(err, "instance can be disabled")
 }
 
 func (s *preheatSuite) TestGetInstance() {
@@ -278,8 +297,8 @@ func (s *preheatSuite) TestDeletePolicy() {
 }
 
 func (s *preheatSuite) TestListPolicies() {
-	s.fakePolicyMgr.On("ListPolicies", s.ctx, mock.Anything).Return([]*policy.Schema{}, nil)
+	s.fakePolicyMgr.On("ListPolicies", s.ctx, &q.Query{}).Return([]*policy.Schema{}, nil)
-	p, err := s.controller.ListPolicies(s.ctx, nil)
+	p, err := s.controller.ListPolicies(s.ctx, &q.Query{})
 	s.NoError(err)
 	s.NotNil(p)
 }

src/server/v2.0/handler/preheat.go

@@ -81,22 +81,6 @@ func (api *preheatAPI) DeleteInstance(ctx context.Context, params operation.Dele
 		return api.SendError(ctx, err)
 	}
 
-	// delete instance should check the instance whether be used by policies
-	policies, err := api.preheatCtl.ListPolicies(ctx, &q.Query{
-		Keywords: map[string]interface{}{
-			"provider_id": instance.ID,
-		},
-	})
-	if err != nil {
-		return api.SendError(ctx, err)
-	}
-
-	if len(policies) > 0 {
-		return api.SendError(ctx, liberrors.New(nil).
-			WithCode(liberrors.PreconditionCode).
-			WithMessage("Can't delete instance %s, %d preheat policies use it as provider", instance.Name, len(policies)))
-	}
-
 	err = api.preheatCtl.DeleteInstance(ctx, instance.ID)
 	if err != nil {
 		return api.SendError(ctx, err)