Make GC occasionally refresh its discovery information by lavalamp · Pull Request #46000 · kubernetes/kubernetes

lavalamp · 2017-05-17T23:01:15Z

ref #44507 - this should unblock AA and TPR even though it's not a full fix.

k8s-github-robot · 2017-05-17T23:01:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp

Details

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

~~cmd/kube-controller-manager/OWNERS~~ [lavalamp]
~~pkg/controller/OWNERS~~ [lavalamp]

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

deads2k · 2017-05-18T12:02:29Z

pkg/controller/garbagecollector/memcachediscovery/client.go

We've had issues caching empty returns. If the list is empty, don't cache it. Same fo rresources.

What causes empty lists?

This is going to be updated every 30s so it should be ok if it forgets stuff for 30s

What causes empty lists?

This is going to be updated every 30s so it should be ok if it forgets stuff for 30s

Server start seems to start listening for discovery before registering groups. It would be pretty bad if GC didn't work for a 30 second window for core types on startup. It ought not be a difficult check to add.

seems like the wrong fix, server could be in the middle of registering groups, too, and then we'd get some right and some wrong for 30s.

If an empty list is an error, then apiserver should probably not return 200s on empty lists?

deads2k · 2017-05-18T12:03:41Z

pkg/controller/garbagecollector/memcachediscovery/client.go

this looks off. Why are you always fresh? Deserves a comment if its right.

Alright, I think I see. Setting this means that you're never being invalidated by higher logic. Definitely needs a comment explaining it and how its to be used.

Honestly the interface didn't document what this is supposed to do...

Reading the code that calls Fresh I still don't understand what it is supposed to do.

deads2k · 2017-05-18T12:07:09Z

pkg/controller/garbagecollector/memcachediscovery/client.go

why? Just for the cached return on error? I'd rather see that handled by outter logic that handles the error.

deads2k · 2017-05-18T12:10:01Z

I think that this needs at least one e2e test that creates a TPR and proves that GC collects it.

lavalamp · 2017-05-18T17:37:38Z

thanks for feedback.

we've got to stop making unwieldy e2e tests. This should be testable as a unit or integration test in the GC, which I should add.

ncdc · 2017-05-19T13:37:44Z

@lavalamp I don't think this is sufficient - the graph builder currently only sets up its monitors to list/watch resources when the GC controller is first instantiated, but it doesn't refresh right now.

lavalamp · 2017-05-19T21:27:10Z

@ncdc good point, I will fix that in another PR.

caesarxuchao · 2017-05-19T22:39:03Z

pkg/controller/garbagecollector/memcachediscovery/client.go

I think this is a better definition of Fresh(). Could you update the comment in the interface as well?

The current comment in the interface describes a narrower case that's implemented by CachedDiscoveryClient, could you move the current comment in the interface to there?

The current comment makes no sense to me whatsoever, so ok.

lavalamp · 2017-05-19T23:12:41Z

@mikedanese any idea why bazel can't find the new import?

krmayankk · 2017-05-20T03:53:07Z

pkg/controller/garbagecollector/memcachediscovery/client.go

@lavalamp what does DiscoveryInterface provide ? Is this related to service discovery or api discovery ?

mikedanese · 2017-05-22T21:34:43Z

Looks like you figured it out.

lavalamp · 2017-05-22T22:36:15Z

