Skip to content

Commit

Permalink
Fix some issues in manual mode (#235)
Browse files Browse the repository at this point in the history 
* fix panic

Signed-off-by: haorenfsa <[email protected]>

* fix unexpected rollout when switching configmap

Signed-off-by: haorenfsa <[email protected]>

---------

Signed-off-by: haorenfsa <[email protected]>
  • Loading branch information
@haorenfsa
haorenfsa authored Jan 16, 2025
1 parent a7cf9b1 commit 4e34fe9
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 39 deletions.
17 changes: 9 additions & 8 deletions pkg/controllers/deploy_ctrl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,16 +145,14 @@ var _ DeployControllerBiz = &DeployControllerBizImpl{}
type DeployControllerBizImpl struct {
component MilvusComponent
DeployModeChanger
statusSyncer MilvusStatusSyncerInterface
util DeployControllerBizUtil
cli client.Client
util DeployControllerBizUtil
cli client.Client
}

func NewDeployControllerBizImpl(component MilvusComponent, statusSyncer MilvusStatusSyncerInterface, util DeployControllerBizUtil, modeChanger DeployModeChanger, cli client.Client) *DeployControllerBizImpl {
func NewDeployControllerBizImpl(component MilvusComponent, util DeployControllerBizUtil, modeChanger DeployModeChanger, cli client.Client) *DeployControllerBizImpl {
return &DeployControllerBizImpl{
component: component,
DeployModeChanger: modeChanger,
statusSyncer: statusSyncer,
util: util,
cli: cli,
}
Expand Down Expand Up @@ -200,9 +198,8 @@ func (c *DeployControllerBizImpl) IsUpdating(ctx context.Context, mc v1beta1.Mil
if mc.Spec.IsStopping() {
return false, nil
}
err := c.statusSyncer.UpdateStatusForNewGeneration(ctx, &mc, false)
if err != nil {
return false, errors.Wrap(err, "update status for new generation")
if mc.Status.ObservedGeneration < mc.Generation {
return true, nil
}
cond := v1beta1.GetMilvusConditionByType(&mc.Status, v1beta1.MilvusUpdated)
if cond == nil || cond.Status != corev1.ConditionTrue {
Expand Down Expand Up @@ -325,6 +322,10 @@ func (c *DeployControllerBizImpl) HandleManualMode(ctx context.Context, mc v1bet
if currentDeploy == nil {
return errors.Errorf("[%s]'s current deployment not found", c.component.Name)
}
// should not update deploy with replicas if it's in manual mode
if getDeployReplicas(currentDeploy) != 0 {
return nil
}
podTemplate := c.util.RenderPodTemplateWithoutGroupID(mc, &currentDeploy.Spec.Template, c.component, true)
if c.util.IsNewRollout(ctx, currentDeploy, podTemplate) {
return c.util.PrepareNewRollout(ctx, mc, currentDeploy, podTemplate)
Expand Down
5 changes: 2 additions & 3 deletions pkg/controllers/deploy_ctrl_factory.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,17 @@ type DeployControllerBizFactoryImpl struct {
}

// NewDeployControllerBizFactory creates a new DeployControllerBizFactory
func NewDeployControllerBizFactory(modeChangerFactory DeployModeChangerFactory, statusSyncer MilvusStatusSyncerInterface, utilFactory DeployControllerBizUtilFactory, cli client.Client) *DeployControllerBizFactoryImpl {
func NewDeployControllerBizFactory(modeChangerFactory DeployModeChangerFactory, utilFactory DeployControllerBizUtilFactory, cli client.Client) *DeployControllerBizFactoryImpl {
return &DeployControllerBizFactoryImpl{
modeChangerFactory: modeChangerFactory,
statusSyncer: statusSyncer,
utilFactory: utilFactory,
cli: cli,
}
}

// GetBiz get DeployControllerBiz for the given component
func (f *DeployControllerBizFactoryImpl) GetBiz(component MilvusComponent) DeployControllerBiz {
return NewDeployControllerBizImpl(component, f.statusSyncer, f.utilFactory.GetBizUtil(component), f.modeChangerFactory.GetDeployModeChanger(component), f.cli)
return NewDeployControllerBizImpl(component, f.utilFactory.GetBizUtil(component), f.modeChangerFactory.GetDeployModeChanger(component), f.cli)
}

type DeployModeChangerFactory interface {
Expand Down
47 changes: 20 additions & 27 deletions pkg/controllers/deploy_ctrl_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,11 +190,10 @@ func TestDeployControllerImpl_Reconcile(t *testing.T) {
func TestDeployControllerBizImpl_CheckDeployMode(t *testing.T) {
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()
mockStatusSyncer := NewMockMilvusStatusSyncerInterface(mockCtrl)
mockUtil := NewMockDeployControllerBizUtil(mockCtrl)
mockCli := NewMockK8sClient(mockCtrl)
mockModeChanger := NewMockDeployModeChanger(mockCtrl)
bizImpl := NewDeployControllerBizImpl(QueryNode, mockStatusSyncer, mockUtil, mockModeChanger, mockCli)
bizImpl := NewDeployControllerBizImpl(QueryNode, mockUtil, mockModeChanger, mockCli)
mc := v1beta1.Milvus{}
component := QueryNode
t.Run("status v2 shows twoDeploy", func(t *testing.T) {
Expand Down Expand Up @@ -222,11 +221,10 @@ func TestDeployControllerBizImpl_CheckDeployMode(t *testing.T) {
func TestDeployControllerBizImpl_IsUpdating(t *testing.T) {
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()
mockStatusSyncer := NewMockMilvusStatusSyncerInterface(mockCtrl)
mockUtil := NewMockDeployControllerBizUtil(mockCtrl)
mockCli := NewMockK8sClient(mockCtrl)
mockModeChanger := NewMockDeployModeChanger(mockCtrl)
bizImpl := NewDeployControllerBizImpl(QueryNode, mockStatusSyncer, mockUtil, mockModeChanger, mockCli)
bizImpl := NewDeployControllerBizImpl(QueryNode, mockUtil, mockModeChanger, mockCli)
mc := v1beta1.Milvus{}
mc.Default()
component := QueryNode
Expand All @@ -245,21 +243,16 @@ func TestDeployControllerBizImpl_IsUpdating(t *testing.T) {
mc.Spec.Com.Standalone.Replicas = int32Ptr(1)
})

t.Run("update status failed", func(t *testing.T) {
mockStatusSyncer.EXPECT().UpdateStatusForNewGeneration(ctx, &mc, false).Return(errMock)
_, err := bizImpl.IsUpdating(ctx, mc)
assert.Error(t, err)
})

t.Run("milvus condition not exist, sugguests updating", func(t *testing.T) {
mockStatusSyncer.EXPECT().UpdateStatusForNewGeneration(ctx, &mc, false).Return(nil)
mc.Generation = 2
mc.Status.ObservedGeneration = 1
t.Run("generation not updated, sugguests updating", func(t *testing.T) {
ret, err := bizImpl.IsUpdating(ctx, mc)
assert.NoError(t, err)
assert.True(t, ret)
})

mc.Status.ObservedGeneration = 2
t.Run("milvus condition shows updating", func(t *testing.T) {
mockStatusSyncer.EXPECT().UpdateStatusForNewGeneration(ctx, &mc, false).Return(nil)
mc.Status.Conditions = []v1beta1.MilvusCondition{
{
Type: v1beta1.MilvusUpdated,
Expand All @@ -279,14 +272,12 @@ func TestDeployControllerBizImpl_IsUpdating(t *testing.T) {
}

t.Run("get old deploy failed", func(t *testing.T) {
mockStatusSyncer.EXPECT().UpdateStatusForNewGeneration(ctx, &mc, false).Return(nil)
mockUtil.EXPECT().GetOldDeploy(ctx, mc, component).Return(nil, errMock)
_, err := bizImpl.IsUpdating(ctx, mc)
assert.Error(t, err)
})

t.Run("is new rollout, so updating", func(t *testing.T) {
mockStatusSyncer.EXPECT().UpdateStatusForNewGeneration(ctx, &mc, false).Return(nil)
deploy := appsv1.Deployment{}

mockUtil.EXPECT().GetOldDeploy(ctx, mc, component).Return(&deploy, nil)
Expand All @@ -299,7 +290,6 @@ func TestDeployControllerBizImpl_IsUpdating(t *testing.T) {
})

t.Run("not updating", func(t *testing.T) {
mockStatusSyncer.EXPECT().UpdateStatusForNewGeneration(ctx, &mc, false).Return(nil)
deploy := appsv1.Deployment{}

mockUtil.EXPECT().GetOldDeploy(ctx, mc, component).Return(&deploy, nil)
Expand All @@ -315,7 +305,7 @@ func TestDeployControllerBizImpl_IsUpdating(t *testing.T) {
func TestDeployControllerBizImpl_IsPaused(t *testing.T) {
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()
bizImpl := NewDeployControllerBizImpl(QueryNode, nil, nil, nil, nil)
bizImpl := NewDeployControllerBizImpl(QueryNode, nil, nil, nil)
mc := v1beta1.Milvus{}
mc.Spec.Mode = v1beta1.MilvusModeCluster
mc.Default()
Expand All @@ -338,11 +328,10 @@ func TestDeployControllerBizImpl_IsPaused(t *testing.T) {
func TestDeployControllerBizImpl_HandleCreate(t *testing.T) {
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()
mockStatusSyncer := NewMockMilvusStatusSyncerInterface(mockCtrl)
mockUtil := NewMockDeployControllerBizUtil(mockCtrl)
mockCli := NewMockK8sClient(mockCtrl)
mockModeChanger := NewMockDeployModeChanger(mockCtrl)
bizImpl := NewDeployControllerBizImpl(QueryNode, mockStatusSyncer, mockUtil, mockModeChanger, mockCli)
bizImpl := NewDeployControllerBizImpl(QueryNode, mockUtil, mockModeChanger, mockCli)
mc := v1beta1.Milvus{}
deploy := appsv1.Deployment{}
t.Run("get querynode deploy failed", func(t *testing.T) {
Expand Down Expand Up @@ -383,11 +372,10 @@ func TestDeployControllerBizImpl_HandleCreate(t *testing.T) {
func TestDeployControllerBizImpl_HandleStop(t *testing.T) {
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()
mockStatusSyncer := NewMockMilvusStatusSyncerInterface(mockCtrl)
mockUtil := NewMockDeployControllerBizUtil(mockCtrl)
mockCli := NewMockK8sClient(mockCtrl)
mockModeChanger := NewMockDeployModeChanger(mockCtrl)
bizImpl := NewDeployControllerBizImpl(QueryNode, mockStatusSyncer, mockUtil, mockModeChanger, mockCli)
bizImpl := NewDeployControllerBizImpl(QueryNode, mockUtil, mockModeChanger, mockCli)
mc := v1beta1.Milvus{}
mc.Spec.Mode = v1beta1.MilvusModeCluster
mc.Default()
Expand Down Expand Up @@ -432,11 +420,10 @@ func TestDeployControllerBizImpl_HandleStop(t *testing.T) {
func TestDeployControllerBizImpl_HandleScaling(t *testing.T) {
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()
mockStatusSyncer := NewMockMilvusStatusSyncerInterface(mockCtrl)
mockUtil := NewMockDeployControllerBizUtil(mockCtrl)
mockCli := NewMockK8sClient(mockCtrl)
mockModeChanger := NewMockDeployModeChanger(mockCtrl)
bizImpl := NewDeployControllerBizImpl(QueryNode, mockStatusSyncer, mockUtil, mockModeChanger, mockCli)
bizImpl := NewDeployControllerBizImpl(QueryNode, mockUtil, mockModeChanger, mockCli)
mc := v1beta1.Milvus{}
mc.Spec.Mode = v1beta1.MilvusModeCluster
mc.Default()
Expand Down Expand Up @@ -469,11 +456,10 @@ func TestDeployControllerBizImpl_HandleScaling(t *testing.T) {
func TestDeployControllerBizImpl_HandleRolling(t *testing.T) {
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()
mockStatusSyncer := NewMockMilvusStatusSyncerInterface(mockCtrl)
mockUtil := NewMockDeployControllerBizUtil(mockCtrl)
mockCli := NewMockK8sClient(mockCtrl)
mockModeChanger := NewMockDeployModeChanger(mockCtrl)
bizImpl := NewDeployControllerBizImpl(QueryNode, mockStatusSyncer, mockUtil, mockModeChanger, mockCli)
bizImpl := NewDeployControllerBizImpl(QueryNode, mockUtil, mockModeChanger, mockCli)
mc := v1beta1.Milvus{}
deploy := appsv1.Deployment{}
deploy2 := appsv1.Deployment{}
Expand Down Expand Up @@ -546,11 +532,10 @@ func TestDeployControllerBizImpl_HandleRolling(t *testing.T) {
func TestDeployControllerBizImpl_HandleManualMode(t *testing.T) {
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()
mockStatusSyncer := NewMockMilvusStatusSyncerInterface(mockCtrl)
mockUtil := NewMockDeployControllerBizUtil(mockCtrl)
mockCli := NewMockK8sClient(mockCtrl)
mockModeChanger := NewMockDeployModeChanger(mockCtrl)
bizImpl := NewDeployControllerBizImpl(QueryNode, mockStatusSyncer, mockUtil, mockModeChanger, mockCli)
bizImpl := NewDeployControllerBizImpl(QueryNode, mockUtil, mockModeChanger, mockCli)
mc := v1beta1.Milvus{}
mc.Spec.Mode = v1beta1.MilvusModeCluster
mc.Default()
Expand All @@ -567,6 +552,7 @@ func TestDeployControllerBizImpl_HandleManualMode(t *testing.T) {
})

deploy := &appsv1.Deployment{}
deploy.Spec.Replicas = int32Ptr(0)
t.Run("no rolling, renew deploy annotation, update requeue", func(t *testing.T) {
mockUtil.EXPECT().GetDeploys(ctx, mc).Return(deploy, nil, nil)
mockUtil.EXPECT().RenderPodTemplateWithoutGroupID(mc, gomock.Any(), QueryNode, true).Return(nil)
Expand All @@ -578,4 +564,11 @@ func TestDeployControllerBizImpl_HandleManualMode(t *testing.T) {
assert.Equal(t, ErrRequeue, err)
})

t.Run("active deploy has replica, no new rollout", func(t *testing.T) {
deploy.Spec.Replicas = int32Ptr(1)
mockUtil.EXPECT().GetDeploys(ctx, mc).Return(deploy, nil, nil)
err := bizImpl.HandleManualMode(ctx, mc)
assert.NoError(t, err)
})

}
2 changes: 1 addition & 1 deletion pkg/controllers/setup.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ func SetupControllers(ctx context.Context, mgr manager.Manager, stopReconcilers
k8sUtil := NewK8sUtil(mgr.GetClient())
bizUtilFactory := NewDeployControllerBizUtilFactory(mgr.GetClient(), k8sUtil)
modeChangerFactory := NewDeployModeChangerFactory(mgr.GetClient(), k8sUtil)
deployCtrlBizFactory := NewDeployControllerBizFactory(modeChangerFactory, statusSyncer, bizUtilFactory, mgr.GetClient())
deployCtrlBizFactory := NewDeployControllerBizFactory(modeChangerFactory, bizUtilFactory, mgr.GetClient())
rollingModeStatusUpdater := NewRollingModeStatusUpdater(mgr.GetClient(), deployCtrlBizFactory)
deployCtrl := NewDeployController(deployCtrlBizFactory, NewCommonComponentReconciler(reconciler), rollingModeStatusUpdater)
reconciler.deployCtrl = deployCtrl
Expand Down

0 comments on commit 4e34fe9

Please sign in to comment.