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

Kube GC improvements backports #14358

Merged

Conversation

ncdc
Copy link
Contributor

@ncdc ncdc commented May 25, 2017

Use shared informers for GC if possible
Update required verbs for deletable resources
Allow list of ignored resources to be customized

@smarterclayton @liggitt @deads2k @sttts @mfojtik

ncdc added 2 commits May 25, 2017 12:34
Use shared informers for GC if possible
Update required verbs for deletable resources
Allow list of ignored resources to be customized
@ncdc
Copy link
Contributor Author

ncdc commented May 25, 2017

This seems to work locally. It logged that it was using shared informers for GC where it could. It can't, however, use shared informers for OpenShift resources, because the Kubernetes GarbageController doesn't have access to any shared informers other than those in Kubernetes. We could modify the GC configuration to take in something like

type MonitorFactory interface {
  MonitorFor(resource schema.GroupResource) (cache.Controller, error)
}

and then allow OpenShift to use one of its own that knows how to look up shared informers for both Kubernetes and OpenShift resources. Do you think this change is worth doing, given the desire to have OpenShift run on top of Kubernetes?

[test]

@smarterclayton
Copy link
Contributor

smarterclayton commented May 25, 2017 via email

@smarterclayton
Copy link
Contributor

Tests have a few failures, otherwise LGTM. The MonitorFor we should try to get for 3.6 if we can - we need to get our cache counts down to hit our scale numbers.

@ncdc
Copy link
Contributor Author

ncdc commented May 26, 2017

@smarterclayton latest commit hopefully fixes the test issues

@smarterclayton
Copy link
Contributor

Worked, was flake.

@smarterclayton
Copy link
Contributor

[merge][severity:blocker] serious performance regression

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 73f9c02

@smarterclayton
Copy link
Contributor

[test]

@smarterclayton
Copy link
Contributor

smarterclayton commented May 27, 2017 via email

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 73f9c02

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge Running (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/826/) (Base Commit: 908f6a0) (Extended Tests: blocker)

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1814/) (Base Commit: 908f6a0)

@smarterclayton
Copy link
Contributor

Failed due to idler flake, force merging

@smarterclayton smarterclayton merged commit 78178cc into openshift:master May 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants