diff --git a/pkg/controller/deployment/deployment_controller.go b/pkg/controller/deployment/deployment_controller.go index 6c6ee47a806..b1c63c0b06d 100644 --- a/pkg/controller/deployment/deployment_controller.go +++ b/pkg/controller/deployment/deployment_controller.go @@ -543,7 +543,26 @@ func (dc *DeploymentController) syncDeployment(key string) error { return dc.syncStatusOnly(d) } - err = dc.classifyReplicaSets(deployment) + // Why run the cleanup policy only when there is no rollback request? + // The thing with the cleanup policy currently is that it is far from smart because it takes into account + // the latest replica sets while it should instead retain the latest *working* replica sets. This means that + // you can have a cleanup policy of 1 but your last known working replica set may be 2 or 3 versions back + // in the history. + // Eventually we will want to find a way to recognize replica sets that have worked at some point in time + // (and chances are higher that they will work again as opposed to others that didn't) for candidates to + // automatically roll back to (#23211) and the cleanup policy should help. + if d.Spec.RollbackTo == nil { + _, oldRSs, err := dc.getAllReplicaSetsAndSyncRevision(d, false) + if err != nil { + return err + } + // So far the cleanup policy was executed once a deployment was paused, scaled up/down, or it + // succesfully completed deploying a replica set. Decouple it from the strategies and have it + // run almost unconditionally - cleanupDeployment is safe by default. + dc.cleanupDeployment(oldRSs, d) + } + + err = dc.classifyReplicaSets(d) if err != nil { return err } diff --git a/pkg/controller/deployment/recreate.go b/pkg/controller/deployment/recreate.go index 54e4290a9ff..3c3be85481e 100644 --- a/pkg/controller/deployment/recreate.go +++ b/pkg/controller/deployment/recreate.go @@ -73,8 +73,6 @@ func (dc *DeploymentController) rolloutRecreate(deployment *extensions.Deploymen return dc.syncRolloutStatus(allRSs, newRS, deployment) } - dc.cleanupDeployment(oldRSs, deployment) - // Sync deployment status return dc.syncRolloutStatus(allRSs, newRS, deployment) } diff --git a/pkg/controller/deployment/rolling.go b/pkg/controller/deployment/rolling.go index 1c41508336f..d6ddf9da8ce 100644 --- a/pkg/controller/deployment/rolling.go +++ b/pkg/controller/deployment/rolling.go @@ -55,8 +55,6 @@ func (dc *DeploymentController) rolloutRolling(deployment *extensions.Deployment return dc.syncRolloutStatus(allRSs, newRS, deployment) } - dc.cleanupDeployment(oldRSs, deployment) - // Sync deployment status return dc.syncRolloutStatus(allRSs, newRS, deployment) } diff --git a/pkg/controller/deployment/sync.go b/pkg/controller/deployment/sync.go index ba41759e04b..929a868e623 100644 --- a/pkg/controller/deployment/sync.go +++ b/pkg/controller/deployment/sync.go @@ -58,7 +58,6 @@ func (dc *DeploymentController) sync(deployment *extensions.Deployment) error { // so we can abort this resync return err } - dc.cleanupDeployment(oldRSs, deployment) allRSs := append(oldRSs, newRS) return dc.syncDeploymentStatus(allRSs, newRS, deployment)