move plugin-specific init code into plugin hooks #2217

liggitt · 2019-04-10T00:45:50Z

restores manager.New signature from before Reorganize code to allow conditional enablement of runtimes #2209 was merged
moves plugin install import packages to standalone packages (so that importing the docker package, for example, which is done to satisfy the DockerImages() and DockerInfo() methods, doesn't automatically run the docker collector)
adds a github.com/cadvisor/container/install package that installs all optional container collectors
installs all optional plugins in manager test and in cadvisor#main()
switches plugin registration to be an actual interface, instead of a bare method
moves plugin-specific init code into plugin hooks so callers don't have to recreate it

liggitt · 2019-04-10T00:46:22Z

still testing this, but wanted to get it opened for early review

liggitt · 2019-04-10T00:47:10Z

the first commit simply restores the manager.New signature and logic... the remaining commits make it easy to see the imports getting whittled away

liggitt · 2019-04-10T00:47:36Z

I'll open a WIP PR in kubernetes as well to exercise vendoring and CI there

liggitt · 2019-04-10T01:11:12Z

/retest

dims · 2019-04-10T01:12:14Z

/test pull-cadvisor-e2e

dims · 2019-04-10T01:25:28Z

thanks for taking care of this @liggitt. glad we were able to go back to the old signature!

dims · 2019-04-10T01:25:34Z

/lgtm

liggitt · 2019-04-10T01:33:20Z

kube PR to exercise CI is at kubernetes/kubernetes#76355

derekwaynecarr · 2019-04-10T01:37:09Z

I like this :-)

…

On Tue, Apr 9, 2019 at 9:33 PM Jordan Liggitt ***@***.***> wrote: kube PR to exercise CI is at kubernetes/kubernetes#76355 <kubernetes/kubernetes#76355> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2217 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF8dbPMFT1ST19zD9jOauReK9f71ArAvks5vfT9ogaJpZM4cl44U> .

liggitt · 2019-04-10T02:16:34Z

yay for CI:

ERROR(darwin/386,darwin/amd64,windows/386,windows/amd64) cmd/genkubedocs/gen_kube_docs.go:33:13: could not import k8s.io/kubernetes/cmd/kubelet/app (type-checking package "k8s.io/kubernetes/cmd/kubelet/app" failed (/home/prow/go/src/k8s.io/kubernetes/cmd/kubelet/app/auth.go:34:2: could not import k8s.io/kubernetes/pkg/kubelet/server (type-checking package "k8s.io/kubernetes/pkg/kubelet/server" failed (/home/prow/go/src/k8s.io/kubernetes/pkg/kubelet/server/server.go:36:18: could not import github.com/google/cadvisor/container (type-checking package "k8s.io/kubernetes/vendor/github.com/google/cadvisor/container" failed (/home/prow/go/src/k8s.io/kubernetes/vendor/github.com/google/cadvisor/container/factory.go:83:32: Context not declared by package fs))))))
ERROR(darwin/386,darwin/amd64,windows/386,windows/amd64) cmd/genman/gen_kube_man.go:37:13: could not import k8s.io/kubernetes/cmd/kubelet/app (type-checking package "k8s.io/kubernetes/cmd/kubelet/app" failed (/home/prow/go/src/k8s.io/kubernetes/cmd/kubelet/app/auth.go:34:2: could not import k8s.io/kubernetes/pkg/kubelet/server (type-checking package "k8s.io/kubernetes/pkg/kubelet/server" failed (/home/prow/go/src/k8s.io/kubernetes/pkg/kubelet/server/server.go:36:18: could not import github.com/google/cadvisor/container (type-checking package "k8s.io/kubernetes/vendor/github.com/google/cadvisor/container" failed (/home/prow/go/src/k8s.io/kubernetes/vendor/github.com/google/cadvisor/container/factory.go:83:32: Context not declared by package fs))))))

since the Context type was used in the plugin interface, moved it to the os-neutral fs/types.go file

liggitt · 2019-04-10T02:21:56Z

/retest

dashpole · 2019-04-10T03:15:03Z

I'm out for a couple weeks. If this needs attention soon, @derekwaynecarr or @tallclair will need to review it. Otherwise, I'll take a look when I get back.

liggitt · 2019-04-10T03:29:39Z

thanks for the heads up. yeah, would like to avoid letting the signature change linger in cadvisor master

dims · 2019-04-10T10:09:59Z

/lgtm

/assign @tallclair @derekwaynecarr

liggitt · 2019-04-10T13:11:09Z

kube CI was green with this (kubernetes/kubernetes#76355)

derekwaynecarr · 2019-04-10T15:28:18Z

I am fine with this change.

jaypipes · 2019-04-10T15:39:59Z

container/docker/install/install.go

+	err := container.RegisterPlugin("docker", container.PluginFn{
+		InitializeContextFn: func(context *fs.Context) error {
+			docker.SetTimeout(dockerClientTimeout)
+			// Try to connect to docker indefinitely on startup.


Is the Docker plugin the only one that retries the initialization because of the design of Docker itself? or were you planning on adding this connection retry logic for the other plugins at a later time? I'm just curious why the docker plugin is different from the others... thanks in advance for your answer!

this is simply moving the existing behavior, I'll defer to the cadvisor maintainers on the reason for the current behavior

Ah, cool. Sorry, didn't realize it was the existing behaviour...

historical context: #1871

It was fixing a specific bug with docker. Not sure if the same bug could exist in the other runtimes...

tallclair

LGTM, just a couple stylistic points - take 'em or leave 'em.

container/rkt/install/install.go

container/factory.go

tallclair · 2019-04-10T17:19:30Z

container/docker/install/install.go

+	err := container.RegisterPlugin("docker", container.PluginFn{
+		InitializeContextFn: func(context *fs.Context) error {
+			docker.SetTimeout(dockerClientTimeout)
+			// Try to connect to docker indefinitely on startup.


historical context: #1871

It was fixing a specific bug with docker. Not sure if the same bug could exist in the other runtimes...

…interfaces

This config struct is now used in the plugin InitializeContext method, so it should be defined in the os-neutral types.go file

liggitt · 2019-04-11T13:53:30Z

comments addressed... diff visible at https://github.com/google/cadvisor/compare/655e1c19b6a3e679c0ecff2cb40b57fc2716f09d..f8a73e018b9690a8c6f0c5ce05c367d6bb59bb66

dims · 2019-04-11T13:57:13Z

/lgtm

@tallclair can we please merge this? would be good to try to get a full weekends worth of CI testing in k/k

Restore manager.New signature, initialization code

f7b5092

liggitt mentioned this pull request Apr 10, 2019

WIP - Update to latest cadvisor remove mesos rkt kubernetes/kubernetes#76355

Closed

liggitt added a commit to liggitt/kubernetes that referenced this pull request Apr 10, 2019

TMP: point at branch of google/cadvisor#2217 for testing

b75cc77

liggitt mentioned this pull request Apr 10, 2019

Update to latest cadvisor - cleanup mesos/rkt kubernetes/kubernetes#76291

Merged

liggitt changed the title ~~WIP - move plugin-specific init code into plugin hooks~~ move plugin-specific init code into plugin hooks Apr 10, 2019

derekwaynecarr approved these changes Apr 10, 2019

View reviewed changes

jaypipes reviewed Apr 10, 2019

View reviewed changes

tallclair approved these changes Apr 10, 2019

View reviewed changes

liggitt added 5 commits April 11, 2019 13:50

Move auto-registration to explicit install packages, register plugin …

e24fd90

…interfaces

Add InitializeFSContext hook to plugins

7dc4594

Split rkt context initialization

6757727

Split crio context initialization

e9a44a2

Split docker context initialization

a022fa7

Move fs.Context to types.go

f8a73e0

This config struct is now used in the plugin InitializeContext method, so it should be defined in the os-neutral types.go file

liggitt force-pushed the plugin-init-hooks branch from 655e1c1 to f8a73e0 Compare April 11, 2019 13:51

tallclair merged commit 9db8c7d into google:master Apr 11, 2019

liggitt deleted the plugin-init-hooks branch January 23, 2020 15:47

move plugin-specific init code into plugin hooks #2217

move plugin-specific init code into plugin hooks #2217

Uh oh!

Conversation

liggitt commented Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liggitt commented Apr 10, 2019

Uh oh!

liggitt commented Apr 10, 2019

Uh oh!

liggitt commented Apr 10, 2019

Uh oh!

liggitt commented Apr 10, 2019

Uh oh!

dims commented Apr 10, 2019

Uh oh!

dims commented Apr 10, 2019

Uh oh!

dims commented Apr 10, 2019

Uh oh!

liggitt commented Apr 10, 2019

Uh oh!

derekwaynecarr commented Apr 10, 2019 via email

Uh oh!

liggitt commented Apr 10, 2019

Uh oh!

liggitt commented Apr 10, 2019

Uh oh!

dashpole commented Apr 10, 2019

Uh oh!

liggitt commented Apr 10, 2019

Uh oh!

dims commented Apr 10, 2019

Uh oh!

liggitt commented Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derekwaynecarr commented Apr 10, 2019

Uh oh!

jaypipes Apr 10, 2019

Choose a reason for hiding this comment

Uh oh!

liggitt Apr 10, 2019

Choose a reason for hiding this comment

Uh oh!

jaypipes Apr 10, 2019

Choose a reason for hiding this comment

Uh oh!

tallclair Apr 10, 2019

Choose a reason for hiding this comment

Uh oh!

tallclair left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tallclair Apr 10, 2019

Choose a reason for hiding this comment

Uh oh!

liggitt commented Apr 11, 2019

Uh oh!

dims commented Apr 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

liggitt commented Apr 10, 2019 •

edited

Loading

liggitt commented Apr 10, 2019 •

edited

Loading