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

Local Ephemeral Storage to GA KEP #3361

Closed
wants to merge 1 commit into from
Closed

Local Ephemeral Storage to GA KEP #3361

wants to merge 1 commit into from

Conversation

jingxu97
Copy link
Contributor

@jingxu97 jingxu97 commented Jun 8, 2022

add KEP to promote local ephemeral storage to GA

  • One-line PR description:
  • Issue link:
  • Other comments:

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 8, 2022
@k8s-ci-robot k8s-ci-robot added the kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory label Jun 8, 2022
@k8s-ci-robot k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Jun 8, 2022
@Priyankasaggu11929 Priyankasaggu11929 mentioned this pull request Jun 11, 2022
Closed
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 13, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jingxu97
To complete the pull request process, please assign johnbelamaric, saad-ali after the PR has been reviewed.
You can assign the PR to them by writing /assign @johnbelamaric @saad-ali in a comment when ready.

The full list of commands accepted by this bot can be found 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

johnbelamaric
johnbelamaric reviewed Jun 17, 2022
View reviewed changes
keps/prod-readiness/sig-storage/361.yaml Outdated Show resolved Hide resolved
keps/sig-storage/361-local-ephemeral-storage-isolation/kep.yaml Outdated Show resolved Hide resolved
keps/sig-storage/361-local-ephemeral-storage-isolation/kep.yaml Outdated Show resolved Hide resolved
keps/sig-storage/361-local-ephemeral-storage-isolation/README.md Outdated Show resolved Hide resolved
keps/sig-storage/361-local-ephemeral-storage-isolation/README.md Outdated

### Upgrade / Downgrade Strategy

N/A (Feature is enabled by default since 1.10)
Copy link
Member

Choose a reason for hiding this comment

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

So, if someone has it set to off right now, what happens when they upgrade? Can we provide some instruction for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added it, thanks!

keps/sig-storage/361-local-ephemeral-storage-isolation/README.md Outdated

###### How does this feature react if the API server and/or etcd is unavailable?

- kubelet can continue to monitor disk usage, but pod eviction will be impacted.
Copy link
Member

Choose a reason for hiding this comment

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

Meaning it will not be evicted? Or meaning it will not be rescheduled after eviction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added the details, thanks!

johnbelamaric
johnbelamaric reviewed Jun 18, 2022
View reviewed changes
keps/prod-readiness/sig-storage/361.yaml Outdated
# The KEP must have an approver from the
# "prod-readiness-approvers" group
# of http://git.k8s.io/enhancements/OWNERS_ALIASES
kep-number: 3141
Copy link
Member

Choose a reason for hiding this comment

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

why is this not 361?

@jingxu97
Copy link
Contributor Author

@xing-yang @johnbelamaric thanks for reviewing, I addressed the comments, ptal. Thanks!

johnbelamaric
johnbelamaric reviewed Jun 22, 2022
View reviewed changes
keps/sig-storage/361-local-ephemeral-storage-isolation/README.md
request is set for ephemeral storage, scheduler will not take this ephemeral
storage request into consideration when scheduling pod. If limit is set, pod
will not be evicted due to ephemeral storage usage exceeding limit. sizeLimit
for emptyDir will also not be enforced.
Copy link
Member

Choose a reason for hiding this comment

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

So, if someone had set a limit, but the feature was disabled, the limit would have been ignored in the past. If they upgrade, the feature is now locked on. So, will any existing pods that have limits exceeded be evicted?

If so, that's not a blocker - but it IS something that needs to go in the release notes and highlighted as an upgrade risk.

The answer to "will this affect running workloads" should be YES, in the event that the feature was previously disabled. Imagine upgrading and all of a sudden a bunch of Pods get evicted and rescheduled (hopefully!)...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I updated this part here and also risks section. Is it ok?

@xing-yang
Copy link
Contributor

LGTM. Please fix the CI failures.

@mattcary mattcary mentioned this pull request Jun 22, 2022
Merged
@jingxu97 jingxu97 force-pushed the may/storage branch 5 times, most recently from 1b10f83 to 69e5328 Compare June 23, 2022 03:40
@jingxu97
Local Ephemeral Storage to GA KEP
215b811 
add KEP to promote local ephemeral storage to GA
@jingxu97
Copy link
Contributor Author

@johnbelamaric @xing-yang could you please take another look? I fixed test issue and addressed comments. Thanks!

@mattcary
Copy link
Contributor

#3422 merged.

/close

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2022
@k8s-ci-robot
Copy link
Contributor

@jingxu97: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot
Copy link
Contributor

@mattcary: Closed this PR.

In response to this:

#3422 merged.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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

@johnbelamaric johnbelamaric johnbelamaric left review comments

@xing-yang xing-yang xing-yang left review comments

@saad-ali saad-ali Awaiting requested review from saad-ali

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jingxu97 @k8s-ci-robot @xing-yang @mattcary @johnbelamaric