KEP-3746 - Initial KEP for specifying root-fs volume size for Windows containers #3951

marosset · 2023-04-10T23:04:28Z

One-line PR description:

Issue link: Specify root-fs volume size for Windows containers #3746

Other comments:

…tainers Signed-off-by: Mark Rossetti <marosset@microsoft.com>

k8s-ci-robot · 2023-04-13T17:31:35Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: marosset
Once this PR has been reviewed and has the lgtm label, please assign wojtek-t for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

keps/prod-readiness/OWNERS
~~keps/sig-windows/OWNERS~~ [marosset]

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

keps/sig-windows/3746-windows-root-fs-volume-size/README.md

jsturtevant · 2023-04-14T20:59:03Z

keps/sig-windows/3746-windows-root-fs-volume-size/README.md

+
+A user wants to run a workload that requires more than 20Gb of disk space on the root-fs volume.
+
+#### Story 2


what about running less than 20gb?

keps/sig-windows/3746-windows-root-fs-volume-size/README.md

jsturtevant · 2023-04-14T20:59:55Z

keps/sig-windows/3746-windows-root-fs-volume-size/README.md

+-->
+
+IO performance when writing to `emptyDir` volumes is worse then writing to container root fs.
+https://github.com/microsoft/Windows-Containers/issues/345


ah I see you mentioned it down here. Maybe could link to the various issues we've gotten related to this?

liggitt · 2023-05-02T16:12:13Z

keps/sig-windows/3746-windows-root-fs-volume-size/README.md

+  - name: container1
+    resources:
+      limits:
+        storage: 30Gi


agree storage is ambiguous in how it interacts with mounted volumes... would a more explicit rootfs-storage be better?

also, is there a reason this would not apply to both linux and windows?

what was the resolution of the name storage being ambiguous?

liggitt · 2023-05-02T17:19:27Z

keps/sig-windows/3746-windows-root-fs-volume-size/README.md

+  CRI or CNI may require updating that component before the kubelet.
+-->
+
+N/A


I'm guessing this would be added to a lot of the places we include ephemeral storage, in terms of validation.

See use of EphemeralStorage in https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/helper/helpers.go, which fans out to helper methods like IsStandardContainerResourceName, IsStandardResourceName, IsStandardQuotaResourceName, etc.

Any validation of resources and allowed values for resources would need to consider skew so we don't persist data an n-1 API server or controller would choke on.

Updated - PTAL

not seeing the update, still looks like N/A

I left a comment at #3951 (comment) with a sketch of how to relax validation safely and be sure you are considering the impact on all callers

Signed-off-by: Mark Rossetti <marosset@microsoft.com>

marosset · 2023-05-22T16:59:00Z

/api-review

liggitt · 2023-05-24T20:10:01Z

keps/sig-windows/3746-windows-root-fs-volume-size/README.md

+  - name: container1
+    resources:
+      limits:
+        storage: 30Gi


what was the resolution of the name storage being ambiguous?

liggitt · 2023-05-24T20:16:09Z

keps/sig-windows/3746-windows-root-fs-volume-size/README.md

