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

README: update local storage docs #303

Closed
wants to merge 3 commits into from

Conversation

kingsleyzissou
Copy link
Contributor

Since we are always pulling the latest image by default the container image will always be in the host's local storage. Update the readme to reflect this.

Additionally, address one or two of the minor comments from #120.

A local container image from the host is being used.
Small fix to the `image_type` fixture by deduplicating the call to `build_images`
Comment on lines +50 to +51
NOTE: local storage is being used by default. If the `--local` flag is not provided, as in the above example,
the latest image will be pulled into the local storage.
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no pulling involved here, right? We're just...using the image that is local. But now I'm confused as if it's default, why do we need to talk about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We changed this up in #262 so that we always pull the container image

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bad default, we are experiencing bad behaviour with ai-lab-recipes, because the images are huge. Default should be to only pull if the image is missing.

Copy link

This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale Issue or PR with no activity for extended period of time label Apr 27, 2024
@achilleas-k
Copy link
Member

What's the status of this? Do we still need it?

@kingsleyzissou
Copy link
Contributor Author

What's the status of this? Do we still need it?

I think @ondrejbudai had some reservations about it but I might be misremembering... but if I am, what are your thoughts here?

@ondrejbudai
Copy link
Member

First things first: I'm still not sure whether we should indeed mount the host's storage r/w:

  • I'm not sure whether sharing the storage (read/write) between multiple podman versions is alright.
  • On the other hand, mounting it r/o means that we cannot lock it, and then very weird issues might arise from the fact that the host can modify the storage while we build from it.

I think I would prefer keeping the mount r/w, because it feels simpler to an end user.

Regarding --local: let's recap what it does today: --local is equal to --pull=always (if bib is compared to podman build or podman run). --local=false equals to --pull=never.

So I think we should deprecate --local and just implement --pull with the same semantics as in podman build or podman run. This way, our CLIs match, and it's easy to understand what bib is actually doing (--local is actually very non-descriptive).

One missing detail: What should be the default behavior? podman build has --pull=always as the default, whereas podman run uses --pull=missing. podman run feels more used than build, so missing would be the natural candidate. On the other hand, podman build is semantically more similar to what bib does, always pulling the latest and greatest image feels in the spirit of image builder (if you don't pin your deps) and it's also a better idea from the security perspective.

@cgwalters thoughts?

@kingsleyzissou
Copy link
Contributor Author

Yeah those are all valid points. The --local flag isn't very descriptive. I'll wait for Colin's feedback on that.

Regarding mounting the store r/o I don't think it's possible because we mount the container images in osbuild:

@ondrejbudai
Copy link
Member

This makes me wonder if we can create a container from a read-only image (this should be possible, right?) and then mount the container. Alternatively, we can teach osbuild to use podman as a buildroot, but that's a more long-term project.

@cgwalters
Copy link
Contributor

Regarding --local: let's recap what it does today: --local is equal to --pull=always (if bib is compared to podman build or podman run). --local=false equals to --pull=never.

It's the opposite right? --local means --pull=never.

BTW the big problem here is not actually about pulling it's about things like bib not using the container auth stack #142 by default, which still applies without specifying --local.

@cgwalters
Copy link
Contributor

cgwalters commented May 2, 2024

My take here is we should change bib to exactly match the podman default: pull-if-not-exists.

Anyone who wants to update the image can already do podman pull on it explicitly too.

But I am also fine with having a --pull=always in bib (but if it exists it must honor the container auth stack and actually the full host container config for /etc/containers around image registry remapping, etc.). IOW it needs to defer to executing podman pull on the host or equivalent.

@github-actions github-actions bot removed the Stale Issue or PR with no activity for extended period of time label May 3, 2024
@kingsleyzissou
Copy link
Contributor Author

kingsleyzissou commented May 3, 2024

It's the opposite right? --local means --pull=never.

This is just another sign that this is really bad UX and a bad flag name 😅 but yeah you're right --local == --pull=never

@kingsleyzissou
Copy link
Contributor Author

But I am also fine with having a --pull=always in bib (but if it exists it must honor the container auth stack and actually the full host container config for /etc/containers around image registry remapping, etc.). IOW it needs to defer to executing podman pull on the host or equivalent.

I think we're doing that already, no?

@kingsleyzissou
Copy link
Contributor Author

Small POC here: #418

@rhatdan
Copy link
Contributor

rhatdan commented May 4, 2024

The default should be --pull=missing.

@rhatdan
Copy link
Contributor

rhatdan commented May 4, 2024

$ podman run --help | grep -- --pull
--pull string Pull image policy ("always"|"missing"|"never"|"newer") (default "missing")

@rhatdan
Copy link
Contributor

rhatdan commented May 4, 2024

Not sure why you need to mount host storage read/write. Lock files should be able to be used if you use an ADDITIONAL STORE.

If you mount /var/lib/containers/storage:/usr/lib/containers/storage:RO and have storage.conf setup with an additional store pointed at /usr/lib/containers/storage then everything will work without having a writeable storage directory.

Latest fedora podman has this setup by default.

 $ grep -C 3 additionalimagestore /usr/share/containers/storage.conf 
# AdditionalImageStores is used to pass paths to additional Read/Only image stores
# Must be comma separated list.
additionalimagestores = [
"/usr/lib/containers/storage",
]

@kingsleyzissou
Copy link
Contributor Author

Not sure why you need to mount host storage read/write. Lock files should be able to be used if you use an ADDITIONAL STORE.

If you mount /var/lib/containers/storage:/usr/lib/containers/storage:RO and have storage.conf setup with an additional store pointed at /usr/lib/containers/storage then everything will work without having a writeable storage directory.

Latest fedora podman has this setup by default.


 $ grep -C 3 additionalimagestore /usr/share/containers/storage.conf 

# AdditionalImageStores is used to pass paths to additional Read/Only image stores

# Must be comma separated list.

additionalimagestores = [

"/usr/lib/containers/storage",

]

The issue we are having is we mount the container in an osbuild stage and copy the contents into the build root. Trying to mount a container from the RO store errors out and the image build fails. Is there any way to work around this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants