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

podman-remote: default PullPolicy must be missing instead of empty str #12210

Closed

Conversation

flouthoc
Copy link
Collaborator

@flouthoc flouthoc commented Nov 8, 2021

If registry is remote default pull policy must be missing instead of
remote. Since empty str == pullpolicy {always} for more context on
this behaviour check #10739

[NO TESTS NEEDED]

Closes: #12201

If registry is remote default pull policy must be `missing` instead of
remote. Since `empty str == pullpolicy {always}` for more context on
this behaviour check containers#10739

[NO TESTS NEEDED]

Signed-off-by: Aditya Rajan <[email protected]>
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 8, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: flouthoc
To complete the pull request process, please assign luap99 after the PR has been reviewed.
You can assign the PR to them by writing /assign @luap99 in a comment when ready.

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

@flouthoc
Copy link
Collaborator Author

flouthoc commented Nov 8, 2021

@afbjorklund @vrothberg It seems we are passing empty string in case of podman-remote which is not expected. Default policy must be missing always. @afbjorklund Could you try above patch please.

@afbjorklund
Copy link
Contributor

afbjorklund commented Nov 8, 2021

Could you try above patch please.

I don't see any changes in behaviour.

Possibly the old server (3.2.1) didn't have any API field to send the "policy" flag in, it seems it was added in 3.4.1 ?

@flouthoc
Copy link
Collaborator Author

flouthoc commented Nov 8, 2021

@afbjorklund I think yes it was introduced after #10739. Could you try with version 3.4.1 or above. I don't know if there are any plans of back-porting the field though.

@afbjorklund
Copy link
Contributor

The moral of the story is that you always need to match the podman client with the server, I suppose.

@afbjorklund
Copy link
Contributor

afbjorklund commented Nov 8, 2021

Could you try with version 3.4.1 or above.

Podman 3.2.1 was the latest version for Ubuntu 21.10

https://packages.ubuntu.com/search?keywords=podman

Otherwise one would have to go back to the vendor packages (Kubic) again, instead of using the system packages ? Or use the static binaries perhaps, for the older clients.

@flouthoc
Copy link
Collaborator Author

flouthoc commented Nov 8, 2021

@afbjorklund were you able to try this out with 3.4.1

@Luap99
Copy link
Member

Luap99 commented Nov 8, 2021

I think the default policy must be set on the server side and never the client. We control podman-remote but other API users face the same problem.

@Luap99
Copy link
Member

Luap99 commented Nov 8, 2021

@vrothberg PTAL

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

I concur with @Luap99. We may need a similar check in the handlers.

What exactly in #10739 adds context to this issue? The two issues look substantially different to me.

@flouthoc
Copy link
Collaborator Author

flouthoc commented Nov 8, 2021

@vrothberg I think PR added a default to always when empty policy string is specified i am not entirely sure though https://github.com/containers/podman/pull/10739/files#diff-ff4f6ad01da9e06c3f164f840cc9376e3cbc746c74e5811ae59690323eb1fa37R152 but i agree with @Luap99 we can move defaults to server side.

@flouthoc flouthoc marked this pull request as draft November 8, 2021 14:58
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 8, 2021
@rhatdan
Copy link
Member

rhatdan commented Nov 8, 2021

I agree this should be handled on the remote side of the connection.

@afbjorklund
Copy link
Contributor

Need to try again with 3.4.1 client towards a 3.4.1 server, but I suspect that it works (or else that release does not) ?

I was expecting a pull policy of "missing" for run and "always" for pull. Think I need a 3.2.1 client for the 3.2.1 server.

@afbjorklund
Copy link
Contributor

@flouthoc

Confirm that the 3.4.1 server works with the 3.4.1 client:

DEBU[0000] DoRequest Method: GET URI: http://d/v3.4.1/libpod/_ping 
DEBU[0000] Loading registries configuration "/etc/containers/registries.conf" 
DEBU[0000] DoRequest Method: POST URI: http://d/v3.4.1/libpod/images/pull 
DEBU[0000] DoRequest Method: POST URI: http://d/v3.4.1/libpod/containers/create 

And also that the 3.4.1 server works with the 3.2.1 client:

DEBU[0000] DoRequest Method: GET URI: http://d/v3.2.1/libpod/_ping 
DEBU[0000] DoRequest Method: GET URI: http://d/v3.2.1/libpod/images/docker.io%2Fbusybox/exists 
DEBU[0000] DoRequest Method: POST URI: http://d/v3.2.1/libpod/containers/create 

The problem is with the 3.2.1 server and the 3.4.1 client.

Unfortunately, that is also the scenario with lima and brew...

@rhatdan
Copy link
Member

rhatdan commented Nov 8, 2021

If this gets fixed in the server, then only the server will matter.

@flouthoc flouthoc closed this Nov 9, 2021
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

podman --remote run pulls image on every invocation
5 participants