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

docker{,-rootful}.yaml: Use param in docker templates #2515

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

norio-nomura
Copy link
Contributor

@norio-nomura norio-nomura commented Jul 26, 2024

based on #2498

  • docker.yaml: add .param.CONTAINERD_IMAGE_STORE
    By passing the --set .param.CONTAINERD_IMAGE_STORE=true option to limactl {create,start,edit}, the .features."containerd-snapshotter" option will be enabled in docker/daemon.json inside the VM.
  • docker.yaml: add .param.ROOTFUL
    By passing the --set .param.ROOTFUL=true option to limactl {create,start,edit}, Docker inside the VM will run in rootful mode.
  • docker-rootful.yaml: make everything common except for setting .param.ROOTFUL=true in docker.yaml.

Thanks,

@norio-nomura
Copy link
Contributor Author

Once #2498 is merged, I will rebase onto the main branch and then mark this as ready for review.

@norio-nomura norio-nomura force-pushed the use-param-in-docker-templates branch from 8741129 to e5a135d Compare July 26, 2024 02:19
@AkihiroSuda AkihiroSuda added this to the v0.23.0 milestone Jul 27, 2024
By passing the `--set .param.ContainerdImageStore=true` option to `limactl {create,start,edit}`, the `.features."containerd-snapshotter"` option will be enabled in `docker/daemon.json` inside the VM.

Signed-off-by: Norio Nomura <[email protected]>
By passing the `--set .param.Rootful=true` option to `limactl {create,start,edit}`, Docker inside the VM will run in rootful mode.

Signed-off-by: Norio Nomura <[email protected]>
…m.Rootful=true` in `docker.yaml`.

Signed-off-by: Norio Nomura <[email protected]>
@norio-nomura norio-nomura force-pushed the use-param-in-docker-templates branch from e5a135d to cdb7732 Compare July 28, 2024 00:23
@AkihiroSuda
Copy link
Member

Is this still draft?

@norio-nomura norio-nomura marked this pull request as ready for review July 28, 2024 03:33
@norio-nomura
Copy link
Contributor Author

Could it be that the change to Ready for review will not be notified? I switched to Ready for review.

AkihiroSuda
AkihiroSuda previously approved these changes Jul 28, 2024
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

I would like to see our provisioning scripts written in "baby-bash" to make them easier to understand for beginners.

This is obviously a personal opinion, so maybe wait for some confirmation from other @lima-vm/reviewers in case they don't agree with me. 😄

examples/docker-rootful.yaml Outdated Show resolved Hide resolved
examples/docker-rootful.yaml Outdated Show resolved Hide resolved
examples/docker-rootful.yaml Outdated Show resolved Hide resolved
examples/docker-rootful.yaml Outdated Show resolved Hide resolved
examples/docker-rootful.yaml Outdated Show resolved Hide resolved
examples/docker-rootful.yaml Outdated Show resolved Hide resolved
examples/docker-rootful.yaml Outdated Show resolved Hide resolved
examples/docker-rootful.yaml Outdated Show resolved Hide resolved
- Use `if then else fi` instead of `||`
- Use long-from options
- Omit double quotes during variable expansion where it is clear that spaces are not included
- Use upper case on param variable names
- Use wrapper functions instead variable expansion. e.g.(`systemctl_wrapper`)
- Assign param variable to shell variable to making it easier to read cloud-init-output.log
- Remove `systemctl --user start dbus` since it not required any more
- Add some comments to describe the intentions that are difficult to infer from the code

Signed-off-by: Norio Nomura <[email protected]>
@norio-nomura
Copy link
Contributor Author

Updated to apply reviews

@norio-nomura
Copy link
Contributor Author

Updated PR description:

  • .param.ContainerdImageStore -> .param.CONTAINERD_IMAGE_STORE
  • .param.Rootful -> .param.ROOTFUL

@afbjorklund
Copy link
Member

Looks super-complicated to me.

@AkihiroSuda
Copy link
Member

Looks super-complicated to me.

param.CONTAINERD_IMAGE_STORE is still acceptable?

@jandubois
Copy link
Member

Looks super-complicated to me.

param.CONTAINERD_IMAGE_STORE is still acceptable?

I think we should go through a few rounds of simplifying it before passing judgement.

@norio-nomura
Copy link
Contributor Author

For param.CONTAINERD_IMAGE_STORE, refer to this Docker Desktop page.
Enabling this option in a Docker host started with Lima allows building multi-platform images without creating a builder instance using docker buildx create.
The param feature was originally created to make it easy to toggle this option on and off.

@norio-nomura
Copy link
Contributor Author

Looks super-complicated to me.

Since this option allows you to turn complicated settings on and off with a single option, it’s naturally going to seem complicated.
If the way I've written it makes it look more complicated, I'm sorry, but I don't know how to improve it.

@jandubois
Copy link
Member

Sorry, I didn't get around to do another round of code review yet; I wrote #2520 instead to explain my general view/vision of where we should be going with this.

I would like to continue to discuss this PR to see if we can't get it into a shape where we agree that having a combined template is better than 2 separate ones. I don't know if we will get there, but I think we should try.

@afbjorklund
Copy link
Member

afbjorklund commented Jul 30, 2024

If the way I've written it makes it look more complicated, I'm sorry, but I don't know how to improve it.

You don't have to be sorry, it was a hot day and it felt "too long" - I should have given it a proper review.

My bad.

@afbjorklund
Copy link
Member

param.CONTAINERD_IMAGE_STORE is still acceptable?

I still feel bad about param.ContainerdImageStore, but I was referring to the length of the new templates (doubled).

Switching from Bourne shell to Bash, also takes some getting used to. Simple things like readonly and function.

@AkihiroSuda AkihiroSuda mentioned this pull request Jul 30, 2024
@AkihiroSuda AkihiroSuda removed this from the v0.23.0 milestone Jul 30, 2024
@jandubois
Copy link
Member

I'm sorry, but I don't know how to improve it.

I just wanted to let you know that this issue is still on my radar, but I will be offline/travelling until the end of the month and won't have time to look at it again until I'm back.

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.

4 participants