Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replicaset Conflict #3218

Closed
zachaller opened this issue Dec 6, 2023 · 0 comments · Fixed by #3216
Closed

Replicaset Conflict #3218

zachaller opened this issue Dec 6, 2023 · 0 comments · Fixed by #3216
Labels
bug Something isn't working

Comments

@zachaller
Copy link
Collaborator

zachaller commented Dec 6, 2023

Notes on RS conflict: This should also not cause any isses because we generally retry the reconcile on errors but it's noisy and inefficent

When looking at context.go we have this bit of code below with 3 functions calls c.isScalingEvent() and c.syncReplicasOnly() and c.rolloutCanary(). I also added as a comment the functions calls they make and we can see they all end up calling c.syncReplicaSetRevision()

rollouts/context.go

        isScalingEvent, err := c.isScalingEvent() // Calls: getAllReplicaSetsAndSyncRevision() ----> c.syncReplicaSetRevision()
	if err != nil {
		return err
	}

	if isScalingEvent {
		return c.syncReplicasOnly() // Calls: c.getAllReplicaSetsAndSyncRevision() ----> c.syncReplicaSetRevision()
	}

	if c.rollout.Spec.Strategy.BlueGreen != nil {
		return c.rolloutBlueGreen() // Calls: getAllReplicaSetsAndSyncRevision() ----> c.syncReplicaSetRevision()
	}

	// Due to the rollout validation before this, when we get here strategy is canary
	return c.rolloutCanary() // Calls: c.getAllReplicaSetsAndSyncRevision(true) ---->  c.syncReplicaSetRevision()
}

In synRelicasOnly() and isScalingEvent() we do not update c.newRS they both do something like

_, err := c.getAllReplicaSetsAndSyncRevision(false)

Which I suspect then when we call rolloutCanary() we are working on with an old replicaset

I think this happens generally when there is a scaling event and we call syncReplicasOnly() and we had already updated the rs from the isScalingEvent() call. The other case would be when we update the rs in the isScalingEvent() call and then we also try to update in the rolloutCanary() or rolloutBlueGreen() calls.

snippet from func syncReplicaSetRevision() this is where the actual update happens showing just for context

        annotationsUpdated := annotations.SetNewReplicaSetAnnotations(c.rollout, rsCopy, newRevision, true)
	minReadySecondsNeedsUpdate := rsCopy.Spec.MinReadySeconds != c.rollout.Spec.MinReadySeconds
	affinityNeedsUpdate := replicasetutil.IfInjectedAntiAffinityRuleNeedsUpdate(rsCopy.Spec.Template.Spec.Affinity, *c.rollout)

	if annotationsUpdated || minReadySecondsNeedsUpdate || affinityNeedsUpdate {
		rsCopy.Spec.MinReadySeconds = c.rollout.Spec.MinReadySeconds
		rsCopy.Spec.Template.Spec.Affinity = replicasetutil.GenerateReplicaSetAffinity(*c.rollout)
		rs, err := c.kubeclientset.AppsV1().ReplicaSets(rsCopy.ObjectMeta.Namespace).Update(ctx, rsCopy, metav1.UpdateOptions{})
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant