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

add kubelet configuration crd and controller #249

Merged
merged 1 commit into from Jan 11, 2019

Conversation

@rphillips
Copy link
Contributor

rphillips commented Dec 19, 2018

Purpose

The kubelet config is serialized into the ignition object and expecting a user to deserialize/serialize the config is not an easy process. Create a nicer interface (KubeletConfig CRD) for users to set Kubelet configuration options.

Implementation

  • Adds a kubelet-config-controller go routine to the Machine Config Controller (MCC)
  • Adds a KubeletConfig CRD

Example CRD

apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
metadata:
  name: set-max-pods
spec:
  machineConfigPoolSelector:
    matchLabels:
      custom-kubelet: small-pods
  kubeletConfig:
    maxPods: 100

The kubeletConfig attribute is an instance of KubeletConfiguration.

Todo

  • Docs

Design

Design

/cc @derekwaynecarr @sjenning

if err != nil {
return err
}
err = mergo.Merge(originalKubeConfig, newKubeletConfig.Spec.KubeletConfig, mergo.WithOverride)

This comment has been minimized.

@sjenning

sjenning Dec 19, 2018

Contributor

would be nice to see if there is something already in the vendor tree that can do this vs importing a new dep for this one call

This comment has been minimized.

@sjenning

sjenning Dec 19, 2018

Contributor

actually, this is already in the vendor tree. nm.

@abhinavdahiya

This comment has been minimized.

Copy link
Member

abhinavdahiya commented Dec 19, 2018

@rphillips can you documentation around what this new controller is doing. currently I only see this

`KubeletConfigController` is responsible for wrapping custom Kubelet configurations within a CRD. The available options are documented within the KubeletConfiguration (https://github.com/kubernetes/kubernetes/blob/release-1.11/pkg/kubelet/apis/kubeletconfig/v1beta1/types.go#L45).

Can you expand on

  1. how the CR ties to a machineconfigpool
  2. how is utilizes the exisiting selection and merging of machineconfig per pool
  3. does is manage its own machineconfig object
  4. nice touch would be how to reset to defaults
    and more

It will help everyone see the motivation, expected outcome and then check the code. Thanks a ton! 😇

@cgwalters

This comment has been minimized.

Copy link
Contributor

cgwalters commented Dec 19, 2018

I don't object to this offhand but...I think it'd be easier (and beneficial across the board) to change our spec to support inline data for files, even if Ignition upstream doesn't. It just makes sense in YAML.

@jlebon

This comment has been minimized.

Copy link
Member

jlebon commented Dec 20, 2018

I think it'd be easier (and beneficial across the board) to change our spec to support inline data for files, even if Ignition upstream doesn't

Hmm, could work. Though we'd still need some translation layer in the MCS to convert it back to an Ignition-compatible config.

@cgwalters

This comment has been minimized.

Copy link
Contributor

cgwalters commented Dec 21, 2018

Hmm, could work. Though we'd still need some translation layer in the MCS to convert it back to an Ignition-compatible config.

Yeah, but that's just URL-encoding.

Anyways there are clearly tradeoffs here - a CRD can have validation and things like that. More work and more complexity, but yes bigger benefit.

I'm not an expert in kubelet config, but...if all we're going to end up doing is e.g. proxying documented command line options and not doing much validation in the controller I think the argument weighs more to just using an Ignition fragment to add a drop-in that adds kubelet args directly. Presumably most people changing kubelet flags are going to have some expertise level?

@rphillips rphillips force-pushed the rphillips:feat/add_kubelet_configuration_crd branch from 6679037 to 39db2d4 Jan 7, 2019

@rphillips

This comment has been minimized.

Copy link
Contributor

rphillips commented Jan 7, 2019

Back from vacation. Had a brief chat with @abhinavdahiya... I'm going to tweak this PR to include a label selector for the MachineConfigPool and also change the kubelet config controller to own it's MachineConfig. In addition, I will add more documentation.

@rphillips rphillips force-pushed the rphillips:feat/add_kubelet_configuration_crd branch 2 times, most recently from 71a69ad to f1d4b81 Jan 8, 2019

@rphillips

This comment has been minimized.

Copy link
Contributor

rphillips commented Jan 8, 2019

/retest

2 similar comments
@rphillips

This comment has been minimized.

Copy link
Contributor

rphillips commented Jan 8, 2019

/retest

@rphillips

This comment has been minimized.

Copy link
Contributor

rphillips commented Jan 8, 2019

/retest

@rphillips rphillips changed the title WIP: add kubelet configuration crd and controller add kubelet configuration crd and controller Jan 8, 2019

@rphillips

This comment has been minimized.

Copy link
Contributor

rphillips commented Jan 8, 2019

This is ready for review... Working on patch to add more documentation.

@rphillips

This comment has been minimized.

Copy link
Contributor

rphillips commented Jan 8, 2019

The design has been updated with how the controller works.

@rphillips

This comment has been minimized.

Copy link
Contributor

rphillips commented Jan 8, 2019

/test e2e-aws

@abhinavdahiya

This comment has been minimized.

Copy link
Member

abhinavdahiya commented Jan 8, 2019

The design has been updated with how the controller works.

@rphillips can we find a place for the design in the repo itself.. ? I think it will be useful.

@rphillips

This comment has been minimized.

Copy link
Contributor

rphillips commented Jan 8, 2019

@abhinavdahiya added the selector and the design doc.

@kikisdeliveryservice

This comment has been minimized.

Copy link
Member

kikisdeliveryservice commented Jan 10, 2019

Thanks @rphillips !!!

@sjenning sjenning referenced this pull request Jan 10, 2019

Merged

Update SSH keys via MCD #115

@abhinavdahiya

This comment has been minimized.

Copy link
Member

abhinavdahiya commented Jan 10, 2019

Thanks @rphillips !!!

/hold cancel

@rphillips

This comment has been minimized.

Copy link
Contributor

rphillips commented Jan 10, 2019

/retest

@rphillips rphillips force-pushed the rphillips:feat/add_kubelet_configuration_crd branch from 6a1abec to b229bb6 Jan 10, 2019

@rphillips rphillips force-pushed the rphillips:feat/add_kubelet_configuration_crd branch from b229bb6 to 443e701 Jan 10, 2019

@rphillips

This comment has been minimized.

Copy link
Contributor

rphillips commented Jan 11, 2019

/retest

1 similar comment
@sjenning

This comment has been minimized.

Copy link
Contributor

sjenning commented Jan 11, 2019

/retest

@abhinavdahiya

This comment has been minimized.

Copy link
Member

abhinavdahiya commented Jan 11, 2019

/lgtm

@openshift-ci-robot

This comment has been minimized.

Copy link

openshift-ci-robot commented Jan 11, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, rphillips

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rphillips

This comment has been minimized.

Copy link
Contributor

rphillips commented Jan 11, 2019

/retest

2 similar comments
@abhinavdahiya

This comment has been minimized.

Copy link
Member

abhinavdahiya commented Jan 11, 2019

/retest

@ashcrow

This comment has been minimized.

Copy link
Member

ashcrow commented Jan 11, 2019

/retest

@sjenning

This comment has been minimized.

Copy link
Contributor

sjenning commented Jan 11, 2019

lets hold on the retesting. this is not likely to work until the api.ci registry situation is resolved. i only retested this morning because, for a brief time, it was working 😞

@ashcrow

This comment has been minimized.

Copy link
Member

ashcrow commented Jan 11, 2019

error: unable to create a release: operator "machine-config-operator" failed to map images: image file "/tmp/release-image-0.0.1-2019-01-11-164325543760227/machine-config-operator/image-references" referenced image "machine-config-server" that is not part of the input images
@sjenning

This comment has been minimized.

Copy link
Contributor

sjenning commented Jan 11, 2019

ok looks like it might be cleared now 🤞
/retest

@rphillips

This comment has been minimized.

Copy link
Contributor

rphillips commented Jan 11, 2019

/retest

@sjenning

This comment has been minimized.

Copy link
Contributor

sjenning commented Jan 11, 2019

/test rhel-images

@sjenning

This comment has been minimized.

Copy link
Contributor

sjenning commented Jan 11, 2019

/test e2e-aws

1 similar comment
@rphillips

This comment has been minimized.

Copy link
Contributor

rphillips commented Jan 11, 2019

/test e2e-aws

@sjenning

This comment has been minimized.

Copy link
Contributor

sjenning commented Jan 11, 2019

we are getting timeouts against the AWS API creating cluster resources in terraform 😞

@sjenning

This comment has been minimized.

Copy link
Contributor

sjenning commented Jan 11, 2019

/test e2e-aws

@sjenning

This comment has been minimized.

Copy link
Contributor

sjenning commented Jan 11, 2019

ok, cluster got setup in e2e-aws this time. here's hoping a test doesn't flake 🤞

@openshift-merge-robot openshift-merge-robot merged commit e591a9f into openshift:master Jan 11, 2019

5 checks passed

ci/prow/e2e-aws Job succeeded.
Details
ci/prow/images Job succeeded.
Details
ci/prow/rhel-images Job succeeded.
Details
ci/prow/unit Job succeeded.
Details
tide In merge pool.
Details
@rphillips

This comment has been minimized.

Copy link
Contributor

rphillips commented Jan 11, 2019

Success!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment