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-3542: СRI image pulling with progress notification #3547

Open
wants to merge 36 commits into
base: master
Choose a base branch
from
Open

KEP-3542: СRI image pulling with progress notification #3547

wants to merge 36 commits into from
+920 −0

Conversation

byako
Copy link
Contributor

@byako byako commented Sep 26, 2022

  • One-line PR description: adding new KEP
  • Other comments:

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 26, 2022
@k8s-ci-robot k8s-ci-robot added the kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory label Sep 26, 2022
@k8s-ci-robot k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Sep 26, 2022
@k8s-ci-robot
Copy link
Contributor

Welcome @byako!

It looks like this is your first PR to kubernetes/enhancements 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/enhancements has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 26, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @byako. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 26, 2022
@byako byako mentioned this pull request Sep 26, 2022
Open
4 tasks
sftim
sftim reviewed Sep 29, 2022
View reviewed changes
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Thanks for the KEP suggestion.

keps/sig-node/3542-cri-image-pulling-with-progress-notification/README.md Outdated Show resolved Hide resolved
@byako byako marked this pull request as draft September 30, 2022 09:20
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 30, 2022
@SergeyKanzhelev SergeyKanzhelev moved this from Triage to Waiting on Author in SIG Node PR Triage Sep 30, 2022
jukkar and others added 2 commits October 3, 2022 10:37
@jukkar
KEP-3542: Add design details and info about testing.
fa0339a 
Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
@byako
Merge pull request #1 from jukkar/kep-3542-cri-image-pulling-with-status
4d6658c 
KEP-3542: Add design details and info about testing.
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 4, 2022 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 4, 2022
@byako byako marked this pull request as ready for review October 5, 2022 18:30
@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 Oct 5, 2022
mrunalp
mrunalp reviewed Oct 5, 2023
View reviewed changes
keps/sig-node/3542-cri-image-pulling-with-progress-notification/README.md
Following new CRI streaming API call is proposed:

// PullImageWithProgress pulls an image with authentication config.
// It returns periodically amount of image pulled so far.
Copy link
Contributor

Choose a reason for hiding this comment

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

How frequently will it stream updates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PullImageWithProgressRequest type of argument will tell it how often and based on what to send updates.

mrunalp
mrunalp reviewed Oct 5, 2023
View reviewed changes
keps/sig-node/3542-cri-image-pulling-with-progress-notification/README.md Outdated
The PullImageWithProgressRequest contains base information needed to do the image pull (image name, auth config
and sandbox information), and it will also contain information how often the server should send progress reports.
The CRI client can restrict the progress reporting to be time-based (e.g. once every n. seconds),
or based on size (amount of bytes/KiB/MiB downloaded). The size-based should be the default one.
Copy link
Contributor

Choose a reason for hiding this comment

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

Time based one is more useful for detecting whether there is progress or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose we shouldn't restrict the runtime to chose the default, I'll remove that sentence.

mrunalp
mrunalp reviewed Oct 5, 2023
View reviewed changes
keps/sig-node/3542-cri-image-pulling-with-progress-notification/README.md
// - positive number up to 4294967295 (uint32 max number, approx. 136 years)
// Default: 10
// +optional
NoProgressTimeout int32 `json:"NoProgressTimeout,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

How will this interact with the current CRI timeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose this was meant to be much shorter than CRI timeout and tell the runtime how soon to report failure if there was no progress in image downloading. Without it the CRI timeout would cancel the call much later.

mrunalp
mrunalp reviewed Oct 5, 2023
View reviewed changes
keps/sig-node/3542-cri-image-pulling-with-progress-notification/README.md Outdated

In Alpha:

- image pulling operation should fail faster.
Copy link
Contributor

Choose a reason for hiding this comment

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

large image pulls don't timeout on CRI timeout and are completed without being restarted/cancelled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this was about image size. Wasn't the point of no-progress timeout to fail faster when there is no progress and the image is impossible to download? I'll make it clear in phrasing here.

@pacoxu pacoxu moved this from Needs Approver to Needs Reviewer in SIG Node PR Triage Oct 16, 2023
@pacoxu
Copy link
Member

pacoxu commented Oct 16, 2023

/milestone clear
as we miss the enhancement freeze and no exception is open yet.

@k8s-ci-robot k8s-ci-robot removed this from the v1.28 milestone Oct 16, 2023
@byako
Copy link
Contributor Author

byako commented Oct 16, 2023

I cannot request exception for this, SIG leadership does not approve this, I'll have to move this to next release.

@k8s-triage-robot
Copy link

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-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 22, 2024
@SergeyKanzhelev
Copy link
Member

/stage alpha
/milestone v1.30

@k8s-ci-robot k8s-ci-robot added the stage/alpha Denotes an issue tracking an enhancement targeted for Alpha status label Jan 26, 2024
@k8s-ci-robot k8s-ci-robot added this to the v1.30 milestone Jan 26, 2024
@byako byako requested a review from mrunalp February 8, 2024 18:11
@k8s-triage-robot
Copy link

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-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 9, 2024
@byako
Copy link
Contributor Author

byako commented Mar 12, 2024

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Mar 12, 2024
@pacoxu
Copy link
Member

pacoxu commented Mar 26, 2024

/milestone clear

@k8s-ci-robot k8s-ci-robot removed this from the v1.30 milestone Mar 26, 2024
@k8s-triage-robot
Copy link

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-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 24, 2024
@frittentheke
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 25, 2024
@frittentheke
Copy link

With images containing LLMs and other piles of static data alongside the running software itself having some progress indication (and lack thereof) would be great.

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

@saschagrunert saschagrunert saschagrunert left review comments

@ruiwen-zhao ruiwen-zhao ruiwen-zhao left review comments

@wzshiming wzshiming wzshiming left review comments

@sftim sftim sftim left review comments

@mikebrow mikebrow mikebrow approved these changes

@dchen1107 dchen1107 Awaiting requested review from dchen1107

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@adisky adisky Awaiting requested review from adisky

@wojtek-t wojtek-t Awaiting requested review from wojtek-t

@mrunalp mrunalp Awaiting requested review from mrunalp

Assignees

@mrunalp mrunalp

@pacoxu pacoxu

@ruiwen-zhao ruiwen-zhao

@mikebrow mikebrow

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 ok-to-test Indicates a non-member PR verified by an org member that is safe to test. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. stage/alpha Denotes an issue tracking an enhancement targeted for Alpha status
Projects
SIG Node PR Triage
Status: Needs Reviewer
SIG Node PR Triage
Needs Reviewer
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet