add unit test & split workload mutating webhook

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

config/webhook/manifests.yaml

@@ -65,48 +65,6 @@ webhooks:
     resources:
     - deployments
   sideEffects: None
-- admissionReviewVersions:
-  - v1
-  - v1beta1
-  clientConfig:
-    service:
-      name: webhook-service
-      namespace: system
-      path: /mutate-apps-v1-statefulset
-  failurePolicy: Fail
-  name: mstatefulset.kb.io
-  rules:
-  - apiGroups:
-    - apps
-    apiVersions:
-    - v1
-    operations:
-    - UPDATE
-    resources:
-    - statefulsets
-  sideEffects: None
-- admissionReviewVersions:
-  - v1
-  - v1beta1
-  clientConfig:
-    service:
-      name: webhook-service
-      namespace: system
-      path: /mutate-apps-kruise-io-statefulset
-  failurePolicy: Fail
-  name: madvancedstatefulset.kb.io
-  rules:
-  - apiGroups:
-    - apps.kruise.io
-    apiVersions:
-    - v1alpha1
-    - v1beta1
-    operations:
-    - CREATE
-    - UPDATE
-    resources:
-    - statefulsets
-  sideEffects: None
 - admissionReviewVersions:
   - v1
   - v1beta1

config/webhook/patch_manifests.yaml

@@ -18,16 +18,16 @@ webhooks:
       matchExpressions:
         - key: rollouts.kruise.io/workload-type
           operator: Exists
-  - name: mstatefulset.kb.io
-    objectSelector:
-      matchExpressions:
-        - key: rollouts.kruise.io/workload-type
-          operator: Exists
-  - name: madvancedstatefulset.kb.io
-    objectSelector:
-      matchExpressions:
-        - key: rollouts.kruise.io/workload-type
-          operator: Exists
+  # - name: mstatefulset.kb.io
+  #   objectSelector:
+  #     matchExpressions:
+  #       - key: rollouts.kruise.io/workload-type
+  #         operator: Exists
+  # - name: madvancedstatefulset.kb.io
+  #   objectSelector:
+  #     matchExpressions:
+  #       - key: rollouts.kruise.io/workload-type
+  #         operator: Exists
   - name: mdeployment.kb.io
     objectSelector:
       matchExpressions:

pkg/controller/batchrelease/batchrelease_executor.go

@@ -151,7 +151,7 @@ func (r *Executor) progressBatches(release *v1beta1.BatchRelease, newStatus *v1b
 			result = reconcile.Result{RequeueAfter: DefaultDuration}
 			removeProgressingCondition(newStatus)
 			newStatus.CanaryStatus.CurrentBatchState = v1beta1.VerifyingBatchState
-		case errors.IsFatal(err):
+		case errors.IsBadRequest(err):
 			progressingStateTransition(newStatus, v1.ConditionTrue, v1beta1.ProgressingReasonInRolling, err.Error())
 			fallthrough
 		default:

pkg/controller/batchrelease/control/bluegreenstyle/cloneset/control.go

@@ -94,7 +94,7 @@ func (rc *realController) Initialize(release *v1beta1.BatchRelease) error {
 	// patch the cloneset
 	setting, err := control.GetOriginalSetting(rc.object)
 	if err != nil {
-		return errors.NewFatalError(fmt.Errorf("cannot get original setting for cloneset %v: %s from annotation", klog.KObj(rc.object), err.Error()))
+		return errors.NewBadRequestError(fmt.Errorf("cannot get original setting for cloneset %v: %s from annotation", klog.KObj(rc.object), err.Error()))
 	}
 	control.InitOriginalSetting(&setting, rc.object)
 	patchData := patch.NewClonesetPatch()
@@ -115,7 +115,7 @@ func (rc *realController) Initialize(release *v1beta1.BatchRelease) error {
 
 func (rc *realController) UpgradeBatch(ctx *batchcontext.BatchContext) error {
 	if err := control.ValidateReadyForBlueGreenRelease(rc.object); err != nil {
-		return errors.NewFatalError(fmt.Errorf("cannot upgrade batch, because cloneset %v doesn't satisfy conditions: %s", klog.KObj(rc.object), err.Error()))
+		return errors.NewBadRequestError(fmt.Errorf("cannot upgrade batch, because cloneset %v doesn't satisfy conditions: %s", klog.KObj(rc.object), err.Error()))
 	}
 	desired, _ := intstr.GetScaledValueFromIntOrPercent(&ctx.DesiredSurge, int(ctx.Replicas), true)
 	current, _ := intstr.GetScaledValueFromIntOrPercent(&ctx.CurrentSurge, int(ctx.Replicas), true)
@@ -133,10 +133,6 @@ func (rc *realController) UpgradeBatch(ctx *batchcontext.BatchContext) error {
 }
 
 func (rc *realController) Finalize(release *v1beta1.BatchRelease) error {
-	if rc.finalized() {
-		return nil // No need to finalize again
-	}
-
 	if release.Spec.ReleasePlan.BatchPartition != nil {
 		// continuous release (not supported yet)
 		/*
@@ -148,39 +144,37 @@ func (rc *realController) Finalize(release *v1beta1.BatchRelease) error {
 		return nil
 	}
 
-	c := util.GetEmptyObjectWithKey(rc.object)
-	setting, err := control.GetOriginalSetting(rc.object)
-	if err != nil {
-		return errors.NewFatalError(fmt.Errorf("cannot get original setting for cloneset %v: %s from annotation", klog.KObj(rc.object), err.Error()))
-	}
-	patchData := patch.NewClonesetPatch()
-	if rc.object.Spec.MinReadySeconds != setting.MinReadySeconds {
-		// restore the hpa
-		if err := hpa.RestoreHPA(rc.client, rc.object); err != nil {
+	// restore the original setting and remove annotation
+	if !rc.restored() {
+		c := util.GetEmptyObjectWithKey(rc.object)
+		setting, err := control.GetOriginalSetting(rc.object)
+		if err != nil {
 			return err
 		}
-		// restore the original setting
+		patchData := patch.NewClonesetPatch()
 		patchData.UpdateMinReadySeconds(setting.MinReadySeconds)
 		patchData.UpdateMaxSurge(setting.MaxSurge)
 		patchData.UpdateMaxUnavailable(setting.MaxUnavailable)
+		patchData.DeleteAnnotation(v1beta1.OriginalDeploymentStrategyAnnotation)
+		patchData.DeleteAnnotation(util.BatchReleaseControlAnnotation)
 		if err := rc.client.Patch(context.TODO(), c, patchData); err != nil {
 			return err
 		}
+		klog.InfoS("Finalize: cloneset bluegreen release: wait all pods updated and ready", "cloneset", klog.KObj(rc.object))
 	}
-	klog.InfoS("Finalize: cloneset bluegreen release: wait all pods updated and ready", "cloneset", klog.KObj(rc.object))
+
 	// wait all pods updated and ready
 	if rc.object.Status.ReadyReplicas != rc.object.Status.UpdatedReadyReplicas {
-		return errors.NewBenignError(fmt.Errorf("cloneset %v finalize not done, readyReplicas %d != updatedReadyReplicas %d, current policy %s",
+		return errors.NewRetryError(fmt.Errorf("cloneset %v finalize not done, readyReplicas %d != updatedReadyReplicas %d, current policy %s",
 			klog.KObj(rc.object), rc.object.Status.ReadyReplicas, rc.object.Status.UpdatedReadyReplicas, release.Spec.ReleasePlan.FinalizingPolicy))
 	}
 	klog.InfoS("Finalize: cloneset bluegreen release: all pods updated and ready")
-	// restore annotation
-	patchData.DeleteAnnotation(v1beta1.OriginalDeploymentStrategyAnnotation)
-	patchData.DeleteAnnotation(util.BatchReleaseControlAnnotation)
-	return rc.client.Patch(context.TODO(), c, patchData)
+
+	// restore the hpa
+	return hpa.RestoreHPA(rc.client, rc.object)
 }
 
-func (rc *realController) finalized() bool {
+func (rc *realController) restored() bool {
 	if rc.object == nil || rc.object.DeletionTimestamp != nil {
 		return true
 	}

pkg/controller/batchrelease/control/bluegreenstyle/cloneset/control_test.go

@@ -31,13 +31,15 @@ import (
 	control "github.com/openkruise/rollouts/pkg/controller/batchrelease/control"
 	"github.com/openkruise/rollouts/pkg/util"
 	apps "k8s.io/api/apps/v1"
+	autoscalingv1 "k8s.io/api/autoscaling/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/uuid"
 	"k8s.io/utils/pointer"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
 
@@ -101,7 +103,7 @@ var (
 			UpdateRevision:       "version-2",
 			CurrentRevision:      "version-1",
 			ObservedGeneration:   1,
-			CollisionCount:       pointer.Int32Ptr(1),
+			CollisionCount:       pointer.Int32(1),
 		},
 	}
 
@@ -141,14 +143,157 @@ var (
 			},
 		},
 	}
+	hpaDemo = &autoscalingv1.HorizontalPodAutoscaler{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: "autoscaling/v1",
+			Kind:       "HorizontalPodAutoscaler",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "hpa",
+			Namespace: cloneKey.Namespace,
+		},
+		Spec: autoscalingv1.HorizontalPodAutoscalerSpec{
+			ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
+				APIVersion: "apps.kruise.io/v1alpha1",
+				Kind:       "CloneSet",
+				Name:       cloneDemo.Name,
+			},
+			MinReplicas: pointer.Int32(1),
+			MaxReplicas: 10,
+		},
+	}
 )
 
 func init() {
 	apps.AddToScheme(scheme)
 	rolloutapi.AddToScheme(scheme)
 	kruiseappsv1alpha1.AddToScheme(scheme)
+	autoscalingv1.AddToScheme(scheme)
+}
+
+func TestControlPackage(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "CloneSet Control Package Suite")
 }
 
+var _ = Describe("CloneSet Control", func() {
+	var (
+		c        client.Client
+		rc       *realController
+		cloneset *kruiseappsv1alpha1.CloneSet
+		release  *v1beta1.BatchRelease
+		hpa      *autoscalingv1.HorizontalPodAutoscaler
+	)
+
+	BeforeEach(func() {
+		cloneset = cloneDemo.DeepCopy()
+		release = releaseDemo.DeepCopy()
+		hpa = hpaDemo.DeepCopy()
+		c = fake.NewClientBuilder().
+			WithScheme(scheme).
+			WithObjects(cloneset, release, hpa).
+			Build()
+		rc = &realController{
+			key:    types.NamespacedName{Namespace: cloneset.Namespace, Name: cloneset.Name},
+			client: c,
+		}
+	})
+
+	It("should initialize cloneset successfully", func() {
+		// build controller
+		_, err := rc.BuildController()
+		Expect(err).NotTo(HaveOccurred())
+		// call Initialize method
+		err = retryFunction(3, func() error {
+			return rc.Initialize(release)
+		})
+		Expect(err).NotTo(HaveOccurred())
+		// inspect if HPA is disabled
+		disabledHPA := &autoscalingv1.HorizontalPodAutoscaler{}
+		err = c.Get(context.TODO(), types.NamespacedName{Namespace: hpa.Namespace, Name: hpa.Name}, disabledHPA)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(disabledHPA.Spec.ScaleTargetRef.Name).To(Equal(cloneset.Name + "-DisableByRollout"))
+
+		// inspect if Cloneset is patched properly
+		updatedCloneset := &kruiseappsv1alpha1.CloneSet{}
+		err = c.Get(context.TODO(), client.ObjectKeyFromObject(cloneset), updatedCloneset)
+		Expect(err).NotTo(HaveOccurred())
+
+		// inspect if annotations are added
+		Expect(updatedCloneset.Annotations).To(HaveKey(v1beta1.OriginalDeploymentStrategyAnnotation))
+		Expect(updatedCloneset.Annotations).To(HaveKey(util.BatchReleaseControlAnnotation))
+		Expect(updatedCloneset.Annotations[util.BatchReleaseControlAnnotation]).To(Equal(getControlInfo(release)))
+
+		// inspect if strategy is updated
+		Expect(updatedCloneset.Spec.UpdateStrategy.Paused).To(BeFalse())
+		Expect(updatedCloneset.Spec.UpdateStrategy.MaxSurge.IntVal).To(Equal(int32(1)))
+		Expect(updatedCloneset.Spec.UpdateStrategy.MaxUnavailable.IntVal).To(Equal(int32(0)))
+		Expect(updatedCloneset.Spec.MinReadySeconds).To(Equal(int32(v1beta1.MaxReadySeconds)))
+	})
+
+	It("should finalize CloneSet successfully", func() {
+		// hack to patch cloneset status
+		cloneset.Status.UpdatedReadyReplicas = 10
+		err := c.Status().Update(context.TODO(), cloneset)
+		Expect(err).NotTo(HaveOccurred())
+		// build controller
+		rc.object = nil
+		_, err = rc.BuildController()
+		Expect(err).NotTo(HaveOccurred())
+		// call Finalize method
+		err = retryFunction(3, func() error {
+			return rc.Finalize(release)
+		})
+		Expect(err).NotTo(HaveOccurred())
+
+		// inspect if CloneSet is patched properly
+		updatedCloneset := &kruiseappsv1alpha1.CloneSet{}
+		err = c.Get(context.TODO(), client.ObjectKeyFromObject(cloneset), updatedCloneset)
+		Expect(err).NotTo(HaveOccurred())
+
+		// inspect if annotations are removed
+		Expect(updatedCloneset.Annotations).NotTo(HaveKey(v1beta1.OriginalDeploymentStrategyAnnotation))
+		Expect(updatedCloneset.Annotations).NotTo(HaveKey(util.BatchReleaseControlAnnotation))
+
+		// inspect if strategy is restored
+		Expect(updatedCloneset.Spec.UpdateStrategy.MaxSurge).To(BeNil())
+		Expect(*updatedCloneset.Spec.UpdateStrategy.MaxUnavailable).To(Equal(intstr.IntOrString{Type: intstr.Int, IntVal: 1}))
+		Expect(updatedCloneset.Spec.MinReadySeconds).To(Equal(int32(0)))
+
+		// inspect if HPA is restored
+		restoredHPA := &autoscalingv1.HorizontalPodAutoscaler{}
+		err = c.Get(context.TODO(), types.NamespacedName{Namespace: hpa.Namespace, Name: hpa.Name}, restoredHPA)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(restoredHPA.Spec.ScaleTargetRef.Name).To(Equal(cloneset.Name))
+	})
+
+	It("should upgradBatch for CloneSet successfully", func() {
+		// call Initialize method
+		_, err := rc.BuildController()
+		Expect(err).NotTo(HaveOccurred())
+		err = retryFunction(3, func() error {
+			return rc.Initialize(release)
+		})
+		Expect(err).NotTo(HaveOccurred())
+
+		// call UpgradeBatch method
+		rc.object = nil
+		_, err = rc.BuildController()
+		Expect(err).NotTo(HaveOccurred())
+		batchContext, err := rc.CalculateBatchContext(release)
+		Expect(err).NotTo(HaveOccurred())
+		err = rc.UpgradeBatch(batchContext)
+		Expect(err).NotTo(HaveOccurred())
+
+		// inspect if CloneSet is patched properly
+		updatedCloneset := &kruiseappsv1alpha1.CloneSet{}
+		err = c.Get(context.TODO(), client.ObjectKeyFromObject(cloneset), updatedCloneset)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(*updatedCloneset.Spec.UpdateStrategy.MaxSurge).To(Equal(intstr.IntOrString{Type: intstr.String, StrVal: "10%"}))
+		Expect(*updatedCloneset.Spec.UpdateStrategy.MaxUnavailable).To(Equal(intstr.IntOrString{Type: intstr.Int, IntVal: 0}))
+	})
+})
+
 func TestCalculateBatchContext(t *testing.T) {
 	RegisterFailHandler(Fail)
 	cases := map[string]struct {
@@ -392,3 +537,12 @@ func getControlInfo(release *v1beta1.BatchRelease) string {
 	owner, _ := json.Marshal(metav1.NewControllerRef(release, release.GetObjectKind().GroupVersionKind()))
 	return string(owner)
 }
+
+func retryFunction(limit int, f func() error) (err error) {
+	for i := limit; i >= 0; i-- {
+		if err = f(); err == nil {
+			return nil
+		}
+	}
+	return err
+}