Enable garbage collection of custom resources by ironcladlou · Pull Request #47665 · kubernetes/kubernetes

ironcladlou · 2017-06-16T17:35:31Z

Enhance the garbage collector to periodically refresh the resources it monitors (via discovery) to enable custom resource definition GC (addressing #44507 and reverting #47432).

This is a replacement for #46000.

/cc @lavalamp @deads2k @sttts @caesarxuchao

/ref #48065

The garbage collector now supports custom APIs added via CustomResourceDefinition or aggregated apiservers. Note that the garbage collector controller refreshes periodically, so there is a latency between when the API is added and when the garbage collector starts to manage it.

ironcladlou · 2017-06-19T14:43:47Z

@k8s-bot pull-kubernetes-unit test this

ironcladlou · 2017-06-19T14:44:02Z

@k8s-bot pull-kubernetes-verify test this

deads2k · 2017-06-22T18:53:14Z

cmd/kube-controller-manager/app/core.go

I preferred the construct that got back a caching discovery client and simply started a gofunc that called reset.

deads2k · 2017-06-22T18:55:21Z

cmd/kube-controller-manager/app/core.go

So building a restmapper gives me back a function that somehow handles the garbage collector? that feels weirdly intertwined.

This part of the code was authored by @lavalamp so I can't speak to the decisions leading to the current design, but it works and so I kept it. If you feel strongly we can continue the discussion about a different factoring.

If you feel strongly we can continue the discussion about a different factoring.

I feel strongly. I'm not certain that the monitor sync has to line up exactly with the restmapper sync, but if it does, pass both through to the garbage collector and have the function live there.

If you feel strongly we can continue the discussion about a different factoring.

still outstanding.

deads2k · 2017-06-22T19:31:34Z

pkg/controller/garbagecollector/graph_builder.go

Why don't we make them stop channels here?

Using a nil stop channel to indicate the monitor's start/stopped state.

Using a nil stop channel to indicate the monitor's start/stopped state.

doced and I didn't read to it before commenting her?

deads2k · 2017-06-22T19:33:52Z

Macro-level observation. This makes creating links to resources hosted outside of the main API server exciting. If the API server goes down, then every resource with an owner-ref to it will be reaped.

deads2k · 2017-06-22T19:37:14Z

pkg/controller/garbagecollector/graph_builder.go

This is HasSynced is different than a normal HasSynced method which only moves to true and never back to false. I think this method is IsSynced.

deads2k · 2017-06-22T20:04:09Z

pkg/controller/garbagecollector/memcachediscovery/client.go

I'd expect this in client-go.

deads2k · 2017-06-22T20:08:00Z

pkg/controller/garbagecollector/memcachediscovery/client.go

Does this ever fail for legitimate reasons, like a CRD being deleted for instance.

I need to think about this a little more, but in the meantime maybe @lavalamp can comment more quickly since this whole file is his original unmodified work from #46000.

I need to think about this a little more, but in the meantime maybe @lavalamp can comment more quickly since this whole file is his original unmodified work from #46000.

He probably won't comment here.

deads2k · 2017-06-22T20:09:08Z

pkg/controller/garbagecollector/memcachediscovery/client.go

Given that this will prevent proper handling higher up the chain, it seems worth a utilruntime.HandleError.

deads2k · 2017-06-22T20:09:34Z

pkg/controller/garbagecollector/metaonly/metaonly.go

accidental date change?

deads2k · 2017-06-22T20:10:28Z

pkg/controller/garbagecollector/memcachediscovery/client.go

How about getting this merged separately and see about making use of it in the namespace cleanup controller? It's a smaller controller and it would reduce the size of this pull.

No objection from me. Who else might have an opinion on this?

No objection from me. Who else might have an opinion on this?

Me, @sttts (who will agree with me), and @caesarxuchao who will also likely agree.

deads2k · 2017-06-22T20:13:06Z

pkg/controller/garbagecollector/setup/setup.go

Not a fan of this construct. The garbagecollector already has a reference to this RESTMapper, that seems like a more proper place to tie these things together. You could isolate this to a member function there instead of returning a function to be called later with the thing that requested this.

@sttts

Automatic merge from submit-queue (batch tested with PRs 49712, 49694, 49714, 49670, 49717) Reduce GC e2e test flakiness Increase GC wait timeout in a flaky e2e test. The test expects a GC operation to be performed within 30s, while in practice the operation often takes longer due to a delay between the enqueueing of the owner's delete operation and the GC's actual processing of that event. Doubling the time seems to stabilize the test. The test's assumptions can be revisited, and the processing delay under load can be investigated in the future. Extracted from #47665 per #47665 (comment). /cc @sttts @caesarxuchao @deads2k @kubernetes/sig-api-machinery-bugs ```release-note NONE ```

caesarxuchao · 2017-07-28T17:46:39Z

/lgtm

caesarxuchao · 2017-07-28T17:48:36Z

@ironcladlou could you open an issue saying that we need to add tests to validate that GC works with aggregated apis?

caesarxuchao · 2017-07-28T17:48:59Z

/assign @deads2k
for approval

deads2k · 2017-07-28T17:50:14Z

/approve no-issue

k8s-github-robot · 2017-07-28T17:50:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, deads2k, ironcladlou

Associated issue: 44507

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kube-controller-manager/OWNERS~~ [deads2k]
~~hack/OWNERS~~ [deads2k]
~~pkg/controller/garbagecollector/OWNERS~~ [caesarxuchao,deads2k]
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [deads2k]
~~test/OWNERS~~ [deads2k]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

ironcladlou · 2017-07-28T17:51:27Z

Opened #49801 for followup.

caesarxuchao · 2017-07-28T17:54:18Z

Thank you @ironcladlou for the hard work.

ironcladlou · 2017-07-28T17:56:11Z

@caesarxuchao

Thank you @ironcladlou for the hard work.

Team effort! ✋

k8s-github-robot · 2017-07-29T10:59:35Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-ci-robot · 2017-07-29T12:09:13Z

@ironcladlou: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-etcd3	`d08dfb9`	link	`/test pull-kubernetes-e2e-gce-etcd3`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-github-robot · 2017-07-29T12:28:52Z

Automatic merge from submit-queue (batch tested with PRs 49538, 49708, 47665, 49750, 49528)

liggitt · 2017-07-31T03:29:30Z

test/integration/garbagecollector/garbage_collector_test.go

+// TODO: Consider how this could be represented with table-style tests (e.g. a
+// before/after expected object graph given a delete operation targetting a
+// specific node in the before graph with certain delete options).
+func TestMixedRelationships(t *testing.T) {


This is flaking according to http://storage.googleapis.com/k8s-metrics/flakes-latest.json

Thanks- will look into it

k8s-ci-robot requested review from caesarxuchao, deads2k, lavalamp and sttts June 16, 2017 17:35

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 16, 2017

k8s-github-robot assigned caesarxuchao and jszczepkowski Jun 16, 2017

ironcladlou changed the title ~~WIP: Make GC occasionally refresh its discovery information~~ Enable garbage collection of custom resources Jun 16, 2017

ironcladlou force-pushed the gc-poll-types branch from 9845efa to c49ae04 Compare June 21, 2017 19:28

jszczepkowski removed their assignment Jun 22, 2017

ironcladlou force-pushed the gc-poll-types branch 2 times, most recently from 0a87fee to 2f28fce Compare June 22, 2017 14:49

deads2k reviewed Jun 22, 2017

View reviewed changes

pkg/controller/garbagecollector/memcachediscovery/client.go Outdated

Copy link
Copy Markdown

Contributor

deads2k Jun 22, 2017

Choose a reason for hiding this comment

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

I'd expect this in client-go.

deads2k reviewed Jun 22, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2017

k8s-ci-robot assigned deads2k Jul 28, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2017

ironcladlou mentioned this pull request Jul 28, 2017

Add garbage collector tests to cover aggregated API servers #49801

Open

nikhita mentioned this pull request Jul 29, 2017

[WIP] apiextensions: add integration test for GC #46585

Closed

k8s-github-robot merged commit 7be28a1 into kubernetes:master Jul 29, 2017

liggitt reviewed Jul 31, 2017

View reviewed changes

caesarxuchao mentioned this pull request Jul 31, 2017

Set default vmodule flag in integration tests #49886

Merged

ironcladlou mentioned this pull request Jul 31, 2017

Make GC occasionally refresh its discovery information #46000

Closed

hongchaodeng mentioned this pull request Jul 31, 2017

Use upstream CR garbage collection coreos/etcd-operator#1337

Closed

This was referenced Aug 1, 2017

Temporarily remove SQ-blocking integration test #49944

Closed

Revert SQ-blocking PR (on custom-resource GC) #49946

Closed

ash2k mentioned this pull request Aug 1, 2017

DefaultRESTMapper does not work for custom resources CRD #49948

Closed

brancz mentioned this pull request Aug 3, 2017

Use ownerReferences prometheus-operator/prometheus-operator#12

Closed

ironcladlou mentioned this pull request Aug 11, 2017

Finalizers are broken for CRDs in 1.7 #50528

Closed

ironcladlou mentioned this pull request Aug 25, 2017

Controller manager NPE because some controllers aren't ready to receive shared informer events immediately after adding handlers #51013

Closed

jlewi mentioned this pull request Sep 26, 2017

Use K8s Garbage Collection kubeflow/trainer#42

Closed

nikhita mentioned this pull request May 23, 2018

Support object count quota for custom resources #64201

Closed

Conversation

ironcladlou commented Jun 16, 2017 • edited by enisoc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ironcladlou commented Jun 19, 2017

Uh oh!

ironcladlou commented Jun 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Jun 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caesarxuchao commented Jul 28, 2017

Uh oh!

caesarxuchao commented Jul 28, 2017

Uh oh!

caesarxuchao commented Jul 28, 2017

Uh oh!

deads2k commented Jul 28, 2017

Uh oh!

k8s-github-robot commented Jul 28, 2017

Uh oh!

ironcladlou commented Jul 28, 2017

Uh oh!

caesarxuchao commented Jul 28, 2017

Uh oh!

ironcladlou commented Jul 28, 2017

Uh oh!

k8s-github-robot commented Jul 29, 2017

Uh oh!

k8s-ci-robot commented Jul 29, 2017

Uh oh!

k8s-github-robot commented Jul 29, 2017

Uh oh!

liggitt Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ironcladlou commented Jun 16, 2017 •

edited by enisoc

Loading

liggitt Jul 31, 2017 •

edited

Loading