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.

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

Way to specify number of image pull attempts and the pause between attempts #19770

Closed
praiskup opened this issue Aug 28, 2023 · 14 comments · Fixed by #21659
Closed

Way to specify number of image pull attempts and the pause between attempts #19770

praiskup opened this issue Aug 28, 2023 · 14 comments · Fixed by #21659
Labels
kind/feature Categorizes issue or PR as related to a new feature. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. stale-issue

Comments

@praiskup
Copy link

praiskup commented Aug 28, 2023

Feature request description

Currently, there's just a default, 3 attempts with 1s delay between attempts:

https://github.com/containers/common/blob/e028741ef77fdfa3ae261b9d23cdd50253d586c4/libimage/copier.go#L27-L30

It would be nice to have this configurable.

Forwarded-from: #14359
Needed by: rpm-software-management/mock#1191

Suggest potential solution

If we could have a configuration-file option rather than a command-line option (or both ideally)? The thing is that we could configure the new option without checking if Podman supports this or not.

Have you considered any alternatives?

We can implement re-tries (also?) in Mock. How likely we can get this feature into Podman on RHEL8+, and what would be the ETA? We need to some plan for Mock and Copr where we started to rely on Podman pull functionality, and we need to have this rather quickly.

Additional context

Add any other context or screenshots about the feature request here.

@praiskup praiskup added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 28, 2023
@vrothberg
Copy link
Member

Thanks for opening the issue, @praiskup.

Adding fields to containers.conf seems reasonable to me. @mtrmac @rhatdan WDYT?

@mtrmac
Copy link
Collaborator

mtrmac commented Aug 28, 2023

  • I’m not too convinced a system-wide config file is the right place. Retry behavior is inherently operation-specific; “what if two users need different trade-offs”? A CLI option would make more sense to me.
    • In particular, “a config file option is better than a CLI option because we could use it whether Podman supports it or not” is not convincing to me; this suggests that Podman is silently ignoring unknown config options (? I think that’s not really good), and that users are now relying on that (which I think is definitely undesirable and needs to be stopped urgently).
    • That said, the recent (HPC?) discussions about modular / named config sets make the CLI/config file conceptual difference much smaller, so having that in a config file might ultimately be both correct and more convenient — each environment / set of tradeoffs can specify a single config file option, instead of many individual trade-off options.
  • Skopeo already exposes one of the pkg/retry tunables as a CLI option, so there’s a precedent.

praiskup added a commit to praiskup/mock that referenced this issue Aug 29, 2023
Move podman_check_native_image_architecture() to the later stage, so we
don't retry upon architecture mismatch.

Relates: containers/podman#19770
praiskup added a commit to praiskup/mock that referenced this issue Aug 29, 2023
Move podman_check_native_image_architecture() to the later stage, so we
don't retry upon architecture mismatch.

Relates: containers/podman#19770
praiskup added a commit to praiskup/mock that referenced this issue Aug 29, 2023
Move podman_check_native_image_architecture() to the later stage, so we
don't retry upon architecture mismatch.

Relates: containers/podman#19770
praiskup added a commit to praiskup/mock that referenced this issue Aug 29, 2023
Move podman_check_native_image_architecture() to the later stage, so we
don't retry upon architecture mismatch.

Relates: containers/podman#19770
praiskup added a commit to praiskup/mock that referenced this issue Aug 29, 2023
Move podman_check_native_image_architecture() to the later stage, so we
don't retry upon architecture mismatch.

Relates: containers/podman#19770
praiskup added a commit to praiskup/mock that referenced this issue Aug 29, 2023
Move podman_check_native_image_architecture() to the later stage, so we
don't retry upon architecture mismatch.

Relates: containers/podman#19770
praiskup added a commit to praiskup/mock that referenced this issue Aug 29, 2023
Move podman_check_native_image_architecture() to the later stage, so we
don't retry upon architecture mismatch.

Relates: containers/podman#19770
praiskup added a commit to praiskup/mock that referenced this issue Aug 29, 2023
Move podman_check_native_image_architecture() to the later stage, so we
don't retry upon architecture mismatch.

Relates: containers/podman#19770
praiskup added a commit to praiskup/mock that referenced this issue Aug 29, 2023
Move podman_check_native_image_architecture() to the later stage, so we
don't retry upon architecture mismatch.

Relates: containers/podman#19770
Closes: rpm-software-management#1200
praiskup added a commit to praiskup/mock that referenced this issue Aug 29, 2023
Move podman_check_native_image_architecture() to the later stage, so we
don't retry upon architecture mismatch.

Relates: containers/podman#19770
Closes: rpm-software-management#1200
praiskup added a commit to rpm-software-management/mock that referenced this issue Aug 30, 2023
Move podman_check_native_image_architecture() to the later stage, so we
don't retry upon architecture mismatch.

Relates: containers/podman#19770
Closes: #1200
@github-actions
Copy link

A friendly reminder that this issue had no activity for 30 days.

@dustymabe
Copy link
Contributor

I think something like this would be nice. I could see a configuration option for setting things globally and then CLI options for overriding that configuration.

Right now we are dealing with quay CDN DNS flakes and many others have dealt with it too:

podman build has:

--retry=attempts
    Number of times to retry in case of failure when performing pull of images from registry. Default is 3.

--retry-delay=duration
    Duration of delay between retry attempts in case of failure when performing pull of images from registry. Default is 2s.

It would be nice if say podman pull and podman run had similar options.

@dustymabe
Copy link
Contributor

Thanks @rhatdan for #21659

Any chance of adding something similar to podman run? We can change our workflow to podman pull and then podman run if needed, but would prefer it in one command if possible.

@rhatdan
Copy link
Member

rhatdan commented Feb 19, 2024

Do you think this would be better to be just containers.conf then here?

@dustymabe
Copy link
Contributor

Do you think this would be better to be just containers.conf then here?

Not sure.

I did mention in #19770 (comment) that:

I think something like this would be nice. I could see a configuration option for setting things globally and then CLI options for overriding that configuration.

Which still makes sense to me. A config option sets the default value and then runtime CLI options override that.

So maybe the answer to your question is both

@mtrmac
Copy link
Collaborator

mtrmac commented Feb 19, 2024

/me points at #19770 (comment) again

@mtrmac
Copy link
Collaborator

mtrmac commented Feb 19, 2024

I think config file options make good sense only with Podman’s config modules; otherwise, it is inherently operation-specific, even on “single-purpose” machines (like OpenShift nodes) if the machine setup also happens by pulling images.

@dustymabe
Copy link
Contributor

dustymabe commented Feb 19, 2024

@mtrmac could the config file option just be set to default to the current behavior today and only people who need to change it change it? i.e. I could see the default being no retry at all, but then if someone has flaky DNS or some other intermittent networking issues they could change the global default on that node without having to update actual code that is doing the container pull operations (which either they might not own or could take time).

@mtrmac
Copy link
Collaborator

mtrmac commented Feb 19, 2024

Why not, it happens to be up to other people to maintain that :)

I think this immediate conversation was a request about retry in podman run followed by a question about the config file — which I interpret as a choice between a CLI and a config file; in that case I think fairly strongly it should be the CLI.

@dustymabe
Copy link
Contributor

Yeah. I think I was taking it back to the original description where they said:

If we could have a configuration-file option rather than a command-line option (or both ideally)? The thing is that we could configure the new option without checking if Podman supports this or not.

So I think they were wanting both. Now wanting it and getting it are two different things. I'm happy either way (after all I'm not the one doing the work), but I argue it would be better to have both.

@rhatdan
Copy link
Member

rhatdan commented Feb 20, 2024

Right now, for podman pull and build the default is 3 and delay of 2s. I can attempt to add support for containers.conf to handle this. Then it will need to be vendored into buildah and podman, and have podman build, and podman pull default to the containers.conf setting, finally podman run and podman kube play would need to start supporting it, then we can talk about adding options.

  --retry uint                   number of times to retry in case of failure when performing pull (default 3)
  --retry-delay string           delay between retries in case of pull failures (default "2s")

@mtrmac
Copy link
Collaborator

mtrmac commented Feb 20, 2024

It’s the other way around - adding CLI options can be done within Podman itself without dealing with all the dependencies. IMHO it’s both more important and easier.

@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label May 21, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators May 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/feature Categorizes issue or PR as related to a new feature. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. stale-issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants