Skip to content

migrate to kubelet config file#96

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
rphillips:fixes/migrate_to_kubelet_config
Oct 2, 2018
Merged

migrate to kubelet config file#96
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
rphillips:fixes/migrate_to_kubelet_config

Conversation

@rphillips
Copy link
Copy Markdown
Contributor

@rphillips rphillips commented Sep 26, 2018

Migrates the kubelet to a kubelet.conf file.

The customer typically had to set configuration settings within the kubelet unit file and pass command-line flags to reconfigure the Kubelet. With this patch, the configuration file will allow a subset of options to be set by the customer.

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 26, 2018
@rphillips
Copy link
Copy Markdown
Contributor Author

/cc @derekwaynecarr

Copy link
Copy Markdown
Member

@ashcrow ashcrow left a comment

Choose a reason for hiding this comment

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

I did a quick look through and it looks good.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why this extra line ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed in 0ee9e75.

@rphillips
Copy link
Copy Markdown
Contributor Author

/test unit

@abhinavdahiya
Copy link
Copy Markdown
Contributor

abhinavdahiya commented Sep 28, 2018

/approve

@aaronlevy do you mind taking a quick look

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 28, 2018
@aaronlevy
Copy link
Copy Markdown

To my naive eyes this lgtm. I'm assuming flags that are left over are not represented in the config file?

Might want someone else from pod team / someone more familiar with kubelet mechanics to take a pass - but could always be follow up.

Not strictly related to this, but could also drop --exit-on-lock-contention flag -- I really really hope no one is still trying to self-host the kubelet (we might want to get that deprecated/removed upstream -- and I'll apologize to everyone for asking for it in the first place...)

@rphillips
Copy link
Copy Markdown
Contributor Author

Removed the --exit-on-lock-contention. The rest of the flags have not been migrated into the kubelet configuration upstream yet.

@aaronlevy
Copy link
Copy Markdown

One last question is that it looks like serialize-image-pulls changed from false to true. Just confirming that this is intentional / expected?

@abhinavdahiya
Copy link
Copy Markdown
Contributor

abhinavdahiya commented Oct 2, 2018

/approve cancel

One last question is that it looks like serialize-image-pulls changed from false to true. Just confirming that this is intentional / expected?

@aaronlevy good catch.

@rphillips ?

@openshift-ci-robot openshift-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 2, 2018
@rphillips rphillips force-pushed the fixes/migrate_to_kubelet_config branch from dae4d2a to 459ce44 Compare October 2, 2018 00:52
@rphillips
Copy link
Copy Markdown
Contributor Author

@aaronlevy good catch.

fixed and squashed.

@aaronlevy
Copy link
Copy Markdown

This has been dragging on (apologies) so I can lgtm, but also just noticed that --lock-file=/var/run/lock/kubelet.lock also still exists - which is also part of the self-hosting kubelet behavior we no longer use.

@rphillips rphillips force-pushed the fixes/migrate_to_kubelet_config branch from 459ce44 to b17b7b2 Compare October 2, 2018 20:33
@rphillips
Copy link
Copy Markdown
Contributor Author

@aaronlevy updated to remove the lock

@aaronlevy
Copy link
Copy Markdown

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 2, 2018
@abhinavdahiya
Copy link
Copy Markdown
Contributor

/approve

@openshift-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details 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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 2, 2018
@openshift-merge-robot openshift-merge-robot merged commit 6fe2c38 into openshift:master Oct 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants