enhancement (CollaSet): fix review issues

apis/apps/v1alpha1/collaset_types.go

@@ -72,11 +72,11 @@ type CollaSetSpec struct {
 	// same Template, but individual replicas also have a consistent identity.
 	// If unspecified, defaults to 0.
 	// +optional
-	Replicas int32 `json:"replicas,omitempty"`
+	Replicas *int32 `json:"replicas,omitempty"`
 
 	// Selector is a label query over pods that should match the replica count.
 	// It must match the pod template's labels.
-	Selector metav1.LabelSelector `json:"selector,omitempty"`
+	Selector *metav1.LabelSelector `json:"selector,omitempty"`
 
 	// Template is the object that describes the pod that will be created if
 	// insufficient replicas are detected. Each pod stamped out by the CollaSet

pkg/controllers/collaset/collaset_controller.go

@@ -133,65 +133,51 @@ func (r *CollaSetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 		return ctrl.Result{}, nil
 	}
 
-	newStatus, err := DoReconcile(instance)
+	currentRevision, updatedRevision, revisions, collisionCount, _, err := revisionManager.ConstructRevisions(instance, false)
 	if err != nil {
-		return ctrl.Result{}, err
+		return ctrl.Result{}, fmt.Errorf("fail to construct revision for CollaSet %s/%s: %s", instance.Namespace, instance.Name, err)
+	}
+
+	newStatus := &appsv1alpha1.CollaSetStatus{
+		// record collisionCount
+		CollisionCount:  collisionCount,
+		CurrentRevision: currentRevision.Name,
+		UpdatedRevision: updatedRevision.Name,
 	}
 
+	newStatus, err = DoReconcile(instance, updatedRevision, revisions, newStatus)
+	// update status anyway
 	if err := r.updateStatus(ctx, instance, newStatus); err != nil {
 		return ctrl.Result{}, fmt.Errorf("fail to update status of CollaSet %s: %s", req, err)
 	}
 
-	return ctrl.Result{}, nil
+	return ctrl.Result{}, err
 }
 
-func DoReconcile(instance *appsv1alpha1.CollaSet) (*appsv1alpha1.CollaSetStatus, error) {
-	newStatus := instance.Status.DeepCopy()
-
-	currentRevision, updatedRevision, revisions, collisionCount, _, err := revisionManager.ConstructRevisions(instance, false)
-	if err != nil {
-		return newStatus, fmt.Errorf("fail to construct revision for CollaSet %s/%s: %s", instance.Namespace, instance.Name, err)
-	}
-
-	// record collisionCount
-	newStatus.CollisionCount = collisionCount
-	newStatus.CurrentRevision = currentRevision.Name
-	newStatus.UpdatedRevision = updatedRevision.Name
-
+func DoReconcile(instance *appsv1alpha1.CollaSet, updatedRevision *appsv1.ControllerRevision, revisions []*appsv1.ControllerRevision, newStatus *appsv1alpha1.CollaSetStatus) (*appsv1alpha1.CollaSetStatus, error) {
 	podWrappers, newStatus, syncErr := doSync(instance, updatedRevision, revisions, newStatus)
-
-	newStatus = calculateStatus(instance, newStatus, updatedRevision, podWrappers, syncErr)
-	if syncErr != nil {
-		return newStatus, syncErr
-	}
-
-	return newStatus, nil
+	return calculateStatus(instance, newStatus, updatedRevision, podWrappers, syncErr), syncErr
 }
 
 func doSync(instance *appsv1alpha1.CollaSet, updatedRevision *appsv1.ControllerRevision, revisions []*appsv1.ControllerRevision, newStatus *appsv1alpha1.CollaSetStatus) ([]*collasetutils.PodWrapper, *appsv1alpha1.CollaSetStatus, error) {
-	filteredPods, err := podControl.GetFilteredPods(&instance.Spec.Selector, instance)
+	filteredPods, err := podControl.GetFilteredPods(instance.Spec.Selector, instance)
 	if err != nil {
 		return nil, newStatus, err
 	}
 
 	synced, podWrappers, ownedIDs, err := syncControl.SyncPods(instance, filteredPods, updatedRevision, newStatus)
-	if err != nil {
+	if err != nil || synced {
 		return podWrappers, newStatus, err
-	} else if synced {
-		return podWrappers, newStatus, nil
 	}
 
-	if scaling, err := syncControl.Scale(instance, podWrappers, revisions, updatedRevision, ownedIDs, newStatus); err != nil {
+	scaling, err := syncControl.Scale(instance, podWrappers, revisions, updatedRevision, ownedIDs, newStatus)
+	if err != nil || scaling {
 		return podWrappers, newStatus, err
-	} else if scaling {
-		return podWrappers, newStatus, nil
 	}
 
-	if _, err := syncControl.Update(instance, podWrappers, revisions, updatedRevision, ownedIDs, newStatus); err != nil {
-		return podWrappers, newStatus, err
-	}
+	_, err = syncControl.Update(instance, podWrappers, revisions, updatedRevision, ownedIDs, newStatus)
 
-	return podWrappers, newStatus, nil
+	return podWrappers, newStatus, err
 }
 
 func calculateStatus(instance *appsv1alpha1.CollaSet, newStatus *appsv1alpha1.CollaSetStatus, updatedRevision *appsv1.ControllerRevision, podWrappers []*collasetutils.PodWrapper, syncErr error) *appsv1alpha1.CollaSetStatus {
@@ -242,7 +228,8 @@ func calculateStatus(instance *appsv1alpha1.CollaSet, newStatus *appsv1alpha1.Co
 	newStatus.UpdatedReadyReplicas = updatedReadyReplicas
 	newStatus.UpdatedAvailableReplicas = updatedAvailableReplicas
 
-	if newStatus.UpdatedAvailableReplicas == instance.Spec.Replicas {
+	if (instance.Spec.Replicas == nil && newStatus.UpdatedAvailableReplicas == 0) ||
+		*instance.Spec.Replicas == newStatus.UpdatedAvailableReplicas {
 		newStatus.CurrentRevision = updatedRevision.Name
 	}
 

pkg/controllers/collaset/collaset_controller_test.go

@@ -73,8 +73,8 @@ var _ = Describe("collaset controller", func() {
 				Name:      "foo",
 			},
 			Spec: appsv1alpha1.CollaSetSpec{
-				Replicas: 2,
-				Selector: metav1.LabelSelector{
+				Replicas: int32Pointer(2),
+				Selector: &metav1.LabelSelector{
 					MatchLabels: map[string]string{
 						"app": "foo",
 					},
@@ -152,7 +152,7 @@ var _ = Describe("collaset controller", func() {
 
 		// scale in pods
 		Expect(updateCollaSetWithRetry(c, cs.Namespace, cs.Name, func(cls *appsv1alpha1.CollaSet) bool {
-			cls.Spec.Replicas = 0
+			cls.Spec.Replicas = int32Pointer(0)
 			return true
 		})).Should(BeNil())
 
@@ -186,8 +186,8 @@ var _ = Describe("collaset controller", func() {
 				Name:      "foo",
 			},
 			Spec: appsv1alpha1.CollaSetSpec{
-				Replicas: 4,
-				Selector: metav1.LabelSelector{
+				Replicas: int32Pointer(4),
+				Selector: &metav1.LabelSelector{
 					MatchLabels: map[string]string{
 						"app": "foo",
 					},
@@ -375,8 +375,8 @@ var _ = Describe("collaset controller", func() {
 				Name:      "foo",
 			},
 			Spec: appsv1alpha1.CollaSetSpec{
-				Replicas: 1,
-				Selector: metav1.LabelSelector{
+				Replicas: int32Pointer(1),
+				Selector: &metav1.LabelSelector{
 					MatchLabels: map[string]string{
 						"app": "foo",
 					},
@@ -497,8 +497,8 @@ var _ = Describe("collaset controller", func() {
 				Name:      "foo",
 			},
 			Spec: appsv1alpha1.CollaSetSpec{
-				Replicas: 2,
-				Selector: metav1.LabelSelector{
+				Replicas: int32Pointer(2),
+				Selector: &metav1.LabelSelector{
 					MatchLabels: map[string]string{
 						"app": "foo",
 					},
@@ -773,3 +773,7 @@ func createNamespace(c client.Client, namespaceName string) error {
 
 	return c.Create(context.TODO(), ns)
 }
+
+func int32Pointer(val int32) *int32 {
+	return &val
+}

pkg/controllers/collaset/revision.go

@@ -57,7 +57,7 @@ type revisionOwnerAdapter struct {
 
 func (roa *revisionOwnerAdapter) GetSelector(obj metav1.Object) *metav1.LabelSelector {
 	ips, _ := obj.(*appsalphav1.CollaSet)
-	return &ips.Spec.Selector
+	return ips.Spec.Selector
 }
 
 func (roa *revisionOwnerAdapter) GetCollisionCount(obj metav1.Object) *int32 {
@@ -75,7 +75,7 @@ func (roa *revisionOwnerAdapter) GetPatch(obj metav1.Object) ([]byte, error) {
 	return getCollaSetPatch(cs)
 }
 
-func (roa *revisionOwnerAdapter) GetOwneeLabels(obj metav1.Object) map[string]string {
+func (roa *revisionOwnerAdapter) GetSelectorLabels(obj metav1.Object) map[string]string {
 	ips, _ := obj.(*appsalphav1.CollaSet)
 	labels := map[string]string{}
 	for k, v := range ips.Spec.Template.Labels {

pkg/controllers/collaset/synccontrol/sync_control.go

@@ -66,7 +66,7 @@ func (sc *RealSyncControl) SyncPods(instance *appsv1alpha1.CollaSet, filteredPod
 	var ownedIDs map[int]*appsv1alpha1.ContextDetail
 	var err error
 	if err := retry.RetryOnConflict(retry.DefaultBackoff, func() error {
-		ownedIDs, err = podcontext.AllocateID(sc.client, instance, updatedRevision.Name, int(instance.Spec.Replicas))
+		ownedIDs, err = podcontext.AllocateID(sc.client, instance, updatedRevision.Name, int(replicasRealValue(instance.Spec.Replicas)))
 		return err
 	}); err != nil {
 		return false, nil, ownedIDs, fmt.Errorf("fail to allocate %d IDs using context when sync Pods: %s", instance.Spec.Replicas, err)
@@ -131,7 +131,7 @@ func (sc *RealSyncControl) SyncPods(instance *appsv1alpha1.CollaSet, filteredPod
 }
 
 func (sc *RealSyncControl) Scale(set *appsv1alpha1.CollaSet, podWrappers []*collasetutils.PodWrapper, revisions []*appsv1.ControllerRevision, updatedRevision *appsv1.ControllerRevision, ownedIDs map[int]*appsv1alpha1.ContextDetail, newStatus *appsv1alpha1.CollaSetStatus) (scaled bool, err error) {
-	diff := int(set.Spec.Replicas) - len(podWrappers)
+	diff := int(replicasRealValue(set.Spec.Replicas)) - len(podWrappers)
 	scaling := false
 
 	if diff > 0 {
@@ -459,3 +459,11 @@ func (sc *RealSyncControl) Update(instance *appsv1alpha1.CollaSet, podWrapers []
 
 	return updating || succCount > 0, err
 }
+
+func replicasRealValue(replcias *int32) int32 {
+	if replcias == nil {
+		return 0
+	}
+
+	return *replcias
+}

pkg/controllers/resourcecontext/resourcecontext_controller_test.go

@@ -73,8 +73,8 @@ var _ = Describe("ResourceContext controller", func() {
 				Name:      "foo",
 			},
 			Spec: appsv1alpha1.CollaSetSpec{
-				Replicas: 2,
-				Selector: metav1.LabelSelector{
+				Replicas: int32Pointer(2),
+				Selector: &metav1.LabelSelector{
 					MatchLabels: map[string]string{
 						"app": "foo",
 					},
@@ -150,7 +150,7 @@ var _ = Describe("ResourceContext controller", func() {
 		Expect(c.Get(context.TODO(), types.NamespacedName{Namespace: cs.Namespace, Name: cs.Name}, resourceContext)).Should(BeNil())
 
 		Expect(updateCollaSetWithRetry(c, cs.Namespace, cs.Name, func(cls *appsv1alpha1.CollaSet) bool {
-			cls.Spec.Replicas = 0
+			cls.Spec.Replicas = int32Pointer(0)
 			return true
 		})).Should(BeNil())
 
@@ -346,3 +346,7 @@ func createNamespace(c client.Client, namespaceName string) error {
 
 	return c.Create(context.TODO(), ns)
 }
+
+func int32Pointer(val int32) *int32 {
+	return &val
+}

pkg/controllers/utils/refmanager/ref_manager_test.go

@@ -45,8 +45,8 @@ func Test(t *testing.T) {
 			UID:       types.UID("fake-uid"),
 		},
 		Spec: appsv1alpha1.CollaSetSpec{
-			Replicas: 2,
-			Selector: metav1.LabelSelector{
+			Replicas: int32Pointer(2),
+			Selector: &metav1.LabelSelector{
 				MatchLabels: map[string]string{
 					"app":  "foo",
 					"case": testcase,
@@ -92,7 +92,7 @@ func Test(t *testing.T) {
 		},
 	}
 
-	ref, err := NewRefManager(&MockClient{}, &cs.Spec.Selector, cs, scheme)
+	ref, err := NewRefManager(&MockClient{}, cs.Spec.Selector, cs, scheme)
 	g.Expect(err).Should(gomega.BeNil())
 
 	podObjects := make([]client.Object, len(pods))
@@ -142,3 +142,7 @@ func (c *MockClient) Patch(ctx context.Context, obj client.Object, patch client.
 func (c *MockClient) DeleteAllOf(ctx context.Context, obj client.Object, opts ...client.DeleteAllOfOption) error {
 	return nil
 }
+
+func int32Pointer(val int32) *int32 {
+	return &val
+}

pkg/controllers/utils/revision/revision_manager.go

@@ -46,7 +46,7 @@ type OwnerAdapter interface {
 	GetCollisionCount(obj metav1.Object) *int32
 	GetHistoryLimit(obj metav1.Object) int32
 	GetPatch(obj metav1.Object) ([]byte, error)
-	GetOwneeLabels(obj metav1.Object) map[string]string
+	GetSelectorLabels(obj metav1.Object) map[string]string
 	GetCurrentRevision(obj metav1.Object) string
 	IsInUsed(obj metav1.Object, controllerRevision string) bool
 }
@@ -289,7 +289,7 @@ func (rm *RevisionManager) newRevision(set metav1.Object, revision int64, collis
 		return nil, err
 	}
 
-	revisionLabels := rm.ownerGetter.GetOwneeLabels(set)
+	revisionLabels := rm.ownerGetter.GetSelectorLabels(set)
 	if revisionLabels == nil {
 		revisionLabels = map[string]string{}
 	}