Skip to content
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

ExperimentalAllowedUnsafeSysctls has moved to AllowedUnsafeSysctls in k8s 1.11 #6179

Merged
merged 2 commits into from Dec 7, 2018

Conversation

rdrgmnzs
Copy link
Contributor

@rdrgmnzs rdrgmnzs commented Dec 7, 2018

ExperimentalAllowedUnsafeSysctls was promoted to beta and renamed to AllowedUnsafeSysctls in k8s 1.11.

For reference: kubernetes/kubernetes#63717

/assign @justinsb @mikesplain

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 7, 2018
if kubeletConfig.ExperimentalAllowedUnsafeSysctls != nil {
// The ExperimentalAllowedUnsafeSysctls flag was renamed in k/k #63717
if b.IsKubernetesGTE("1.11") {
glog.V(1).Info("ExperimentalAllowedUnsafeSysctls was renamed in k8s 1.11+, please use AllowedUnsafeSysctls instead.")
Copy link
Member

@justinsb justinsb Dec 7, 2018

Choose a reason for hiding this comment

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

I like this - it feels like mapping it is the correct thing to do here.

But - it's in nodeup, and most people don't see that.

I'm wondering if we should add this to the configuration generation in kops itself. (It's also better to have less logic in nodeup generally - nodeup is ideally dumb, though in practice there are lots of places where it can't be).

We could have a method which does these flags removed / renamed checks, and remaps / warns / errors as appropriate. In this case remapping seems like the right call, but I think we should actively warn the user, to encourage them to change their configuration

@justinsb
Copy link
Member

justinsb commented Dec 7, 2018

Thanks @rdrgmnzs

As per my comment, I think we should think about putting this warning somewhere users will see it every time they use kops with the old flag. WDYT?

/lgtm
(but I'll hold approval until I get your input on where you think it should live)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 7, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, rdrgmnzs

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 7, 2018
@k8s-ci-robot k8s-ci-robot merged commit 7c47a91 into kubernetes:master Dec 7, 2018
@rdrgmnzs
Copy link
Contributor Author

rdrgmnzs commented Dec 7, 2018

@justinsb yeah, I like the idea of having this in Kops instead of nodeup and making the warning more visible. Will work on that and submit a patch.

Weird that the bot already landed this.

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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants