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

KEP-4639: update to most recent implementation state #4897

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

KEP-4639: update to most recent implementation state #4897

wants to merge 1 commit into from
+166 −71

Conversation

haircommander
Copy link
Contributor

  • Other comments: @saschagrunert is on leave this week and is the primary author of this KEP, I am opening this in his stead to get it on PRR radar, but it may change once he returns.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 2, 2024
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 2, 2024
@haircommander haircommander mentioned this pull request Oct 2, 2024
Open
9 tasks
sudo-bmitch
sudo-bmitch suggested changes Oct 2, 2024
View reviewed changes
Copy link
Contributor

@sudo-bmitch sudo-bmitch left a comment

Choose a reason for hiding this comment

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

Please remove references to OCI Artifacts from the KEP since those are explicitly not supported.

@haircommander
Copy link
Contributor Author

I would defer to @saschagrunert on that one. From my perspective, since the kubelet is specifying a registry path, there's no reason from kube's perspective we'd limit it to a just images. We currently don't support artifacts because neither containerd nor CRI-O support them with this. That could be subject to change (and as I understand it there are conversations happening to change it), but from the kubelet's perspective it's just a string that the runtimes interpret

@sudo-bmitch
Copy link
Contributor

There are no implementations that support Artifacts, and discussions for how they could be supported were pushed off so there's no way for them to be implemented. OCI Artifacts are implicitly listed as a non-goal in the KEP. But because of the title, we've been seeing people complaining to OCI about why our spec is wrong.

kannon92
kannon92 reviewed Oct 3, 2024
View reviewed changes
keps/sig-node/4639-oci-volume-source/README.md Outdated
@@ -779,6 +783,7 @@ https://storage.googleapis.com/k8s-triage/index.html
-->

- <test>: <link to test coverage>
SIG node does not typically write e2e tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SIG node does not typically write e2e tests
SIG node does not typically write integration tests

TBH I would probably expand on this. We don't write integration tests because we kubelet is covered by the e2e_node suite. We as a sig would love to write more integration tests but we are limiting in what we can do.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
SIG node does not typically write e2e tests
Integration tests for the kubelet are not existing yet and will be covered by extending the available e2e test suites.

Copy link
Member

Choose a reason for hiding this comment

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

Sascha's look more correct, this is not about what one SIG does, is just because this feature does not require them or are covered with e2e ... if SIG node has to write any API object related feature must probably it must require integration tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've seen some KEPs stating that "integration tests don't add additional test coverage and thus are not used". I think that this can be true: unit tests can provide detailed regression testing by covering all lines and edge cases, while E2E tests exercise the integration (sic!) into a real cluster.

The actual integration tests then fall somewhere in the middle and may be useful to exercise scenarios that are too expensive to test with a full cluster, but I wouldn't say that they are required.

Also, e2e_node is similar to what test/integration is for other code: using a fake apiserver to walk through scenarios without needing a full cluster.

Copy link
Contributor

Choose a reason for hiding this comment

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

We as a sig would love to write more integration tests but we are limiting in what we can do.

Careful here: if lack of resources is the reason for not writing integration tests when they would be desirable for a feature, then development of that feature needs to slow down. Please don't skip testing to deliver features faster.

saschagrunert
saschagrunert reviewed Oct 7, 2024
View reviewed changes
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

Thank you for opening that PR, if we can agree on the scope (beta = subPath support) then I'm happy to move forward with that.

cc @carlory

keps/sig-node/4639-oci-volume-source/README.md Outdated
@@ -779,6 +783,7 @@ https://storage.googleapis.com/k8s-triage/index.html
-->

- <test>: <link to test coverage>
SIG node does not typically write e2e tests
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
SIG node does not typically write e2e tests
Integration tests for the kubelet are not existing yet and will be covered by extending the available e2e test suites.

keps/sig-node/4639-oci-volume-source/README.md Outdated Show resolved Hide resolved
keps/sig-node/4639-oci-volume-source/README.md Outdated Show resolved Hide resolved
@haircommander haircommander force-pushed the volume-beta-update branch 3 times, most recently from 6da15eb to 6eef869 Compare October 7, 2024 18:42
@saschagrunert
Copy link
Member

/lgtm

/unhold

@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 Oct 14, 2024
mikebrow
mikebrow approved these changes Oct 14, 2024
View reviewed changes
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM

@kannon92
Copy link
Contributor

/assign @mrunalp @dchen1107

We want to get the doc updates in but there was no exception so this will stay in alpha.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 15, 2024
@saschagrunert
Copy link
Member

@mrunalp thank you for the review, I added your suggestions and squashed the commits together.

@saschagrunert saschagrunert changed the title KEP-4639: bump to beta KEP-4639: update to most recent implementation state Oct 15, 2024
@haircommander @saschagrunert
KEP-4639: update to most recent implementation state
54e8884 
Signed-off-by: Peter Hunt <[email protected]>
Signed-off-by: Sascha Grunert <[email protected]>
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: deads2k, haircommander, mikebrow
Once this PR has been reviewed and has the lgtm label, please ask for approval from dchen1107. 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:

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

@carlory
Copy link
Member

carlory commented Oct 15, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 15, 2024
mikebrow
mikebrow approved these changes Oct 16, 2024
View reviewed changes
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

/LGTM
also good with the review updates

@tarilabs tarilabs mentioned this pull request Nov 29, 2024
Open
@sudo-bmitch sudo-bmitch mentioned this pull request Dec 4, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlory carlory carlory left review comments

@sftim sftim sftim left review comments

@sohankunkerkar sohankunkerkar sohankunkerkar left review comments

@SergeyKanzhelev SergeyKanzhelev SergeyKanzhelev left review comments

@aojea aojea aojea left review comments

@kannon92 kannon92 kannon92 left review comments

@saschagrunert saschagrunert saschagrunert left review comments

@mrunalp mrunalp mrunalp left review comments

@pohly pohly pohly left review comments

@deads2k deads2k deads2k requested changes

@sudo-bmitch sudo-bmitch sudo-bmitch requested changes

@mikebrow mikebrow mikebrow approved these changes

@dchen1107 dchen1107 Awaiting requested review from dchen1107

Assignees

@mrunalp mrunalp

@dchen1107 dchen1107

@SergeyKanzhelev SergeyKanzhelev

@mikebrow mikebrow

@carlory carlory

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 lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.