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

Deprecate --local flag (HMS-3792) #423

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kingsleyzissou
Copy link
Contributor

@kingsleyzissou kingsleyzissou commented May 7, 2024

Pulling container images before building would break in the case of authenticated images on podman machine, since the auth file lives on the host and podman machine and won't know about it.

This commit deprecates the --local flag and puts the requirement on the user to ensure that the container is in local storage before initiating the build.

Copy link
Contributor

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Overall LGTM just two nits

bib/cmd/bootc-image-builder/main.go Outdated Show resolved Hide resolved
bib/cmd/bootc-image-builder/main.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@mvo5 mvo5 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 this! Looks good, one tiny idea/suggestion inline.

test/test_build.py Outdated Show resolved Hide resolved
@kingsleyzissou
Copy link
Contributor Author

Arghhh of course this breaks tls-verify, how do we want to handle that? We aren't resolving containers from remote registries anymore, so maybe we want to disable this too

@kingsleyzissou kingsleyzissou force-pushed the deprecate-local branch 2 times, most recently from 996a9ff to 0f0f46f Compare May 7, 2024 15:59
achilleas-k
achilleas-k previously approved these changes May 7, 2024
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

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

The joy and simplicity of a mostly-red PR :)

mvo5
mvo5 previously approved these changes May 8, 2024
Copy link
Collaborator

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thank you!

@kingsleyzissou
Copy link
Contributor Author

flake8 failed on line length. Think it should be all good now 🤞

mvo5
mvo5 previously approved these changes May 8, 2024
mvo5
mvo5 previously approved these changes May 8, 2024
@kingsleyzissou
Copy link
Contributor Author

Okay it's finally green now. Removing the flag also broke the cross-arch integration tests :/
Simple PR but had so many knock-on effects

mvo5
mvo5 previously approved these changes May 13, 2024
Copy link
Collaborator

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thank you

Copy link
Collaborator

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

One more note/question but could be followup (just defensive)

test/test_build.py Outdated Show resolved Hide resolved
test/test_build.py Outdated Show resolved Hide resolved
Copy link
Member

@ondrejbudai ondrejbudai left a comment

Choose a reason for hiding this comment

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

I love this! :)

Marking this as request changes, because I want to first talk to the podman desktop folks if this is alright for them, and I want to figure out if we can somehow have bib releases, so users know what version they run, and thus which flags are supported and what's the default behaviour (git SHAs aren't very friendly in this case unfortunately).

@kingsleyzissou kingsleyzissou force-pushed the deprecate-local branch 2 times, most recently from a0f0aa1 to 22daa99 Compare September 11, 2024 15:42
test/testutil.py Outdated Show resolved Hide resolved
mvo5
mvo5 previously approved these changes Sep 11, 2024
Copy link
Collaborator

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thank you

achilleas-k
achilleas-k previously approved these changes Sep 16, 2024
ondrejbudai
ondrejbudai previously approved these changes Sep 17, 2024
@ondrejbudai
Copy link
Member

Thanks! 👍

@achilleas-k achilleas-k added this pull request to the merge queue Sep 17, 2024
@achilleas-k
Copy link
Member

achilleas-k commented Sep 17, 2024

Merge conflicts in the queue

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Sep 17, 2024
kingsleyzissou and others added 3 commits October 10, 2024 09:44
Pulling container images before building would break in the case of
authenticated images on podman machine, since the auth file lives on the
host and not podman machine and won't know about it.

This commit deprecates the `--local` flag and warns users when it is
passed to the CLI so that this won't break things for anyone who might
already be using the flag. This change means that the user will have to
ensure that the container is pulled to the local container store before
initiating the build.

Co-authored-by: Ondřej Budai <[email protected]>
Ensure that the containers have been copied into local
storage for all test cases.

We need to explicitly pull the container into local containers
storage with the correct arch otherwise cross-arch building
fails. The helper function uses the host-arch as a fallback
when no target arch is provided.
Since all containers are coming from local storage and require the user
to pull in the container before-hand, we can disable the `--tls-verify`
flag. The containers will not be resolved from a remote registry but
rather from the local container store.
@jpace121
Copy link

jpace121 commented Oct 13, 2024

Hi! So I'm currently using bootc-image-builder and am purposely not using --local, and being forced to use --local would break the tool for me.

More specifically, on my build computer I set the graphroot in storage.conf to a directory on a separate very large SSD from /, since the SSD I use for / is somewhat small. This means I don't have any images in /var/lib/containers/storage, and I don't want to; I don't have enough storage there. bootc-image-builder hard codes looking in /var/lib/containers/storage when looking for/building manifests though. To workaround this, I tried mounting the host's graphroot in the bootc-image-builder container under /var/lib/containers/storage, but that didn't work because I guess there is some db in that directory that is aware of what directory its in, and when the directories don't line up, podman get grumpy.

If you remove the local option, would you be willing to also find the places where the graphroot is assumed to be /var/lib/containers/storage and provide an option to override it?

EDIT: Thanks for this great tool and bootc in general, it really is a great way to build the custom images I want for my application, while still being able to take advantage of all the benefits of a full Linux Distro.

@achilleas-k
Copy link
Member

@jpace121 I think your use case can be solved by mounting both into the bootc-image-builder container.

podman run -v /var/lib/containers:/var/lib/containers -v /path/to/large/ssd/container/storage:/path/to/large/ssd/container/storage ...

Can you see if that works?

The /var/lib/containers mount should make BIB pick up the storage.conf and point it to the large container storage, which will be at the expected location inside the container.

If you remove the local option, would you be willing to also find the places where the graphroot is assumed to be /var/lib/containers/storage and provide an option to override it?

If the path that graphroot points to isn't mounted in the container, there's not much we can do from inside the container itself to reach it automatically. Unless I'm misunderstanding the suggestion.

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 Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Issue or PR with no activity for extended period of time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants