Refactor to simplify the hard-traveled path of the KubeletConfiguration object #29216

mtaufen · 2016-07-19T17:30:48Z

There are two main goals of this PR:

Make NewMainKubelet take KubeletConfiguration and KubeletDeps as its only arguments.
Finally eliminate the legacy KubeletConfig type.

Why am I doing this?

Long story short, I started adding an endpoint to the Kubelet to display the current config that the Kubelet was running with, and I realized a few things:

There were so many transformations to the configuration, in so many different places, before it was used that I wasn't confident the values initially passed in on the KubeletConfiguration would be the correct values to report by the time someone used the endpoint to check on them.
Trying to reconstruct a KubeletConfiguration object from a mix of the Kubelet object and the legacy KubeletConfig object would just add to the mess (not to mention maintenance burden), and it would be much easier if we passed the KubeletConfiguration all the way down to where we construct the Kubelet object, and then just store a reference to the KubeletConfiguration object on the Kubelet for later retrieval.
My hope is that by eliminating unnecessary internal transformations to the config information, and by consolidating the remaining ones in a single place (NewMainKubelet), we can have a much clearer understanding of what happens to the config before it makes it to the Kubelet object, and also a better ability to report up-to-date information on the status of the Kubelet.

So I started cleaning things up :-).

Discussion points

It was relatively simple to get NewMainKubelet to just take the legacy KubeletConfig as its only argument, because most of its arguments were just passing through KubeletConfig fields or passing information that was generated solely from KubeletConfig fields.

Completely eliminating the legacy KubeletConfig type has been more difficult, because the fields of the KubeletConfiguration do not have a one-to-one relationship with the fields of the KubeletConfig. While I was able to eliminate many of the KubeletConfig fields, I'm starting to get into the nontrivial stuff and I'd like to get a discussion started on what should happen with the remaining fields (pending cherry-picking notwithstanding).

On my kconf-refactor branch, the legacy KubeletConfig object is down to the following 27 fields (from the initial 93). I'd really appreciate any guidance people have on what should happen with these fields.

type KubeletConfig struct {
    Auth                    server.AuthInterface
    AutoDetectCloudProvider bool
    Builder                 KubeletBuilder
    CAdvisorInterface       cadvisor.Interface
    Cloud                   cloudprovider.Interface
    ContainerManager        cm.ContainerManager
    DockerClient            dockertools.DockerInterface
    EventClient             *clientset.Clientset
    Hostname                string
    HostNetworkSources      []string
    HostPIDSources          []string
    HostIPCSources          []string
    KubeClient              *clientset.Clientset
    Mounter                 mount.Interface
    NetworkPlugins          []network.NetworkPlugin
    NodeName                string
    OOMAdjuster             *oom.OOMAdjuster
    OSInterface             kubecontainer.OSInterface
    PodConfig               *config.PodConfig
    Recorder                record.EventRecorder
    Reservation             kubetypes.Reservation
    TLSOptions              *server.TLSOptions
    Writer                  kubeio.Writer
    VolumePlugins           []volume.VolumePlugin
    EvictionConfig          eviction.Config
    ContainerRuntimeOptions []kubecontainer.Option
    Options                 []Option
}

The patterns I've seen so far with respect to eliminating KubeletConfig fields may be of some help:

Some fields could just be eliminated, because they were either the same on KubeletConfiguration or just a typecast away from being the same.
Some fields from KubeletConfiguration just ended up in substructures of KubeletConfig; it was easy to just remove those substructure fields from KubeletConfig and construct them using local vars in NewMainKubelet instead.
Some fields, e.g. Runonce, were able to move into the KubeletConfiguration.

P.S. Part of the way I'm making the transition is by adding an extra KubeletConfiguration argument to functions that originally took a KubeletConfig, and field-by-field, switching those functions over to using information from the KubeletConfiguration. Once the KubeletConfig is gone, I'll remove the KubeletConfig argument, and the transition will be complete.

Final note:
Please try to keep in mind that this is not a general Kubelet cleanup effort, it is just me cleaning things up that are directly in the path of what I'm trying to do. Let's keep this focused on cleanup related to the path that config takes on it's way to the Kubelet.

Release note:

Removed Flags
- Removes the --auth-path flag. This has been deprecated in favor of --kubeconfig for two releases.

This change is

mtaufen · 2016-07-19T17:31:48Z

cc @vishh @mikedanese @timstclair

philips · 2016-07-19T18:17:38Z

cc @aaronlevy @derekparker @pbx0

philips · 2016-07-19T18:20:56Z

This is part of #27980.

googlebot · 2016-07-20T22:05:05Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

k8s-bot · 2016-08-24T20:52:59Z

GCE e2e build/test passed for commit 652024f1912559b207abcb6997edbc2e25162ae4.

mtaufen · 2016-08-24T21:34:26Z

@k8s-bot test this issue #31262

k8s-bot · 2016-08-24T22:14:32Z

GCE e2e build/test passed for commit 652024f1912559b207abcb6997edbc2e25162ae4.

mtaufen · 2016-08-24T22:30:31Z

@k8s-bot unit test this please

k8s-github-robot · 2016-08-24T23:52:18Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-08-25T00:40:40Z

GCE e2e build/test passed for commit 652024f1912559b207abcb6997edbc2e25162ae4.

k8s-github-robot · 2016-08-25T01:37:23Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-08-25T02:25:28Z

GCE e2e build/test passed for commit 652024f1912559b207abcb6997edbc2e25162ae4.

k8s-github-robot · 2016-08-25T13:32:43Z

@mtaufen PR needs rebase

This refactor removes the legacy KubeletConfig object and adds a new KubeletDeps object, which contains injected runtime objects and separates them from static config. It also reduces NewMainKubelet to two arguments: a KubeletConfiguration and a KubeletDeps. Some mesos and kubemark code was affected by this change, and has been modified accordingly. And a few final notes: KubeletDeps: KubeletDeps will be a temporary bin for things we might consider "injected dependencies", until we have a better dependency injection story for the Kubelet. We will have to discuss this eventually. RunOnce: We will likely not pull new KubeletConfiguration from the API server when in runonce mode, so it doesn't make sense to make this something that can be configured centrally. We will leave it as a flag-only option for now. Additionally, it is increasingly looking like nobody actually uses the Kubelet's runonce mode anymore, so it may be a candidate for deprecation and removal.

It has been deprecated for two releases (1.2 and 1.3).

mtaufen · 2016-08-25T18:02:03Z

@vishh Bumping priority due to frequent rebases.

k8s-github-robot · 2016-08-25T18:09:25Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @mtaufen @vishh

vishh · 2016-08-25T18:18:24Z

Ack. There are PRs that are waiting to be rebased on top of this. So bumping priority makes sense.

k8s-bot · 2016-08-25T18:41:12Z

GCE e2e build/test passed for commit 7ae1458.

k8s-github-robot · 2016-08-25T19:38:39Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-08-25T20:17:19Z

GCE e2e build/test passed for commit 7ae1458.

k8s-github-robot · 2016-08-25T20:17:22Z

Automatic merge from submit-queue

vishh · 2016-08-25T20:31:08Z

Finally!!!

On Thu, Aug 25, 2016 at 1:18 PM, Kubernetes Bot notifications@github.com
wrote:

GCE e2e build/test passed for commit 7ae1458
7ae1458
.

Test Results
https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/29216/kubernetes-pull-build-test-e2e-gce/56033

Build Log
http://pr-test.k8s.io/29216/kubernetes-pull-build-test-e2e-gce/56033/build-log.txt

Test Artifacts
https://console.developers.google.com/storage/browser/kubernetes-jenkins/pr-logs/pull/29216/kubernetes-pull-build-test-e2e-gce/56033/artifacts/

Internal Jenkins Results
http://goto.google.com/prkubekins/job/kubernetes-pull-build-test-e2e-gce//56033

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#29216 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGvIKIE665GoMYcBPx0N7b5YW2Wn8gCZks5qjfiDgaJpZM4JP-G6
.

These are long gone, removed in 2016: * AuthPath removal: kubernetes#29216 * Flag removal: kubernetes#40048 This removes the remnants from clientcmd, mostly in the commends describing how the configuration is loaded. Since getServerIdentificationPartialConfig can no longer fail (it copies fields from one struct to another), this drops the error return, along with the error handling in the caller. Signed-off-by: Stephen Kitt <skitt@redhat.com>

These are long gone, removed in 2016: * AuthPath removal: kubernetes#29216 * Flag removal: kubernetes#40048 This removes the remnants from clientcmd, mostly in the comments describing how the configuration is loaded. Since getServerIdentificationPartialConfig can no longer fail (it copies fields from one struct to another), this drops the error return, along with the error handling in the caller. Signed-off-by: Stephen Kitt <skitt@redhat.com>

mtaufen added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jul 19, 2016

googlebot added the cla: yes label Jul 19, 2016

mtaufen added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. labels Jul 19, 2016

k8s-github-robot assigned dchen1107 Jul 19, 2016

mtaufen assigned mtaufen and vishh and unassigned dchen1107 Jul 19, 2016

mtaufen assigned mikedanese, timstclair, Random-Liu, yujuhong, dubstack, mnshaw and ronnielai Jul 19, 2016

mtaufen force-pushed the kconf-refactor branch from eeaf823 to 6567b05 Compare July 20, 2016 16:46

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2016

mtaufen force-pushed the kconf-refactor branch 4 times, most recently from 118c815 to 0ad2914 Compare July 20, 2016 22:05

googlebot removed the cla: yes label Jul 20, 2016

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

mtaufen added 3 commits August 25, 2016 10:57

Only try to connect to docker if we're actually using the docker runtime

0afaeab

Remove the deprecated AuthPath from KubeletServer

7ae1458

It has been deprecated for two releases (1.2 and 1.3).

mtaufen force-pushed the kconf-refactor branch from 652024f to 7ae1458 Compare August 25, 2016 17:57

mtaufen added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Aug 25, 2016

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 25, 2016

vishh added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 25, 2016

k8s-github-robot merged commit df54a28 into kubernetes:master Aug 25, 2016

skitt mentioned this pull request Jul 24, 2024

Drop references to auth-path and kubernetes_auth #125917

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to simplify the hard-traveled path of the KubeletConfiguration object #29216

Refactor to simplify the hard-traveled path of the KubeletConfiguration object #29216

mtaufen commented Jul 19, 2016 •

edited

Loading

mtaufen commented Jul 19, 2016 •

edited

Loading

philips commented Jul 19, 2016

philips commented Jul 19, 2016

googlebot commented Jul 20, 2016

k8s-bot commented Aug 24, 2016

mtaufen commented Aug 24, 2016

k8s-bot commented Aug 24, 2016

mtaufen commented Aug 24, 2016

k8s-github-robot commented Aug 24, 2016

k8s-bot commented Aug 25, 2016

k8s-github-robot commented Aug 25, 2016

k8s-bot commented Aug 25, 2016

k8s-github-robot commented Aug 25, 2016

mtaufen commented Aug 25, 2016

k8s-github-robot commented Aug 25, 2016

vishh commented Aug 25, 2016

k8s-bot commented Aug 25, 2016

k8s-github-robot commented Aug 25, 2016

k8s-bot commented Aug 25, 2016

k8s-github-robot commented Aug 25, 2016

vishh commented Aug 25, 2016

Refactor to simplify the hard-traveled path of the KubeletConfiguration object #29216

Refactor to simplify the hard-traveled path of the KubeletConfiguration object #29216

Conversation

mtaufen commented Jul 19, 2016 • edited Loading

There are two main goals of this PR:

Why am I doing this?

Discussion points

mtaufen commented Jul 19, 2016 • edited Loading

philips commented Jul 19, 2016

philips commented Jul 19, 2016

googlebot commented Jul 20, 2016

k8s-bot commented Aug 24, 2016

mtaufen commented Aug 24, 2016

k8s-bot commented Aug 24, 2016

mtaufen commented Aug 24, 2016

k8s-github-robot commented Aug 24, 2016

k8s-bot commented Aug 25, 2016

k8s-github-robot commented Aug 25, 2016

k8s-bot commented Aug 25, 2016

k8s-github-robot commented Aug 25, 2016

mtaufen commented Aug 25, 2016

k8s-github-robot commented Aug 25, 2016

vishh commented Aug 25, 2016

k8s-bot commented Aug 25, 2016

k8s-github-robot commented Aug 25, 2016

k8s-bot commented Aug 25, 2016

k8s-github-robot commented Aug 25, 2016

vishh commented Aug 25, 2016

mtaufen commented Jul 19, 2016 •

edited

Loading

mtaufen commented Jul 19, 2016 •

edited

Loading