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

UPSTREAM: 44058: Make background garbage collection cascading #14907

Merged
merged 2 commits into from
Jun 28, 2017

Conversation

soltysh
Copy link
Contributor

@soltysh soltysh commented Jun 27, 2017

This is upstream cherry-pick and revert of the changes that happened in origin in the mean time to address problems with garbage collection.

Fixes #14805

@liggitt @jwforres @mfojtik fyi
@tnozicka ptal

@soltysh
Copy link
Contributor Author

soltysh commented Jun 27, 2017

[test]

@@ -142,12 +142,10 @@ func (m *RCControllerRefManager) AdoptReplicationController(rs *v1.ReplicationCo
`{"metadata":{
"ownerReferences":[{"apiVersion":"%s","kind":"%s","name":"%s","uid":"%s","controller":true,"blockOwnerDeletion":true}],
"uid":"%s",
Copy link
Contributor

Choose a reason for hiding this comment

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

@soltysh that trailing comma is invalid JSON

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, will fix.

@tnozicka
Copy link
Contributor

@liggitt I don't think we have backported the finalizers in Deployments yet so nothing to revert for Deplyoments, but I would have to check

@tnozicka
Copy link
Contributor

#14880 should go first to be sure about changing JSON and it also tests cascade deletion for DC->RC->Pod

@tnozicka
Copy link
Contributor

@soltysh
Copy link
Contributor Author

soltysh commented Jun 27, 2017

Do we also need the upstream revert that did similar things?

@liggitt the bits that happened upstream were reverted as part of the same PR, iiuc. Nothing else was added.

Copy link
Contributor

@tnozicka tnozicka left a comment

Choose a reason for hiding this comment

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

lgtm, but wait for #14880 to be sure

@liggitt
Copy link
Contributor

liggitt commented Jun 27, 2017

LGTM as well

@tnozicka
Copy link
Contributor

#14880 is in; [test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 531471d

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2743/) (Base Commit: 6bbad88) (PR Branch Commit: 531471d)

@tnozicka
Copy link
Contributor

The new tests succeeded. Ship it!

@tnozicka tnozicka removed their assignment Jun 28, 2017
@tnozicka tnozicka added this to the 3.6.0 milestone Jun 28, 2017
@soltysh
Copy link
Contributor Author

soltysh commented Jun 28, 2017

[merge] on two lgtm

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 28, 2017

continuous-integration/openshift-jenkins/merge Waiting: You are in the build queue at position: 7

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 531471d

@deads2k
Copy link
Contributor

deads2k commented Jun 28, 2017

Green tests against current HEAD. No conflicts with top of the queue. merging

@deads2k deads2k merged commit 05b5e1d into openshift:master Jun 28, 2017
@soltysh soltysh deleted the issue14805 branch June 29, 2017 10:12
@soltysh soltysh restored the issue14805 branch June 29, 2017 13:07
@soltysh soltysh deleted the issue14805 branch November 7, 2017 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants