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.

Sign up for GitHub

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

Jump to bottom

Do not cleanup already deleted replica sets and add more logging around it #41145

Merged
merged 2 commits into from
Feb 9, 2017
Merged

Do not cleanup already deleted replica sets and add more logging around it #41145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff83eb5
Add more logs during the cleanup phase of a deployment
0xmichalis Feb 9, 2017
97c9e7f
Do not cleanup replicasets already marked for deletion
0xmichalis Feb 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion pkg/controller/deployment/sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,15 +547,17 @@ func (dc *DeploymentController) cleanupDeployment(oldRSs []*extensions.ReplicaSe
}

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

var errList []error
// TODO: This should be parallelized.
for i := int32(0); i < diff; i++ {
rs := oldRSs[i]
// Avoid delete replica set with non-zero replica counts
if rs.Status.Replicas != 0 || *(rs.Spec.Replicas) != 0 || rs.Generation > rs.Status.ObservedGeneration {
if rs.Status.Replicas != 0 || *(rs.Spec.Replicas) != 0 || rs.Generation > rs.Status.ObservedGeneration || rs.DeletionTimestamp != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not about logging, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, but it seems like a possible source of the problem for the flaky test - see #41163 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move it to its own commit? And mention it in the PR. Otherwise, it looks like this only changes some logging.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't there be accompanying unit test update to cover this case as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we do this before the for loop so that we get accurate diff?

continue
}
glog.V(2).Infof("Trying to cleanup replica set %q for deployment %q", rs.Name, deployment.Name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

V(2) looks like pretty low. With which level are e2e tests run?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't expect it be high :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From AWS I can get back V(2) - I will open an issue to bump these once I get a clean run

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#41187

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

if err := dc.client.Extensions().ReplicaSets(rs.Namespace).Delete(rs.Name, nil); err != nil && !errors.IsNotFound(err) {
glog.V(2).Infof("Failed deleting old replica set %v for deployment %v: %v", rs.Name, deployment.Name, err)
errList = append(errList, err)
Expand Down
12 changes: 12 additions & 0 deletions pkg/controller/deployment/sync_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,9 @@ func TestScale(t *testing.T) {

func TestDeploymentController_cleanupDeployment(t *testing.T) {
selector := map[string]string{"foo": "bar"}
alreadyDeleted := newRSWithStatus("foo-1", 0, 0, selector)
now := metav1.Now()
alreadyDeleted.DeletionTimestamp = &now

tests := []struct {
oldRSs []*extensions.ReplicaSet
Expand Down Expand Up @@ -366,10 +369,19 @@ func TestDeploymentController_cleanupDeployment(t *testing.T) {
revisionHistoryLimit: 0,
expectedDeletions: 0,
},
{
oldRSs: []*extensions.ReplicaSet{
alreadyDeleted,
},
revisionHistoryLimit: 0,
expectedDeletions: 0,
},
}

for i := range tests {
test := tests[i]
t.Logf("scenario %d", i)

fake := &fake.Clientset{}
informers := informers.NewSharedInformerFactory(nil, fake, controller.NoResyncPeriodFunc())
controller := NewDeploymentController(informers.Extensions().V1beta1().Deployments(), informers.Extensions().V1beta1().ReplicaSets(), informers.Core().V1().Pods(), fake)
Expand Down
14 changes: 12 additions & 2 deletions test/e2e/framework/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3422,17 +3422,27 @@ func WaitForPodsReady(c clientset.Interface, ns, name string, minReadySeconds in

// Waits for the deployment to clean up old rcs.
func WaitForDeploymentOldRSsNum(c clientset.Interface, ns, deploymentName string, desiredRSNum int) error {
return wait.Poll(Poll, 5*time.Minute, func() (bool, error) {
var oldRSs []*extensions.ReplicaSet
var d *extensions.Deployment

pollErr := wait.PollImmediate(Poll, 5*time.Minute, func() (bool, error) {
deployment, err := c.Extensions().Deployments(ns).Get(deploymentName, metav1.GetOptions{})
if err != nil {
return false, err
}
_, oldRSs, err := deploymentutil.GetOldReplicaSets(deployment, c)
d = deployment

_, oldRSs, err = deploymentutil.GetOldReplicaSets(deployment, c)
if err != nil {
return false, err
}
return len(oldRSs) == desiredRSNum, nil
})
if pollErr == wait.ErrWaitTimeout {
pollErr = fmt.Errorf("%d old replica sets were not cleaned up for deployment %q", len(oldRSs)-desiredRSNum, deploymentName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we not want to print the namespace for the deployment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed here, the ns is available via other means (events for example)

logReplicaSetsOfDeployment(d, oldRSs, nil)
}
return pollErr
}

func logReplicaSetsOfDeployment(deployment *extensions.Deployment, allOldRSs []*extensions.ReplicaSet, newRS *extensions.ReplicaSet) {
Expand Down