From eaaef387e5b31a883325a659be66de3b72799531 Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Thu, 7 Dec 2023 16:58:50 -0600 Subject: [PATCH] fix: conflict on updates to replicaset revision (#3216) * fix: possibly fix conflict on updates to revision Signed-off-by: Zach Aller * wip Signed-off-by: Zach Aller * fix one test Signed-off-by: Zach Aller * fix tests Signed-off-by: Zach Aller * cleanup Signed-off-by: Zach Aller * change order Signed-off-by: Zach Aller * change order to match reality Signed-off-by: Zach Aller * add comments on exptected actions Signed-off-by: Zach Aller --------- Signed-off-by: Zach Aller --- rollout/canary_test.go | 15 ++++++++------- rollout/sync.go | 6 ++++-- rollout/trafficrouting_test.go | 8 ++++---- 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/rollout/canary_test.go b/rollout/canary_test.go index b275170eec..2bd798b813 100644 --- 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 index 2baeac1698..13d3f7fff6 100644 --- 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 index a8f5520ea2..9c20957bf5 100644 --- 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 {