@mikedanese yes, I did, sorry for the noise. (git commit -a doesn't actually add new files, duh...)

sttts · 2017-05-29T15:42:24Z

pkg/controller/garbagecollector/memcachediscovery/client.go

+	defer d.lock.RUnlock()
+	cachedVal, ok := d.groupToServerResources[groupVersion]
+	if !ok {
+		return nil, ErrCacheEmpty


looks more like a NotFound than ErrCacheEmpty.

k8s-github-robot · 2017-05-29T15:42:40Z

@lavalamp PR needs rebase

sttts · 2017-05-29T15:50:48Z

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

+		deletableResources := discovery.FilteredBy(discovery.SupportsAllVerbs{Verbs: []string{"delete"}}, preferredResources)
+		deletableGroupVersionResources, err := discovery.GroupVersionResources(deletableResources)
+		if err != nil {
+			return nil, fmt.Errorf("Failed to parse resources from server: %v", err)


nit: lower-case

deads2k · 2017-06-01T12:47:25Z

@lavalamp are you going to come back to this as a bugfix after freeze?

lavalamp · 2017-06-01T21:46:02Z

@deads2k I am not sure. My calendar is pretty booked over the next two weeks. :/ Let me see if Dawn will buy this is a bug fix.

ash2k · 2017-06-02T01:51:10Z

Please consider moving the client to client-go repo so that it is more reusable. See kubernetes/client-go/issues/200

ironcladlou · 2017-06-12T17:58:55Z

@lavalamp do you have any update on your plans for this PR? Is there anything I can do to help move it forward?

k8s-ci-robot · 2017-06-13T16:59:48Z

@lavalamp: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
Jenkins Bazel Build	31800f978d61b3c09d714356d8a438cfe9ab7491	link	`@k8s-bot bazel test this`
Jenkins verification	31800f978d61b3c09d714356d8a438cfe9ab7491	link	`@k8s-bot verify test this`
Jenkins GCE etcd3 e2e	31800f978d61b3c09d714356d8a438cfe9ab7491	link	`@k8s-bot gce etcd3 e2e test this`
pull-kubernetes-bazel	`246241e`	link	`@k8s-bot pull-kubernetes-bazel test this`
pull-kubernetes-kubemark-e2e-gce	`246241e`	link	`@k8s-bot pull-kubernetes-kubemark-e2e-gce test this`
pull-kubernetes-e2e-kops-aws	`246241e`	link	`@k8s-bot pull-kubernetes-e2e-kops-aws test this`
pull-kubernetes-unit	`246241e`	link	`@k8s-bot pull-kubernetes-unit test this`
pull-kubernetes-verify	`246241e`	link	`@k8s-bot pull-kubernetes-verify test this`
pull-kubernetes-federation-e2e-gce	`246241e`	link	`@k8s-bot pull-kubernetes-federation-e2e-gce test this`
pull-kubernetes-node-e2e	`246241e`	link	`@k8s-bot pull-kubernetes-node-e2e test this`
pull-kubernetes-e2e-gce-etcd3	`246241e`	link	`@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this`

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.

sttts · 2017-06-14T08:10:59Z

pkg/controller/garbagecollector/graph_builder.go

+	}
+}
+
+type monitorSingle struct {


// monitorSingle wraps a cache.Controller and adds another stop channel (in addition to that of Run(stopCh)). It will stop // if either of them is closed.

sttts · 2017-06-14T08:13:28Z

pkg/controller/garbagecollector/graph_builder.go

 }

 func (gb *GraphBuilder) HasSynced() bool {
 	for _, monitor := range gb.monitors {


this misses a lock. Or at the very least it should be documented that it assumes it being taken.

sttts · 2017-06-14T08:16:56Z

pkg/controller/garbagecollector/graph_builder.go

 func (gb *GraphBuilder) Run(stopCh <-chan struct{}) {
+	gb.monitorLock.Lock()
+	defer gb.monitorLock.Unlock()
+	gb.globalStop = stopCh


Something is fishy: NewGarbageCollector calls syncMonitors which depends on globalStop.

sttts · 2017-06-14T08:23:04Z

pkg/controller/garbagecollector/graph_builder.go

 	absentOwnerCache *UIDCache
 }

+type stopMux struct {


this stopMux thing does not make the code easier to understand IMO. Just inline the mux logic into monitorSingle.Run.

It might even be much simpler to have one Go routine waiting on the global lock and shutting down all monitors in a loop. Any special reason that you want to have this logic per monitor?

sttts · 2017-06-14T08:24:02Z

pkg/controller/garbagecollector/metaonly/metaonly.go

 }

-func NewMetadataCodecFactory() serializer.CodecFactory {
+type MetadataCodecFactory struct {


what is this thing doing?

sttts · 2017-06-14T08:24:16Z

pkg/controller/garbagecollector/setup/setup.go

@@ -0,0 +1,70 @@
+/*
+Copyright 2016 The Kubernetes Authors.


Automatic merge from submit-queue (batch tested with PRs 47700, 48464, 48502) Add a refreshing discovery client Introduce a discovery client (implementing `CachedDiscoveryInterface`) which caches discovery information in memory and which can be actively refreshed by the user. This implementation fetches from discovery upon refresh and could later be improved to maintain updates from a watch. Extracted from #47665 and #46000 to help reduce the scope of #48065. ```release-note NONE ```

@lavalamp

Automatic merge from submit-queue (batch tested with PRs 49538, 49708, 47665, 49750, 49528) Enable garbage collection of custom resources 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 ```release-note The garbage collector now supports custom APIs added via CustomeResourceDefinition 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-07-31T18:49:40Z

This PR was superseded by #47665 and can be closed.

lavalamp assigned caesarxuchao and deads2k May 17, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 17, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels May 17, 2017

lavalamp force-pushed the gc-poll-types branch from 4912d6a to 635f71a Compare May 17, 2017 23:08

deads2k reviewed May 18, 2017

View reviewed changes

lavalamp force-pushed the gc-poll-types branch from 635f71a to a5bb216 Compare May 19, 2017 21:24

lavalamp force-pushed the gc-poll-types branch 2 times, most recently from b67ad9b to 31800f9 Compare May 19, 2017 22:34

caesarxuchao reviewed May 19, 2017

View reviewed changes

krmayankk reviewed May 20, 2017

View reviewed changes

Make GC occasionally refresh its discovery information

1240e61

lavalamp force-pushed the gc-poll-types branch from 31800f9 to a6a0b9d Compare May 22, 2017 20:34

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 22, 2017

lavalamp force-pushed the gc-poll-types branch 2 times, most recently from 07e95d7 to 89134ac Compare May 22, 2017 21:08

lavalamp force-pushed the gc-poll-types branch from 89134ac to 69c2ec2 Compare May 22, 2017 22:47

lavalamp force-pushed the gc-poll-types branch from 69c2ec2 to a4f6288 Compare May 22, 2017 23:41

start/stop resource monitors

6cc4044

lavalamp force-pushed the gc-poll-types branch from a4f6288 to 6cc4044 Compare May 22, 2017 23:58

WIP

246241e

sttts reviewed May 29, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 29, 2017

sttts reviewed May 29, 2017

View reviewed changes

sttts mentioned this pull request May 29, 2017

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

Closed

nikhita mentioned this pull request May 29, 2017

integration-test: test for GC nikhita/gsoc-meta-k8s#6

Closed

deads2k mentioned this pull request May 30, 2017

kube-apiextensions-server needs more integration tests #45511

Closed

19 tasks

nikhita mentioned this pull request Jun 4, 2017

Garbage collector should support non-core APIs #44507

Closed

sttts reviewed Jun 14, 2017

View reviewed changes

pkg/controller/garbagecollector/setup/setup.go

@@ -0,0 +1,70 @@

/*

Copyright 2016 The Kubernetes Authors.

Copy link
Copy Markdown

Contributor

sttts Jun 14, 2017

Choose a reason for hiding this comment

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

2017

ironcladlou mentioned this pull request Jun 16, 2017

Enable garbage collection of custom resources #47665

Merged

ironcladlou mentioned this pull request Jun 27, 2017

Add a refreshing discovery client #48148

Closed

ironcladlou mentioned this pull request Jul 5, 2017

Add a refreshing discovery client #48502

Merged

enisoc closed this Jul 31, 2017

Conversation

lavalamp commented May 17, 2017

Uh oh!

k8s-github-robot commented May 17, 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!

deads2k commented May 18, 2017

Uh oh!

lavalamp commented May 18, 2017

Uh oh!

ncdc commented May 19, 2017

Uh oh!

lavalamp commented May 19, 2017

Uh oh!

caesarxuchao May 19, 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!

Choose a reason for hiding this comment

Uh oh!

lavalamp commented May 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikedanese commented May 22, 2017

Uh oh!

lavalamp commented May 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-github-robot commented May 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Jun 1, 2017

Uh oh!

lavalamp commented Jun 1, 2017

Uh oh!

ash2k commented Jun 2, 2017

Uh oh!

ironcladlou commented Jun 12, 2017

Uh oh!

k8s-ci-robot commented Jun 13, 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!

ironcladlou commented Jul 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

caesarxuchao May 19, 2017 •

edited

Loading