fix: conflict on updates to replicaset revision (#3216)

* fix: possibly fix conflict on updates to revision Signed-off-by: Zach Aller <[email protected]> * wip Signed-off-by: Zach Aller <[email protected]> * fix one test Signed-off-by: Zach Aller <[email protected]> * fix tests Signed-off-by: Zach Aller <[email protected]> * cleanup Signed-off-by: Zach Aller <[email protected]> * change order Signed-off-by: Zach Aller <[email protected]> * change order to match reality Signed-off-by: Zach Aller <[email protected]> * add comments on exptected actions Signed-off-by: Zach Aller <[email protected]> --------- Signed-off-by: Zach Aller <[email protected]>
argoproj · Dec 7, 2023 · 1305129 · 1305129
1 parent 5bcacd9
commit 1305129
Show file tree

Hide file tree

Showing 3 changed files with 16 additions and 13 deletions.
diff --git a/rollout/canary_test.go b/rollout/canary_test.go
@@ -823,9 +823,9 @@ func TestRollBackToStable(t *testing.T) {
 	f.rolloutLister = append(f.rolloutLister, r2)
 	f.objects = append(f.objects, r2)
 
-	updatedRSIndex := f.expectUpdateReplicaSetAction(rs1)
-	f.expectUpdateReplicaSetAction(rs1)
-	patchIndex := f.expectPatchRolloutAction(r2)
+	updatedRSIndex := f.expectUpdateReplicaSetAction(rs1) // Bump replicaset revision from 1 to 3
+	f.expectUpdateRolloutAction(r2)                       // Bump rollout revision from 1 to 3
+	patchIndex := f.expectPatchRolloutAction(r2)          // Patch rollout status
 	f.run(getKey(r2, t))
 
 	expectedRS1 := rs1.DeepCopy()
@@ -883,9 +883,9 @@ func TestRollBackToActiveReplicaSetWithinWindow(t *testing.T) {
 	f.rolloutLister = append(f.rolloutLister, r2)
 	f.objects = append(f.objects, r2)
 
-	f.expectUpdateReplicaSetAction(rs1)
-	f.expectUpdateReplicaSetAction(rs1)
-	rolloutPatchIndex := f.expectPatchRolloutAction(r2)
+	f.expectUpdateReplicaSetAction(rs1)                 // Update replicaset revision from 1 to 3
+	f.expectUpdateRolloutAction(r2)                     // Update rollout revision from 1 to 3
+	rolloutPatchIndex := f.expectPatchRolloutAction(r2) // Patch rollout status
 	f.run(getKey(r2, t))
 
 	expectedStepIndex := len(steps)
@@ -963,7 +963,8 @@ func TestRollBackToStableAndStepChange(t *testing.T) {
 	f.objects = append(f.objects, r2)
 
 	updatedRSIndex := f.expectUpdateReplicaSetAction(rs1)
-	f.expectUpdateReplicaSetAction(rs1)
+	//f.expectUpdateReplicaSetAction(rs1)
+	f.expectUpdateRolloutAction(r2)
 	patchIndex := f.expectPatchRolloutAction(r2)
 	f.run(getKey(r2, t))
 

diff --git a/rollout/sync.go b/rollout/sync.go
@@ -280,7 +280,8 @@ func (c *rolloutContext) createDesiredReplicaSet() (*appsv1.ReplicaSet, error) {
 // syncReplicasOnly is responsible for reconciling rollouts on scaling events.
 func (c *rolloutContext) syncReplicasOnly() error {
 	c.log.Infof("Syncing replicas only due to scaling event")
-	_, err := c.getAllReplicaSetsAndSyncRevision(false)
+	var err error
+	c.newRS, err = c.getAllReplicaSetsAndSyncRevision(false)
 	if err != nil {
 		return err
 	}
@@ -326,7 +327,8 @@ func (c *rolloutContext) syncReplicasOnly() error {
 //
 // rsList should come from getReplicaSetsForRollout(r).
 func (c *rolloutContext) isScalingEvent() (bool, error) {
-	_, err := c.getAllReplicaSetsAndSyncRevision(false)
+	var err error
+	c.newRS, err = c.getAllReplicaSetsAndSyncRevision(false)
 	if err != nil {
 		return false, err
 	}

diff --git a/rollout/trafficrouting_test.go b/rollout/trafficrouting_test.go
@@ -1213,10 +1213,10 @@ func TestDontWeightToZeroWhenDynamicallyRollingBackToStable(t *testing.T) {
 	f.rolloutLister = append(f.rolloutLister, r2)
 	f.objects = append(f.objects, r2)
 
-	f.expectUpdateReplicaSetAction(rs1)                 // Updates the revision annotation from 1 to 3
-	f.expectUpdateReplicaSetAction(rs1)                 // repeat of the above (not sure why)
-	scaleUpIndex := f.expectUpdateReplicaSetAction(rs1) // this one scales the stable RS to 10
-	f.expectPatchRolloutAction(r2)
+	f.expectUpdateReplicaSetAction(rs1)                 // Updates the revision annotation from 1 to 3 from func isScalingEvent
+	f.expectUpdateRolloutAction(r2)                     // Update the rollout revision from 1 to 3
+	scaleUpIndex := f.expectUpdateReplicaSetAction(rs1) // Scale The replicaset from 1 to 10 from func scaleReplicaSet
+	f.expectPatchRolloutAction(r2)                      // Updates the rollout status from the scaling to 10 action
 
 	f.fakeTrafficRouting = newUnmockedFakeTrafficRoutingReconciler()
 	f.fakeTrafficRouting.On("UpdateHash", mock.Anything, mock.Anything, mock.Anything).Return(func(canaryHash, stableHash string, additionalDestinations ...v1alpha1.WeightDestination) error {