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.

Sign up for GitHub

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

Jump to bottom

Enable setting cgroup v2 mode in std edition #114

Closed
wants to merge 1 commit into from
Closed

Enable setting cgroup v2 mode in std edition #114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ ${DOCKER} run --rm \
-v "${PWD}/cri-dockerd-${CRI_DOCKERD_VERSION}-${ARCH}.LICENSE:/home/build/cri-dockerd.license:ro" \
-v "${PWD}/sshd.pam:/home/build/sshd.pam:ro" \
$(env | grep ^LIMA_ | xargs -n 1 printf -- '-e %s ') \
-e "LIMA_INSTALL_CGROUP_CONF=${LIMA_INSTALL_CGROUP_CONF}" \
Copy link
Member

Choose a reason for hiding this comment

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

This line is not needed; the line above is exporting all LIMA_* environment variables into the container automatically. The 3 variables below are exceptions because they are renaming variables that don't start with LIMA_* on the host.

-e "LIMA_REPO_VERSION=${REPO_VERSION}" \
-e "LIMA_BUILD_ID=${BUILD_ID}" \
-e "LIMA_VARIANT_ID=${EDITION}" \
Expand Down
2 changes: 2 additions & 0 deletions edition/std
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,5 @@ LIMA_INSTALL_LOGROTATE=true
LIMA_INSTALL_OPENSSH_SFTP_SERVER=true
LIMA_INSTALL_SSHFS=true
LIMA_INSTALL_TINI=true

LIMA_INSTALL_CGROUP_CONF=true
Copy link
Member

Choose a reason for hiding this comment

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

Instead of 2 variables (LIMA_INSTALL_CGROUP_CONF and LIMA_ALPINE_CGROUP_MODE) I would prefer to have just a single variable LIMA_CGROUP_MODE that is set to the mode value, e.g.

Suggested change
LIMA_INSTALL_CGROUP_CONF=true
LIMA_CGROUP_MODE=hybrid

But given that this is the default, this setting should not be made in edition/std at all. Instead there should be a line in edition/min that just documents that the variable exists:

export LIMA_CGROUP_MODE=

Note that I removed the ALPINE_ substring from the variable name; it feels redundant to me, as every setting in this repo is about building an Alpine image.

5 changes: 5 additions & 0 deletions genapkovl-lima.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,11 @@ if [ "${LIMA_INSTALL_CRI_DOCKERD}" == "true" ]; then
cp /home/build/cri-dockerd.license "${tmp}/usr/share/doc/cri-dockerd/LICENSE"
fi

if [ "${LIMA_INSTALL_CGROUP_CONF}" == "true" ]; then
mkdir -p "${tmp}/etc/conf.d"
echo 'rc_cgroup_mode=${LIMA_ALPINE_CGROUP_MODE:-hybrid}' >> ${tmp}/etc/conf.d/cgroups
fi
Comment on lines +296 to +299
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if [ "${LIMA_INSTALL_CGROUP_CONF}" == "true" ]; then
mkdir -p "${tmp}/etc/conf.d"
echo 'rc_cgroup_mode=${LIMA_ALPINE_CGROUP_MODE:-hybrid}' >> ${tmp}/etc/conf.d/cgroups
fi
if [ -n "${LIMA_CGROUP_MODE}" ]; then
mkdir -p "${tmp}/etc/conf.d"
echo 'rc_cgroup_mode=${LIMA_CGROUP_MODE}' >> ${tmp}/etc/conf.d/cgroups
fi

Copy link
Author

@na0x2c6 na0x2c6 Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@jandubois

Thank you for your review and suggestion. I apologize for my insufficient explanation.

In this PR, the intention is not to specify the cgroup mode at the time of the Alpine image build, but rather to make it possible to set the cgroup mode at the time of lima boot. Specifically, I intended to start the lima machine as rc_cgroup_mode=unified when the lima template is set as follows:

env:
  LIMA_ALPINE_CGROUP_MODE: unified

In other words, this PR creates a file /etc/conf.d/cgroups with the content:

rc_cgroup_mode=${LIMA_ALPINE_CGROUP_MODE:-hybrid}

So that at the time of lima machine startup:

  • If the LIMA_ALPINE_CGROUP_MODE environment variable is NOT set in the lima template, cgroup will be (default value) hybrid
  • If the LIMA_ALPINE_CGROUP_MODE environment variable is set in the lima template, cgroup will be set to that value.

With the above, at machine startup, cgroups can be selected using environment variables in a lima template, eliminating the need to prepare separate Alpine images for each cgroup mode.

(I included the string ALPINE_ because it becomes an option at lima startup, but I think it's actually redundant as you said...)

Based on the above intentions, would you say the modification is appropriate? Or is the modification policy inappropriate?

Copy link
Author

Choose a reason for hiding this comment

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

I have uploaded the images, which can be used to test the above behavior, at the following location:

https://github.com/na0x2c6/alpine-lima/releases/tag/v0.2.32-std-cgroup-experimental


mkdir -p "${tmp}/etc"
mkdir -p "${tmp}/proc"
mkdir -p "${tmp}/usr"
Expand Down
Loading