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

cleanupDeployment should sort ReplicaSet with revision rather than createTimeStamp #82570

Open
waynepeking348 opened this issue Sep 11, 2019 · 4 comments · May be fixed by #82783

Comments

@waynepeking348
Copy link
Contributor

commented Sep 11, 2019

What happened:
when deployment controller cleans up old ReplicaSet, it sorts all old ReplicaSets by createTimeStamp and delete the oldest one, which may cause Deployment roll back to a wrong version.

        sort.Sort(controller.ReplicaSetsByCreationTimestamp(cleanableRSes))
	glog.V(4).Infof("Looking to cleanup old replica sets for deployment %q", deployment.Name)

	for i := int32(0); i < diff; i++ {
		rs := cleanableRSes[i]
		// Avoid delete replica set with non-zero replica counts
		if rs.Status.Replicas != 0 || *(rs.Spec.Replicas) != 0 || rs.Generation > rs.Status.ObservedGeneration || rs.DeletionTimestamp != nil {
			continue
		}
		glog.V(4).Infof("Trying to cleanup replica set %q for deployment %q", rs.Name, deployment.Name)
		if err := dc.client.ExtensionsV1beta1().ReplicaSets(rs.Namespace).Delete(rs.Name, nil); err != nil && !errors.IsNotFound(err) {
			// Return error instead of aggregating and continuing DELETEs on the theory
			// that we may be overloading the api server.
			return err
		}
	}

How to reproduce it (as minimally and precisely as possible):
Suppose we set the revisionHistoryLimit of a Deployment to 3, and currently we have 3 Replicas: r1, r2, r3 (the createTimeStamp and Revision both are r3 > r2 > r1); if we upgrade and roll back 3 times continuously, there will be an expected case:

upgrade:
  ReplicaSet will be: r4, r3, r2
  createTimeStamp: r4 > r3 > r2
  reversion: r4 > r3 > r2
rollback: should rollback to v3
  ReplicaSet will be: r4, r3, r2
  createTimeStamp: r4 > r3 > r2
  reversion: r3 > r4 > r2

upgrade:
  ReplicaSet will be: r5, r4, r3
  createTimeStamp: r5 > r4 > r3
  reversion: r5 > r3 > r4
rollback: should rollback to v3
  ReplicaSet will be: r4, r3, r2
  createTimeStamp: r5 > r4 > r3
  reversion: r3 > r5 > r4

upgrade:
  ReplicaSet will be: r6, r5, r4
  createTimeStamp: r6 > r5 > r4
  reversion: r6 > r5 > r4
rollback: should rollback to r3, but instead, we rollbacked to r5, and this may cause error.

What you expected to happen:
when deployment controller cleans up old ReplicaSet, it should sort old ReplicaSets by Revision

@waynepeking348

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

/sig api-machinery
/sig apps

@waynepeking348

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

does this make sense to you?
/assign @deads2k
/cc @derekwaynecarr

@NickrenREN

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

@caesarxuchao

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

/remove-sig api-machinery

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.