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.

Sign up for GitHub

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

Jump to bottom

disable localStorageCapacityIsolation for rootless in >= v1.25.0-alpha.3.440+0064010cddfa00 #2846

Merged
merged 1 commit into from
Aug 4, 2022
Merged

disable localStorageCapacityIsolation for rootless in >= v1.25.0-alpha.3.440+0064010cddfa00 #2846

merged 1 commit into from
Aug 4, 2022

Conversation

BenTheElder
Copy link
Member

required for kubernetes/kubernetes#111513 + rootless, see discussion from here for context kubernetes/enhancements#361 (comment)

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 1, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 1, 2022
@BenTheElder BenTheElder force-pushed the localstorageisolationrootless branch from c744441 to 05f1472 Compare August 1, 2022 05:49
@BenTheElder
Copy link
Member Author

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kind/2846/pull-kind-e2e-kubernetes/1553981391253803008

W0801 06:08:06.638365     249 initconfiguration.go:305] error unmarshaling configuration schema.GroupVersionKind{Group:"kubelet.config.k8s.io", Version:"v1beta1", Kind:"KubeletConfiguration"}: strict decoding error: unknown field "localStorageCapacityIsolation"
W0801 06:08:06.638942     249 configset.go:177] error unmarshaling configuration schema.GroupVersionKind{Group:"kubelet.config.k8s.io", Version:"v1beta1", Kind:"KubeletConfiguration"}: strict decoding error: unknown field "localStorageCapacityIsolation"
I0801 06:08:06.645224     249 common.go:128] WARNING: tolerating control plane version v1.25.0-alpha.3.348+d046a58de435a0, assuming that k8s version 1.24.0 is not released yet

Aside: Looks like I just spotted a kubeadm bug with that warning, should say 1.25.0 is not released yet.

Will circle back to that in the morning.

Next up: making this PR test from kubernetes/kubernetes#111513

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 1, 2022
@BenTheElder
Copy link
Member Author

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kind/2846/pull-kind-e2e-kubernetes/1553988593934929920 is testing with the kubernetes PR branch (no merging, since it needs rebasing anyhow, just whatever code is in that branch)

@BenTheElder
Copy link
Member Author

TODO next: temporarily patch the github actions with rootless CI to clone the Kubernetes PR

@BenTheElder BenTheElder force-pushed the localstorageisolationrootless branch 3 times, most recently from 7866500 to 3e4d140 Compare August 1, 2022 17:12
@BenTheElder BenTheElder mentioned this pull request Aug 1, 2022
Closed
@BenTheElder BenTheElder force-pushed the localstorageisolationrootless branch 9 times, most recently from 3b6bd98 to c5e390a Compare August 2, 2022 00:34
@BenTheElder
Copy link
Member Author

I have the build and test in rootless-on-vagrant-on-macOS "working" but it times out.

I think we're going to need to build an image for kubernetes/kubernetes#111513, push it somewhere, and test from that, unfortunately.

@BenTheElder
Copy link
Member Author

OK we have a rootless CI result with the new option in your PR @jingxu97 https://github.com/kubernetes-sigs/kind/runs/7622572978?check_suite_focus=true

we should double check that it fails without the option set.

This was referenced Aug 2, 2022
Merged
Closed
@BenTheElder BenTheElder force-pushed the localstorageisolationrootless branch 2 times, most recently from 8d42c23 to 4e35e9b Compare August 2, 2022 17:30
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 2, 2022
@BenTheElder
Copy link
Member Author

dropped testing commits, will test further in #2851

we should not merge this until kubernetes/kubernetes#111513 merges, since the new field is not guaranteed yet.
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 2, 2022
@BenTheElder BenTheElder mentioned this pull request Aug 2, 2022
Merged
@BenTheElder
Copy link
Member Author

tested and confirmed we still need this in #2851 (comment)

@BenTheElder BenTheElder mentioned this pull request Aug 2, 2022
Merged
@BenTheElder BenTheElder force-pushed the localstorageisolationrootless branch from 4e35e9b to a54a8da Compare August 3, 2022 23:16
@BenTheElder BenTheElder changed the title [WIP] disable localStorageCapacityIsolation for rootless in > v1.25.0-alpha.2 disable localStorageCapacityIsolation for rootless in >= v1.25.0-alpha.3.505+442574f3a7321f Aug 3, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 3, 2022
@BenTheElder BenTheElder force-pushed the localstorageisolationrootless branch from a54a8da to 4f47894 Compare August 3, 2022 23:18
@BenTheElder BenTheElder changed the title disable localStorageCapacityIsolation for rootless in >= v1.25.0-alpha.3.505+442574f3a7321f disable localStorageCapacityIsolation for rootless in >= v1.25.0-alpha.3.440+0064010cddfa00 Aug 3, 2022
@BenTheElder BenTheElder force-pushed the localstorageisolationrootless branch from 4f47894 to 968c842 Compare August 3, 2022 23:19
@BenTheElder
Copy link
Member Author

/assign @aojea
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 4, 2022
AkihiroSuda
AkihiroSuda reviewed Aug 4, 2022
View reviewed changes
pkg/cluster/internal/kubeadm/config.go
@@ -537,6 +539,7 @@ evictionHard:
{{ range $key := .SortedFeatureGateKeys }}
"{{ $key }}": {{ index $.FeatureGates $key }}
{{end}}{{end}}
localStorageCapacityIsolation: {{if .DisableLocalStorageCapacityIsolation}}false{{else}}true{{end}}
Copy link
Member

@AkihiroSuda AkihiroSuda Aug 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
localStorageCapacityIsolation: {{if .DisableLocalStorageCapacityIsolation}}false{{else}}true{{end}}
{{if .DisableLocalStorageCapacityIsolation}}localStorageCapacityIsolation: false{{end}}

This may look cleaner and less likely to hit an issue with an older version of kubelet that is unaware of localStorageCapacityIsolation

Copy link
Contributor

Choose a reason for hiding this comment

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

agree

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree on cleaner but the unknown fields will never reach kubelet, kubeadm will drop any unknown field because it defaults kubelet config itself. We’ve run this patch with and without the Kubernetes patch.

@aojea
Copy link
Contributor

aojea commented Aug 4, 2022

/lgtm

I prefer to be explicit though and use Akihiro's option

#2846 (comment)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2022
@k8s-ci-robot k8s-ci-robot merged commit 36c6282 into kubernetes-sigs:main Aug 4, 2022
@BenTheElder
Copy link
Member Author

Will follow-up PR when I get back to a computer after the morning dog walk 🙃, I agree, would’ve been fine to update before merge 😅

@BenTheElder BenTheElder deleted the localstorageisolationrootless branch August 4, 2022 14:49
@BenTheElder BenTheElder mentioned this pull request Aug 4, 2022
Merged
@BenTheElder
Copy link
Member Author

ok that took me a bit but filed #2861

This was referenced Aug 9, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

@amwat amwat Awaiting requested review from amwat

@aojea aojea Awaiting requested review from aojea

Assignees

@aojea aojea

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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@BenTheElder @k8s-ci-robot @aojea @AkihiroSuda