From b9839d0677b3b4d058d230f325c5f9304e55709f Mon Sep 17 00:00:00 2001 From: Michail Kargakis Date: Fri, 12 Feb 2016 16:56:44 +0100 Subject: [PATCH] controller: fix cleanup policy for deployments Cleanup policy should run on all replica sets and not only on those that have pods (we will not cleanup those anyway). --- pkg/controller/controller_utils.go | 11 +++++ .../deployment/deployment_controller.go | 42 +++++-------------- test/e2e/util.go | 2 +- 3 files changed, 23 insertions(+), 32 deletions(-) diff --git a/pkg/controller/controller_utils.go b/pkg/controller/controller_utils.go index 9d573560e78..8524ee4d128 100644 --- a/pkg/controller/controller_utils.go +++ b/pkg/controller/controller_utils.go @@ -432,6 +432,17 @@ func FilterActivePods(pods []api.Pod) []*api.Pod { return result } +// FilterActiveReplicaSets returns replica sets that have (or at least ought to have) pods. +func FilterActiveReplicaSets(replicaSets []*extensions.ReplicaSet) []*extensions.ReplicaSet { + active := []*extensions.ReplicaSet{} + for i := range replicaSets { + if replicaSets[i].Spec.Replicas > 0 { + active = append(active, replicaSets[i]) + } + } + return active +} + // ControllersByCreationTimestamp sorts a list of ReplicationControllers by creation timestamp, using their names as a tie breaker. type ControllersByCreationTimestamp []*api.ReplicationController diff --git a/pkg/controller/deployment/deployment_controller.go b/pkg/controller/deployment/deployment_controller.go index f562693c80e..9422488d627 100644 --- a/pkg/controller/deployment/deployment_controller.go +++ b/pkg/controller/deployment/deployment_controller.go @@ -441,7 +441,7 @@ func (dc *DeploymentController) syncDeployment(key string) error { // Rolling back to a revision; no-op if the toRevision is deployment's current revision func (dc *DeploymentController) rollback(deployment *extensions.Deployment, toRevision *int64) (*extensions.Deployment, error) { - newRS, allOldRSs, err := dc.getNewAndAllOldReplicaSets(*deployment) + newRS, allOldRSs, err := dc.getAllReplicaSets(*deployment) if err != nil { return nil, err } @@ -493,14 +493,14 @@ func (dc *DeploymentController) updateDeploymentAndClearRollbackTo(deployment *e } func (dc *DeploymentController) syncRecreateDeployment(deployment extensions.Deployment) error { - newRS, oldRSs, err := dc.getNewAndOldReplicaSets(deployment) + newRS, oldRSs, err := dc.getAllReplicaSets(deployment) if err != nil { return err } - allRSs := append(oldRSs, newRS) + allRSs := append(controller.FilterActiveReplicaSets(oldRSs), newRS) // scale down old replica sets - scaledDown, err := dc.scaleDownOldReplicaSetsForRecreate(oldRSs, deployment) + scaledDown, err := dc.scaleDownOldReplicaSetsForRecreate(controller.FilterActiveReplicaSets(oldRSs), deployment) if err != nil { return err } @@ -526,16 +526,14 @@ func (dc *DeploymentController) syncRecreateDeployment(deployment extensions.Dep // Sync deployment status return dc.syncDeploymentStatus(allRSs, newRS, deployment) - - // TODO: raise an event, neither scaled up nor down. } func (dc *DeploymentController) syncRollingUpdateDeployment(deployment extensions.Deployment) error { - newRS, oldRSs, err := dc.getNewAndOldReplicaSets(deployment) + newRS, oldRSs, err := dc.getAllReplicaSets(deployment) if err != nil { return err } - allRSs := append(oldRSs, newRS) + allRSs := append(controller.FilterActiveReplicaSets(oldRSs), newRS) // Scale up, if we can. scaledUp, err := dc.reconcileNewReplicaSet(allRSs, newRS, deployment) @@ -548,7 +546,7 @@ func (dc *DeploymentController) syncRollingUpdateDeployment(deployment extension } // Scale down, if we can. - scaledDown, err := dc.reconcileOldReplicaSets(allRSs, oldRSs, newRS, deployment, true) + scaledDown, err := dc.reconcileOldReplicaSets(allRSs, controller.FilterActiveReplicaSets(oldRSs), newRS, deployment, true) if err != nil { return err } @@ -564,8 +562,6 @@ func (dc *DeploymentController) syncRollingUpdateDeployment(deployment extension // Sync deployment status return dc.syncDeploymentStatus(allRSs, newRS, deployment) - - // TODO: raise an event, neither scaled up nor down. } // syncDeploymentStatus checks if the status is up-to-date and sync it if necessary @@ -580,10 +576,9 @@ func (dc *DeploymentController) syncDeploymentStatus(allRSs []*extensions.Replic return nil } -// getNewAndMaybeFilteredOldReplicaSets returns new replica set and old replica sets of the deployment. If ignoreNoPod is true, -// the returned old replica sets won't include the ones with no pods; otherwise, all old replica sets will be returned. -func (dc *DeploymentController) getNewAndMaybeFilteredOldReplicaSets(deployment extensions.Deployment, ignoreNoPod bool) (*extensions.ReplicaSet, []*extensions.ReplicaSet, error) { - oldRSs, allOldRSs, err := dc.getOldReplicaSets(deployment) +// getAllReplicaSets returns all the replica sets for the provided deployment (new and all old). +func (dc *DeploymentController) getAllReplicaSets(deployment extensions.Deployment) (*extensions.ReplicaSet, []*extensions.ReplicaSet, error) { + _, allOldRSs, err := dc.getOldReplicaSets(deployment) if err != nil { return nil, nil, err } @@ -604,22 +599,7 @@ func (dc *DeploymentController) getNewAndMaybeFilteredOldReplicaSets(deployment } } - if !ignoreNoPod { - return newRS, allOldRSs, nil - } - return newRS, oldRSs, nil -} - -// getNewAndOldReplicaSets returns new replica set and old replica sets of the deployment. -// Note that the returned old replica sets don't include the ones with no pods. -func (dc *DeploymentController) getNewAndOldReplicaSets(deployment extensions.Deployment) (*extensions.ReplicaSet, []*extensions.ReplicaSet, error) { - return dc.getNewAndMaybeFilteredOldReplicaSets(deployment, true) -} - -// getNewAndAllOldReplicaSets returns new replica set and old replica sets of the deployment. -// Note that all old replica sets are returned, include the ones with no pods. -func (dc *DeploymentController) getNewAndAllOldReplicaSets(deployment extensions.Deployment) (*extensions.ReplicaSet, []*extensions.ReplicaSet, error) { - return dc.getNewAndMaybeFilteredOldReplicaSets(deployment, false) + return newRS, allOldRSs, nil } func maxRevision(allRSs []*extensions.ReplicaSet) int64 { diff --git a/test/e2e/util.go b/test/e2e/util.go index 15856888912..f84692d4088 100644 --- a/test/e2e/util.go +++ b/test/e2e/util.go @@ -2117,7 +2117,7 @@ func waitForDeploymentOldRSsNum(c *clientset.Clientset, ns, deploymentName strin if err != nil { return false, err } - oldRSs, _, err := deploymentutil.GetOldReplicaSets(*deployment, c) + _, oldRSs, err := deploymentutil.GetOldReplicaSets(*deployment, c) if err != nil { return false, err }