Skip to content

Conversation

@saschagrunert
Copy link
Member

What type of PR is this?

/kind feature

What this PR does / why we need it:

We now support a pull context which can actually be cancelled. This way we can combine the pull progress together with a dynamic timeout, which cancels the context if no progress is being made on pull.

Which issue(s) this PR fixes:

Refers to #7885

Special notes for your reviewer:

cc @mrunalp

Does this PR introduce a user-facing change?

Added dynamic image pull timeout if the network availability goes down during the operation.

@saschagrunert saschagrunert requested a review from mrunalp as a code owner March 14, 2024 11:58
@openshift-ci openshift-ci bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Mar 14, 2024
@openshift-ci openshift-ci bot requested review from hasan4791 and wgahnagl March 14, 2024 11:58
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 14, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: saschagrunert

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2024
@saschagrunert saschagrunert force-pushed the pull-timeout branch 2 times, most recently from 45857bd to 7e1adef Compare March 14, 2024 12:15
@codecov
Copy link

codecov bot commented Mar 14, 2024

Codecov Report

Merging #7887 (1a377cc) into main (93b05e5) will decrease coverage by 0.01%.
Report is 6 commits behind head on main.
The diff coverage is 55.55%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7887      +/-   ##
==========================================
- Coverage   48.87%   48.86%   -0.01%     
==========================================
  Files         152      152              
  Lines       16447    16456       +9     
==========================================
+ Hits         8038     8042       +4     
- Misses       7433     7437       +4     
- Partials      976      977       +1     

@saschagrunert
Copy link
Member Author

/retest

@saschagrunert
Copy link
Member Author

/retest

@saschagrunert
Copy link
Member Author

/test ci-e2e

@saschagrunert
Copy link
Member Author

/retest

We now support a pull context which can actually be cancelled. This way
we can combine the pull progress together with a dynamic timeout, which
cancels the context if no progress is being made on pull.

Signed-off-by: Sascha Grunert <[email protected]>
@saschagrunert
Copy link
Member Author

/retest

@saschagrunert
Copy link
Member Author

/test e2e-gcp-ovn

@saschagrunert
Copy link
Member Author

@cri-o/cri-o-maintainers PTAL

@haircommander haircommander modified the milestones: 1.31, 1.30 Mar 27, 2024
@haircommander
Copy link
Member

I think this would fix #6574 as well

// It also checks if progress is being made within a constant timeout.
// If the timeout is reached because no progress updates have been made, then
// the cancel function will be called.
func progressGoRoutine(ctx context.Context, cancel context.CancelFunc, progress <-chan imageTypes.ProgressProperties, remoteCandidateName storage.RegistryImageReference) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: maybe consumeImagePullProgress or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, following-up in #7946

@haircommander
Copy link
Member

/lgtm

nit can be addressed async, or ignored

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2024
@haircommander
Copy link
Member

/retest

@kwilczynski
Copy link
Contributor

/test e2e-gcp-ovn

@kwilczynski
Copy link
Contributor

/retest-required

@openshift-merge-bot openshift-merge-bot bot merged commit 51a8df1 into cri-o:main Apr 1, 2024
@saschagrunert saschagrunert deleted the pull-timeout branch April 2, 2024 06:53
saschagrunert added a commit to saschagrunert/cri-o that referenced this pull request Apr 2, 2024
saschagrunert added a commit to saschagrunert/cri-o that referenced this pull request Jun 6, 2024
This allows to set a pull timeout (currently not used neither by the
kubelet nor cri-tools) using the CRI context. Future consumers can then
limit the pul not only if no network traffic is available (introduced in
\cri-o#7887), but also in a more general way.

Signed-off-by: Sascha Grunert <[email protected]>
saschagrunert added a commit to saschagrunert/cri-o that referenced this pull request Nov 18, 2024
Add the option to be able to fine-tune the pull progress timeout or even
disable it.

Fixes cri-o#8764 and cri-o#8760

Follow-up on: cri-o#7887

Signed-off-by: Sascha Grunert <[email protected]>
saschagrunert added a commit to saschagrunert/cri-o that referenced this pull request Nov 18, 2024
Add the option to be able to fine-tune the pull progress timeout or even
disable it.

Fixes cri-o#8764 and cri-o#8760

Follow-up on: cri-o#7887

Signed-off-by: Sascha Grunert <[email protected]>
saschagrunert added a commit to saschagrunert/cri-o that referenced this pull request Nov 18, 2024
Add the option to be able to fine-tune the pull progress timeout or even
disable it.

Fixes cri-o#8764 and cri-o#8760

Follow-up on: cri-o#7887

Signed-off-by: Sascha Grunert <[email protected]>
saschagrunert added a commit to saschagrunert/cri-o that referenced this pull request Nov 18, 2024
Add the option to be able to fine-tune the pull progress timeout or even
disable it.

Fixes cri-o#8764 and cri-o#8760

Follow-up on: cri-o#7887

Signed-off-by: Sascha Grunert <[email protected]>
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/cri-o that referenced this pull request Nov 25, 2024
Add the option to be able to fine-tune the pull progress timeout or even
disable it.

Fixes cri-o#8764 and cri-o#8760

Follow-up on: cri-o#7887

Signed-off-by: Sascha Grunert <[email protected]>
xw19 pushed a commit to xw19/cri-o that referenced this pull request Dec 4, 2024
Add the option to be able to fine-tune the pull progress timeout or even
disable it.

Fixes cri-o#8764 and cri-o#8760

Follow-up on: cri-o#7887

Signed-off-by: Sascha Grunert <[email protected]>
robbmanes pushed a commit to robbmanes/cri-o that referenced this pull request Feb 17, 2025
Add the option to be able to fine-tune the pull progress timeout or even
disable it.

Fixes cri-o#8764 and cri-o#8760

Follow-up on: cri-o#7887

Signed-off-by: Sascha Grunert <[email protected]>
robbmanes pushed a commit to robbmanes/cri-o that referenced this pull request Feb 18, 2025
Add the option to be able to fine-tune the pull progress timeout or even
disable it.

Fixes cri-o#8764 and cri-o#8760

Follow-up on: cri-o#7887

Signed-off-by: Sascha Grunert <[email protected]>
saschagrunert added a commit to saschagrunert/cri-o that referenced this pull request Feb 24, 2025
Add the option to be able to fine-tune the pull progress timeout or even
disable it.

Fixes cri-o#8764 and cri-o#8760

Follow-up on: cri-o#7887

Signed-off-by: Sascha Grunert <[email protected]>
saschagrunert added a commit to saschagrunert/cri-o that referenced this pull request Feb 24, 2025
Add the option to be able to fine-tune the pull progress timeout or even
disable it.

Fixes cri-o#8764 and cri-o#8760

Follow-up on: cri-o#7887

Signed-off-by: Sascha Grunert <[email protected]>
saschagrunert added a commit to saschagrunert/cri-o that referenced this pull request Feb 24, 2025
Add the option to be able to fine-tune the pull progress timeout or even
disable it.

Fixes cri-o#8764 and cri-o#8760

Follow-up on: cri-o#7887

Signed-off-by: Sascha Grunert <[email protected]>
saschagrunert added a commit to saschagrunert/cri-o that referenced this pull request Feb 24, 2025
Add the option to be able to fine-tune the pull progress timeout or even
disable it.

Fixes cri-o#8764 and cri-o#8760

Follow-up on: cri-o#7887

Signed-off-by: Sascha Grunert <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants