removed status changes, cleaned up tests that referenced them. Implem…

…ented TruncateHistory and fixed getPatch

api/leaderworkerset/v1/leaderworkerset_types.go

@@ -303,20 +303,6 @@ type LeaderWorkerSetStatus struct {
 	// needed for HPA to know what pods belong to the LeaderWorkerSet object. Here
 	// we only select the leader pods.
 	HPAPodSelector string `json:"hpaPodSelector,omitempty"`
-
-	// currentRevision, if not empty, indicates the version of the  worker StatefulSet
-	// used to generate the worker pods in sequence [0,currentReplicas)
-	CurrentRevision string `json:"currentRevision,omitempty"`
-
-	// updateRevision, if not empty, indicates the version of the  worker StatefulSet
-	// used to generate the worker pods in sequence [replicas-updatedReplicas,replicas)
-	UpdateRevision string `json:"updateRevision,omitempty"`
-
-	// collisionCount is the count of hash collisions for lws. The lws controller
-	// uses this field as a collision avoidance mechanism when it needs to create the name for the
-	// newest ControllerRevision.
-	// +optional
-	CollisionCount *int32 `json:"collisionCount,omitempty"`
 }
 
 type LeaderWorkerSetConditionType string

config/crd/bases/leaderworkerset.x-k8s.io_leaderworkersets.yaml

@@ -16204,13 +16204,6 @@ spec:
           status:
             description: LeaderWorkerSetStatus defines the observed state of LeaderWorkerSet
             properties:
-              collisionCount:
-                description: |-
-                  collisionCount is the count of hash collisions for lws. The lws controller
-                  uses this field as a collision avoidance mechanism when it needs to create the name for the
-                  newest ControllerRevision.
-                format: int32
-                type: integer
               conditions:
                 description: Conditions track the condition of the leaderworkerset.
                 items:
@@ -16268,11 +16261,6 @@ spec:
                   - type
                   type: object
                 type: array
-              currentRevision:
-                description: |-
-                  currentRevision, if not empty, indicates the version of the  worker StatefulSet
-                  used to generate the worker pods in sequence [0,currentReplicas)
-                type: string
               hpaPodSelector:
                 description: |-
                   HPAPodSelector for pods that belong to the LeaderWorkerSet object, this is
@@ -16289,11 +16277,6 @@ spec:
                   created (updated or not, ready or not)
                 format: int32
                 type: integer
-              updateRevision:
-                description: |-
-                  updateRevision, if not empty, indicates the version of the  worker StatefulSet
-                  used to generate the worker pods in sequence [replicas-updatedReplicas,replicas)
-                type: string
               updatedReplicas:
                 description: UpdatedReplicas track the number of groups that have
                   been updated (ready or not).

pkg/controllers/leaderworkerset_controller.go

@@ -198,6 +198,9 @@ func (r *LeaderWorkerSetReconciler) rollingUpdateParameters(ctx context.Context,
 	ctx = ctrl.LoggerInto(ctx, log)
 	lwsReplicas := *lws.Spec.Replicas
 
+	// Case 1:
+	// If sts not created yet, all partitions should be updated,
+	// replicas should not change.
 	stsExists, sts, err := stsCreated(ctx, r.Client, lws)
 	if err != nil {
 		return 0, 0, err
@@ -211,6 +214,7 @@ func (r *LeaderWorkerSetReconciler) rollingUpdateParameters(ctx context.Context,
 	if err != nil {
 		return 0, 0, err
 	}
+
 	if !existingControllerRevisions {
 		// Updating from version that did not support Controller Revision. Need to create one first before checking if template has been updated
 		log.V(2).Info(fmt.Sprintf("Creating new controller revision create/update operation for %+v ", lws))
@@ -430,6 +434,7 @@ func (r *LeaderWorkerSetReconciler) updateConditions(ctx context.Context, lws *l
 		conditions = append(conditions, makeCondition(leaderworkerset.LeaderWorkerSetUpgradeInProgress))
 	} else if updatedAndReadyCount == int(*lws.Spec.Replicas) {
 		conditions = append(conditions, makeCondition(leaderworkerset.LeaderWorkerSetAvailable))
+		controllerutils.TruncateHistory(ctx, r.Client, lws, templateHash)
 	} else {
 		conditions = append(conditions, makeCondition(leaderworkerset.LeaderWorkerSetProgressing))
 	}
@@ -729,18 +734,13 @@ func templateUpdated(ctx context.Context, k8sClient client.Client, sts *appsv1.S
 	if err != nil {
 		return false, err
 	}
-	log.V(2).Info(fmt.Sprintf("comparing networkConfig %s, with %s", string(*lws.Spec.NetworkConfig.SubdomainPolicy), string(*baselineLws.Spec.NetworkConfig.SubdomainPolicy)))
-	log.V(2).Info(fmt.Sprintf("Fetching controller revision with hash %s", sts.Labels[leaderworkerset.TemplateRevisionHashKey]))
 	return !utils.EqualLeaderWorkerTemplates(baselineLws, lws), nil
 }
 
 func stsCreated(ctx context.Context, k8sClient client.Client, lws *leaderworkerset.LeaderWorkerSet) (bool, *appsv1.StatefulSet, error) {
 	sts := &appsv1.StatefulSet{}
 	err := k8sClient.Get(ctx, types.NamespacedName{Name: lws.Name, Namespace: lws.Namespace}, sts)
 	if err != nil {
-		// Case 1:
-		// If sts not created yet, all partitions should be updated,
-		// replicas should not change.
 		if apierrors.IsNotFound(err) {
 			return false, nil, nil
 		}

pkg/controllers/pod_controller_test.go

@@ -37,9 +37,12 @@ func TestConstructWorkerStatefulSetApplyConfiguration(t *testing.T) {
 	parentKind := appsv1.SchemeGroupVersion.WithKind("LeaderWorkerSet")
 	lws := testutils.BuildBasicLeaderWorkerSet("test-sample", "default").Replica(1).WorkerTemplateSpec(testutils.MakeWorkerPodSpec()).Size(1).Obj()
 	updateTemplateHash := utils.LeaderWorkerTemplateHash(lws)
+	updateRevision, err := history.NewControllerRevision(lws, parentKind, lws.Labels, testutils.RawLWSTemplate(lws), 1)
+	if err != nil {
+		t.Fatal(err)
+	}
 	lws.Spec.LeaderWorkerTemplate.WorkerTemplate.Spec.Containers[0].Name = "worker"
-	lws.Status.CollisionCount = new(int32)
-	currentRevision, err := history.NewControllerRevision(lws, parentKind, lws.Labels, testutils.RawLWSTemplate(lws), 1, lws.Status.CollisionCount)
+	currentRevision, err := history.NewControllerRevision(lws, parentKind, lws.Labels, testutils.RawLWSTemplate(lws), 2)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -54,7 +57,7 @@ func TestConstructWorkerStatefulSetApplyConfiguration(t *testing.T) {
 	}{
 		{
 			name:     "1 replica, size 1, exclusive placement disabled",
-			revision: currentRevision,
+			revision: updateRevision,
 			pod: &corev1.Pod{
 				ObjectMeta: v1.ObjectMeta{
 					Name:      "test-sample",
@@ -128,7 +131,7 @@ func TestConstructWorkerStatefulSetApplyConfiguration(t *testing.T) {
 		},
 		{
 			name:     "1 replica, size 2, exclusive placement enabled",
-			revision: currentRevision,
+			revision: updateRevision,
 			pod: &corev1.Pod{
 				ObjectMeta: v1.ObjectMeta{
 					Name:      "test-sample",
@@ -205,7 +208,7 @@ func TestConstructWorkerStatefulSetApplyConfiguration(t *testing.T) {
 		},
 		{
 			name:     "1 replica, size 2, subgroupsize 2, exclusive placement enabled",
-			revision: currentRevision,
+			revision: updateRevision,
 			pod: &corev1.Pod{
 				ObjectMeta: v1.ObjectMeta{
 					Name:      "test-sample",
@@ -282,7 +285,7 @@ func TestConstructWorkerStatefulSetApplyConfiguration(t *testing.T) {
 			},
 		},
 		{
-			name:     "LeaderPod has a different template hash than one generated by lws object, use podTemplateSpec from revision",
+			name:     "revision is before update, will use that and the old templateHash to create the worker statefulset configuration",
 			revision: currentRevision,
 			pod: &corev1.Pod{
 				ObjectMeta: v1.ObjectMeta{

pkg/history/controller_history.go

@@ -58,15 +58,13 @@ func ControllerRevisionName(prefix string, hash string) string {
 
 // NewControllerRevision returns a ControllerRevision with a ControllerRef pointing to parent and indicating that
 // parent is of parentKind. The ControllerRevision has labels matching template labels, contains Data equal to data, and
-// has a Revision equal to revision. The collisionCount is used when creating the name of the ControllerRevision
-// so the name is likely unique. If the returned error is nil, the returned ControllerRevision is valid. If the
+// has a Revision equal to revision. If the returned error is nil, the returned ControllerRevision is valid. If the
 // returned error is not nil, the returned ControllerRevision is invalid for use.
 func NewControllerRevision(parent metav1.Object,
 	parentKind schema.GroupVersionKind,
 	templateLabels map[string]string,
 	data runtime.RawExtension,
-	revision int64,
-	collisionCount *int32) (*appsv1.ControllerRevision, error) {
+	revision int64) (*appsv1.ControllerRevision, error) {
 	labelMap := make(map[string]string)
 	for k, v := range templateLabels {
 		labelMap[k] = v
@@ -80,25 +78,22 @@ func NewControllerRevision(parent metav1.Object,
 		Data:     data,
 		Revision: revision,
 	}
-	hash := HashControllerRevision(cr, collisionCount)
+	hash := HashControllerRevision(cr)
 	cr.Name = ControllerRevisionName(parent.GetName(), hash)
 	cr.Labels[ControllerRevisionHashLabel] = hash
 	return cr, nil
 }
 
-// HashControllerRevision hashes the contents of revision's Data using FNV hashing. If probe is not nil, the byte value
-// of probe is added written to the hash as well. The returned hash will be a safe encoded string to avoid bad words.
-func HashControllerRevision(revision *appsv1.ControllerRevision, probe *int32) string {
+// HashControllerRevision hashes the contents of revision's Data using FNV hashing.
+// The returned hash will be a safe encoded string to avoid bad words.
+func HashControllerRevision(revision *appsv1.ControllerRevision) string {
 	hf := fnv.New32()
 	if len(revision.Data.Raw) > 0 {
 		hf.Write(revision.Data.Raw)
 	}
 	if revision.Data.Object != nil {
 		DeepHashObject(hf, revision.Data.Object)
 	}
-	if probe != nil {
-		hf.Write([]byte(strconv.FormatInt(int64(*probe), 10)))
-	}
 	return rand.SafeEncodeString(fmt.Sprint(hf.Sum32()))
 }
 
@@ -181,14 +176,11 @@ type Interface interface {
 	// controller. If the returned error is nil the returned slice of ControllerRevisions is valid. If the
 	// returned error is not nil, the returned slice is not valid.
 	ListControllerRevisions(parent metav1.Object, selector labels.Selector) ([]*appsv1.ControllerRevision, error)
-	// CreateControllerRevision attempts to create the revision as owned by parent via a ControllerRef. If name
-	// collision occurs, collisionCount (incremented each time collision occurs except for the first time) is
-	// added to the hash of the revision and it is renamed using ControllerRevisionName. Implementations may
+	// CreateControllerRevision attempts to create the revision as owned by parent via a ControllerRef. Implementations may
 	// cease to attempt to retry creation after some number of attempts and return an error. If the returned
 	// error is not nil, creation failed. If the returned error is nil, the returned ControllerRevision has been
 	// created.
-	// Callers must make sure that collisionCount is not nil. An error is returned if it is.
-	CreateControllerRevision(parent metav1.Object, revision *appsv1.ControllerRevision, collisionCount *int32) (*appsv1.ControllerRevision, error)
+	CreateControllerRevision(parent metav1.Object, revision *appsv1.ControllerRevision) (*appsv1.ControllerRevision, error)
 	// DeleteControllerRevision attempts to delete revision. If the returned error is not nil, deletion has failed.
 	DeleteControllerRevision(revision *appsv1.ControllerRevision) error
 	// UpdateControllerRevision updates revision such that its Revision is equal to newRevision. Implementations
@@ -236,35 +228,30 @@ func (rh *realHistory) ListControllerRevisions(parent metav1.Object, selector la
 	return owned, err
 }
 
-func (rh *realHistory) CreateControllerRevision(parent metav1.Object, revision *appsv1.ControllerRevision, collisionCount *int32) (*appsv1.ControllerRevision, error) {
-	if collisionCount == nil {
-		return nil, fmt.Errorf("collisionCount should not be nil")
-	}
-
+func (rh *realHistory) CreateControllerRevision(parent metav1.Object, revision *appsv1.ControllerRevision) (*appsv1.ControllerRevision, error) {
 	// Clone the input
 	clone := revision.DeepCopy()
 
-	// Continue to attempt to create the revision updating the name with a new hash on each iteration
-	for {
-		hash := HashControllerRevision(revision, collisionCount)
-		// Update the revisions name
-		clone.Name = ControllerRevisionName(parent.GetName(), hash)
-		ns := parent.GetNamespace()
-		err := rh.Create(rh.context, clone)
-		if errors.IsAlreadyExists(err) {
-			exists := &appsv1.ControllerRevision{}
-			err := rh.Get(rh.context, types.NamespacedName{Namespace: ns, Name: clone.Name}, exists)
-			if err != nil {
-				return nil, err
-			}
-			if bytes.Equal(exists.Data.Raw, clone.Data.Raw) {
-				return exists, nil
-			}
-			*collisionCount++
-			continue
+	hash := HashControllerRevision(revision)
+	// Update the revisions name
+	clone.Name = ControllerRevisionName(parent.GetName(), hash)
+	ns := parent.GetNamespace()
+	err := rh.Create(rh.context, clone)
+	if errors.IsAlreadyExists(err) {
+		exists := &appsv1.ControllerRevision{}
+		err := rh.Get(rh.context, types.NamespacedName{Namespace: ns, Name: clone.Name}, exists)
+		if err != nil {
+			return nil, err
+		}
+		if bytes.Equal(exists.Data.Raw, clone.Data.Raw) {
+			return exists, nil
+		} else {
+			// Since the contents of the revision are used to create the hash, the only way this
+			// happens is if the contents of the revision were changed, which is unintended behavior
+			return nil, fmt.Errorf("controller Revision with same name but different content exists")
 		}
-		return clone, err
 	}
+	return clone, err
 }
 
 func (rh *realHistory) UpdateControllerRevision(revision *appsv1.ControllerRevision, newRevision int64) (*appsv1.ControllerRevision, error) {

pkg/history/controller_history_test.go

@@ -32,18 +32,18 @@ func TestFindEqualRevisions(t *testing.T) {
 	lws1 := testutils.BuildLeaderWorkerSet("test-sample").Obj()
 	lws2 := testutils.BuildLeaderWorkerSet("test-sample").LeaderTemplateSpec(testutils.MakeLeaderPodSpecWithTPUResource()).Obj()
 
-	lws1Revision, err := NewControllerRevision(lws1, parentKind, lws1.Labels, testutils.RawLWSTemplate(lws1), 1, lws1.Status.CollisionCount)
+	lws1Revision, err := NewControllerRevision(lws1, parentKind, lws1.Labels, testutils.RawLWSTemplate(lws1), 1)
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	lws2Revision, err := NewControllerRevision(lws2, parentKind, lws2.Labels, testutils.RawLWSTemplate(lws2), 1, lws2.Status.CollisionCount)
+	lws2Revision, err := NewControllerRevision(lws2, parentKind, lws2.Labels, testutils.RawLWSTemplate(lws2), 1)
 	if err != nil {
 		t.Fatal(err)
 	}
 
 	lws1.Spec.LeaderWorkerTemplate.LeaderTemplate.Spec.Containers[0].Name = "update-name"
-	lws1Revision2, err := NewControllerRevision(lws1, parentKind, lws1.Labels, testutils.RawLWSTemplate(lws1), 1, lws1.Status.CollisionCount)
+	lws1Revision2, err := NewControllerRevision(lws1, parentKind, lws1.Labels, testutils.RawLWSTemplate(lws1), 1)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -91,15 +91,15 @@ func TestFindEqualRevisions(t *testing.T) {
 
 func TestSortControllerRevisions(t *testing.T) {
 	lws := testutils.BuildLeaderWorkerSet("test-sample").Obj()
-	lwsRevision1, err := NewControllerRevision(lws, parentKind, lws.Labels, testutils.RawLWSTemplate(lws), 1, lws.Status.CollisionCount)
+	lwsRevision1, err := NewControllerRevision(lws, parentKind, lws.Labels, testutils.RawLWSTemplate(lws), 1)
 	if err != nil {
 		t.Fatal(err)
 	}
-	lwsRevision2, err := NewControllerRevision(lws, parentKind, lws.Labels, testutils.RawLWSTemplate(lws), 2, lws.Status.CollisionCount)
+	lwsRevision2, err := NewControllerRevision(lws, parentKind, lws.Labels, testutils.RawLWSTemplate(lws), 2)
 	if err != nil {
 		t.Fatal(err)
 	}
-	lwsRevision1Time2, err := NewControllerRevision(lws, parentKind, lws.Labels, testutils.RawLWSTemplate(lws), 1, lws.Status.CollisionCount)
+	lwsRevision1Time2, err := NewControllerRevision(lws, parentKind, lws.Labels, testutils.RawLWSTemplate(lws), 1)
 	if err != nil {
 		t.Fatal(err)
 	}