+### API server updates
+
+Update API server validation to allow `resources.limits.storage` to be specified on containers. While the feature is in development this validation will be gated on by a feature flag.
+The relevant code is in [ValidateContainerResourceName](https://github.com/kubernetes/kubernetes/blob/ad18954259eae3db51bac2274ed4ca7304b923c4/pkg/apis/core/v1/validation/validation.go#L73-L86)


Relaxing validation has to be done over two releases. Otherwise, we allow data to be persisted that will be considered invalid by the previous release of the API server (which could still be running in a multi-server cluster during rolling upgrade).

I would suggest doing the following:

in pkg/apis/core/validation/validation.go, add an AllowContainerStorageResource bool field to PodValidationOptions

make sure that options struct is passed through all validation functions down to ValidateContainerResourceName

this will reveal lots of call paths that lead to this function, all of which have to consider whether to allow the relaxed data or not

in ValidateContainerResourceName, only allow the relaxed validation if the AllowContainerStorageResource field is true

in GetValidationOptionsFromPodSpecAndMeta, compute the validation options based on the new and old object

in this release

on create (new object is non-nil, old object is nil), set the AllowContainerStorageResource field to true only if the feature gate is enabled

on update, set the AllowContainerStorageResource field to true only if the old object already has a container using the storage resource or the feature is enabled

in a future release, enable the feature gate

in an even more future release, graduate the feature gate and remove the option check

liggitt · 2023-05-24T20:16:38Z

keps/sig-windows/3746-windows-root-fs-volume-size/README.md

+  CRI or CNI may require updating that component before the kubelet.
+-->
+
+N/A


not seeing the update, still looks like N/A

jpbetz · 2023-06-12T17:47:49Z

keps/sig-windows/3746-windows-root-fs-volume-size/README.md

+-->
+
+Yes
+If the feature is disabled, then new pods admitted into the cluster and new containers started will always be created with a root-fs volume size of 20Gb (default for Windows containers) instead of the size specified in the Pod spec.


Just to clarify, any existing pods that have been admitted with a root-fs size other than 20Gb will continue to run at the configured size even if the feature is turned off?

Yes, I would expect a pod that has been admitted with a root-fs size other than 20Ggb would continue to run at the configures size after the feature was turned off.

Would you add this detail to the KEP? Thanks!

k8s-ci-robot · 2023-06-14T00:12:09Z

@marosset: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-enhancements-test	`885c570`	link	true	`/test pull-enhancements-test`
pull-enhancements-verify	`885c570`	link	true	`/test pull-enhancements-verify`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

johnbelamaric

Please update with me as approver, and make the addition Joe asked for and I can approve PRR.

johnbelamaric · 2023-06-15T16:39:19Z

keps/prod-readiness/sig-windows/3746.yaml

+# of http://git.k8s.io/enhancements/OWNERS_ALIASES
+kep-number: 3503
+alpha:
+  approver: TBD


please but me here

k8s-triage-robot · 2024-01-20T01:05:00Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2024-02-19T01:11:13Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2024-03-20T01:59:02Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2024-03-20T01:59:06Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/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.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 10, 2023

k8s-ci-robot requested a review from claudiubelu April 10, 2023 23:04

k8s-ci-robot added the kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory label Apr 10, 2023

k8s-ci-robot requested a review from jsturtevant April 10, 2023 23:04

k8s-ci-robot added sig/windows Categorizes an issue or PR as relevant to SIG Windows. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 10, 2023

marosset changed the title ~~WIPKEP-3747 - Initial KEP for specifying root-fs volume size for Windows containers~~ WIP KEP-3747 - Initial KEP for specifying root-fs volume size for Windows containers Apr 10, 2023

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 10, 2023

marosset mentioned this pull request Apr 10, 2023

Specify root-fs volume size for Windows containers #3746

Closed

4 tasks

3747 - Initial KEP for specifying root-fs volume size for Windows con…

0c65a05

…tainers Signed-off-by: Mark Rossetti <marosset@microsoft.com>

marosset force-pushed the 3746-windows-scratch-volume-size-initial-kep branch from 0c2db81 to 0c65a05 Compare April 13, 2023 17:31

k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 13, 2023

jsturtevant reviewed Apr 14, 2023

View reviewed changes

liggitt reviewed May 2, 2023

View reviewed changes

KEP updates

885c570

Signed-off-by: Mark Rossetti <marosset@microsoft.com>

marosset changed the title ~~WIP KEP-3747 - Initial KEP for specifying root-fs volume size for Windows containers~~ KEP-3747 - Initial KEP for specifying root-fs volume size for Windows containers May 17, 2023

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 May 17, 2023

liggitt reviewed May 24, 2023

View reviewed changes

Atharva-Shinde changed the title ~~KEP-3747 - Initial KEP for specifying root-fs volume size for Windows containers~~ KEP-3746 - Initial KEP for specifying root-fs volume size for Windows containers May 25, 2023

jpbetz reviewed Jun 12, 2023

View reviewed changes

johnbelamaric reviewed Jun 15, 2023

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 20, 2024

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 19, 2024

k8s-ci-robot added the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Feb 19, 2024

k8s-ci-robot closed this Mar 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-3746 - Initial KEP for specifying root-fs volume size for Windows containers #3951

KEP-3746 - Initial KEP for specifying root-fs volume size for Windows containers #3951

marosset commented Apr 10, 2023 •

edited

Loading

k8s-ci-robot commented Apr 13, 2023

jsturtevant Apr 14, 2023

jsturtevant Apr 14, 2023

liggitt May 2, 2023

liggitt May 24, 2023

liggitt May 2, 2023

marosset May 17, 2023

liggitt May 24, 2023

liggitt May 24, 2023

marosset commented May 22, 2023

liggitt May 24, 2023

liggitt May 24, 2023

liggitt May 24, 2023

jpbetz Jun 12, 2023

marosset Jun 14, 2023

jpbetz Jun 14, 2023

k8s-ci-robot commented Jun 14, 2023

johnbelamaric left a comment

johnbelamaric Jun 15, 2023

k8s-triage-robot commented Jan 20, 2024

k8s-triage-robot commented Feb 19, 2024

k8s-triage-robot commented Mar 20, 2024

k8s-ci-robot commented Mar 20, 2024


		A user wants to run a workload that requires more than 20Gb of disk space on the root-fs volume.

		#### Story 2

KEP-3746 - Initial KEP for specifying root-fs volume size for Windows containers #3951

KEP-3746 - Initial KEP for specifying root-fs volume size for Windows containers #3951

Conversation

marosset commented Apr 10, 2023 • edited Loading

k8s-ci-robot commented Apr 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marosset commented May 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 14, 2023

johnbelamaric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-triage-robot commented Jan 20, 2024

k8s-triage-robot commented Feb 19, 2024

k8s-triage-robot commented Mar 20, 2024

k8s-ci-robot commented Mar 20, 2024

marosset commented Apr 10, 2023 •

edited

Loading