From d5227e364d9e504ee957054b1a46392c02a43d22 Mon Sep 17 00:00:00 2001 From: Michail Kargakis Date: Wed, 18 Jan 2017 14:35:30 +0100 Subject: [PATCH] controller: decouple cleanup policy from deployment strategies Deployments get cleaned up only when they are paused, they get scaled up/down, or when the strategy that drives rollouts completes. This means that stuck deployments that fall into none of the above categories will not get cleaned up. Since cleanup is already safe by itself (we only delete old replica sets that are synced by the replica set controller and have no replicas) we can execute it for every deployment when there is no intention to rollback. --- .../deployment/deployment_controller.go | 21 ++++++++++++++++++- pkg/controller/deployment/recreate.go | 2 -- pkg/controller/deployment/rolling.go | 2 -- pkg/controller/deployment/sync.go | 1 - 4 files changed, 20 insertions(+), 6 deletions(-) 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)