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

Move disable checks out of systemd generator #4399

Merged
merged 7 commits into from
Oct 9, 2023
Merged

Move disable checks out of systemd generator #4399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e8fda14
fix: cloud-init status --wait broken with KERNEL_CMDLINE (#4399)
holmanb Sep 6, 2023
2b33a31
doc: Describe disabling cloud-init using an environment variable (#4399)
holmanb Sep 7, 2023
f6d784e
tests/integration_tests: add cloud-init disablement coverage (#4399)
holmanb Sep 6, 2023
8e5a59f
benchmark: benchmark cloud-init-generator independent of ds-identify …
holmanb Aug 31, 2023
861a878
systemd: Standardize cloud-init systemd enablement (#4399)
holmanb Sep 11, 2023
316e54d
cloud-init-generator: Various performance optimizations (#4399)
holmanb Sep 11, 2023
8ab927a
ds-identify: exit 2 on disabled state from marker or cmdline (#4399)
blackboxsw Sep 26, 2023
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 Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ render-template:
# Our generator is a shell script. Make it easy to measure the
# generator. This should be monitored for performance regressions
benchmark-generator: FILE=$(GENERATOR_F).tmpl
benchmark-generator: VARIANT="benchmark"
benchmark-generator: export ITER=$(NUM_ITER)
benchmark-generator: render-template
$(BENCHMARK) $(GENERATOR_F)
Expand Down
12 changes: 12 additions & 0 deletions cloudinit/cmd/status.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ class UXAppBootStatusCode(enum.Enum):
DISABLED_BY_GENERATOR = "disabled-by-generator"
DISABLED_BY_KERNEL_CMDLINE = "disabled-by-kernel-cmdline"
DISABLED_BY_MARKER_FILE = "disabled-by-marker-file"
DISABLED_BY_ENV_VARIABLE = "disabled-by-environment-variable"
ENABLED_BY_GENERATOR = "enabled-by-generator"
ENABLED_BY_KERNEL_CMDLINE = "enabled-by-kernel-cmdline"
ENABLED_BY_SYSVINIT = "enabled-by-sysvinit"
Expand All @@ -54,6 +55,7 @@ class UXAppBootStatusCode(enum.Enum):
UXAppBootStatusCode.DISABLED_BY_GENERATOR,
UXAppBootStatusCode.DISABLED_BY_KERNEL_CMDLINE,
UXAppBootStatusCode.DISABLED_BY_MARKER_FILE,
UXAppBootStatusCode.DISABLED_BY_ENV_VARIABLE,
]
)

Expand Down Expand Up @@ -184,6 +186,16 @@ def get_bootstatus(disable_file, paths) -> Tuple[UXAppBootStatusCode, str]:
elif "cloud-init=disabled" in cmdline_parts:
bootstatus_code = UXAppBootStatusCode.DISABLED_BY_KERNEL_CMDLINE
reason = "Cloud-init disabled by kernel parameter cloud-init=disabled"
elif "cloud-init=disabled" in os.environ.get("KERNEL_CMDLINE", "") or (
uses_systemd()
and "cloud-init=disabled"
in subp.subp(["systemctl", "show-environment"]).stdout
):
bootstatus_code = UXAppBootStatusCode.DISABLED_BY_ENV_VARIABLE
reason = (
"Cloud-init disabled by environment variable "
"KERNEL_CMDLINE=cloud-init=disabled"
)
elif os.path.exists(os.path.join(paths.run_dir, "disabled")):
bootstatus_code = UXAppBootStatusCode.DISABLED_BY_GENERATOR
reason = "Cloud-init disabled by cloud-init-generator"
Expand Down
17 changes: 13 additions & 4 deletions doc/rtd/howto/disable_cloud_init.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ How to disable cloud-init
One may wish to disable cloud-init to ensure that it doesn't do anything on
subsequent boots. Some parts of cloud-init may run once per boot otherwise.

There are two cross-platform methods of disabling ``cloud-init``.
There are three cross-platform methods of disabling ``cloud-init``.

Method 1: text file
====================
Expand Down Expand Up @@ -34,6 +34,15 @@ Example (using GRUB2 with Ubuntu):
$ echo 'GRUB_CMDLINE_LINUX="cloud-init=disabled"' >> /etc/default/grub
$ grub-mkconfig -o /boot/efi/EFI/ubuntu/grub.cfg

.. note::
When running in containers, ``cloud-init`` will read an environment
variable named ``KERNEL_CMDLINE`` in place of a kernel commandline.
Method 3: environment variable
==============================

To disable cloud-init, pass the environment variable
``KERNEL_CMDLINE=cloud-init=disabled`` into each of cloud-init's
processes.

Example (using systemd):

.. code-block::

$ echo "DefaultEnvironment=KERNEL_CMDLINE=cloud-init=disabled" >> /etc/systemd/system.conf
3 changes: 1 addition & 2 deletions systemd/cloud-config.service.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,9 @@ After=network-online.target cloud-config.target
After=snapd.seeded.service
Before=systemd-user-sessions.service
Wants=network-online.target cloud-config.target
{% if variant == "rhel" %}
ConditionPathExists=!/etc/cloud/cloud-init.disabled
ConditionKernelCommandLine=!cloud-init=disabled
{% endif %}
ConditionEnvironment=!KERNEL_CMDLINE=cloud-init=disabled

[Service]
Type=oneshot
Expand Down
3 changes: 1 addition & 2 deletions systemd/cloud-final.service.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,9 @@ After=multi-user.target
Before=apt-daily.service
{% endif %}
Wants=network-online.target cloud-config.service
{% if variant == "rhel" %}
ConditionPathExists=!/etc/cloud/cloud-init.disabled
ConditionKernelCommandLine=!cloud-init=disabled
{% endif %}
ConditionEnvironment=!KERNEL_CMDLINE=cloud-init=disabled


[Service]
Expand Down
136 changes: 34 additions & 102 deletions systemd/cloud-init-generator.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ CLOUD_SYSTEM_TARGET="/lib/systemd/system/cloud-init.target"
{% if variant in ["almalinux", "centos", "cloudlinux", "eurolinux", "fedora",
"miraclelinux", "openeuler", "OpenCloudOS", "openmandriva", "rhel", "rocky", "TencentOS", "virtuozzo"] %}
dsidentify="/usr/libexec/cloud-init/ds-identify"
{% elif variant == "benchmark" %}
dsidentify="/bin/true"
{% else %}
dsidentify="/usr/lib/cloud-init/ds-identify"
{% endif %}
Expand All @@ -40,105 +42,51 @@ debug() {
echo "$@" >> "$LOG"
}

etc_file() {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is now handled by ConditionPathExists.

local pprefix="${1:-/etc/cloud/cloud-init.}"
_RET="unset"
[ -f "${pprefix}$ENABLE" ] && _RET="$ENABLE" && return 0
[ -f "${pprefix}$DISABLE" ] && _RET="$DISABLE" && return 0
return 0
}

read_proc_cmdline() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Both container and non-container cases are handled by ConditionKernelCommandline

# return /proc/cmdline for non-container, and /proc/1/cmdline for container
local ctname="systemd"
if [ -n "$CONTAINER" ] && ctname=$CONTAINER ||
systemd-detect-virt --container --quiet; then
if { _RET=$(tr '\0' ' ' < /proc/1/cmdline); } 2>/dev/null; then
_RET_MSG="container[$ctname]: pid 1 cmdline"
return
fi
_RET=""
_RET_MSG="container[$ctname]: pid 1 cmdline not available"
return 0
fi

_RET_MSG="/proc/cmdline"
read _RET < /proc/cmdline
}

kernel_cmdline() {
local cmdline="" tok=""
if [ -n "${KERNEL_CMDLINE+x}" ]; then
Copy link
Member Author

Choose a reason for hiding this comment

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

KERNEL_CMDLINE is now handled by ConditionEnvironment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes and no. It is not handled from a generator perspective. If KERNEL_CMDLINE=cloud-init=disabled exists there is really nothing for the generator to do and it should just exit as systemd will handle the rest via the ConditionEnvironment setting. But this does not apply to the generator and as such dsidentify is still run, which we shouldn't really do if the user disabled cloud-init.

I think the generator should specifically look for KERNEL_CMDLINE=cloud-init=disabled and if found exit and do nothing else.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is addressed in the latest commit.

# use KERNEL_CMDLINE if present in environment even if empty
cmdline=${KERNEL_CMDLINE}
debug 1 "kernel command line from env KERNEL_CMDLINE: $cmdline"
else
read_proc_cmdline && cmdline="$_RET" &&
debug 1 "kernel command line ($_RET_MSG): $cmdline"
fi
_RET="unset"
cmdline=" $cmdline "
tok=${cmdline##* cloud-init=}
[ "$tok" = "$cmdline" ] && _RET="unset"
tok=${tok%% *}
[ "$tok" = "$ENABLE" -o "$tok" = "$DISABLE" ] && _RET="$tok"
return 0
}

default() {
_RET="$ENABLE"
}

check_for_datasource() {
Copy link
Member Author

Choose a reason for hiding this comment

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

No need to define a function that gets called once. Function definition and subsequent lookup at invocation time have costs. Inline it.

local ds_rc=""
if [ ! -x "$dsidentify" ]; then
debug 1 "no ds-identify in $dsidentify"
return 0
fi
$dsidentify
ds_rc=$?
debug 1 "ds-identify rc=$ds_rc"
if [ "$ds_rc" = "0" ]; then
return 0
fi
return 1
}

main() {
local normal_d="$1" early_d="$2" late_d="$3"
local target_name="multi-user.target" gen_d="$early_d"
local link_path="$gen_d/${target_name}.wants/${CLOUD_TARGET_NAME}"
local ds=""
local ds="" ret=""

debug 1 "$0 normal=$normal_d early=$early_d late=$late_d"
debug 2 "$0 $*"

local search result="error" ret=""
Copy link
Member Author

Choose a reason for hiding this comment

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

Setting result to $ENABLE or $DISABLE is unnecessary to accomplish this logic and adds a variable (which adds lookup time) and unnecessary branches (more time cost) and IMO is harder to read. Shift code around to avoid these conditionals and variable usage.

TL;DR: Do the same thing, with less.

for search in kernel_cmdline etc_file default; do
if $search; then
debug 1 "$search found $_RET"
[ "$_RET" = "$ENABLE" -o "$_RET" = "$DISABLE" ] &&
result=$_RET && break
else
ret=$?
debug 0 "search $search returned $ret"
fi
done
# ds=found => enable
# ds=notfound => disable
# <any> => disable
debug 1 "checking for datasource"

# enable AND ds=found == enable
# enable AND ds=notfound == disable
# disable || <any> == disabled
if [ "$result" = "$ENABLE" ]; then
debug 1 "checking for datasource"
check_for_datasource
ds=$?
if [ ! -x "$dsidentify" ]; then
debug 1 "no ds-identify in $dsidentify"
ds=0
fi
$dsidentify
ds=$?
debug 1 "ds-identify rc=$ds"

if [ "$ds" = "1" -o "$ds" = "2" ]; then
if [ "$ds" = "1" ]; then
debug 1 "cloud-init is enabled but no datasource found, disabling"
result="$DISABLE"
else
debug 1 "cloud-init is disabled by kernel commandline or etc_file"
fi
fi
if [ -f "$link_path" ]; then
if rm -f "$link_path"; then
debug 1 "disabled. removed existing $link_path"
else
ret=$?
debug 0 "[$ret] disable failed, remove $link_path"
fi
else
debug 1 "already disabled: no change needed [no $link_path]"
fi
if [ -e "$RUN_ENABLED_FILE" ]; then
debug 1 "removing $RUN_ENABLED_FILE and creating $RUN_DISABLED_FILE"
rm -f "$RUN_ENABLED_FILE"
fi
: > "$RUN_DISABLED_FILE"

if [ "$result" = "$ENABLE" ]; then
elif [ "$ds" = "0" ]; then
if [ -e "$link_path" ]; then
debug 1 "already enabled: no change needed"
else
Expand All @@ -157,22 +105,6 @@ main() {
rm -f $RUN_DISABLED_FILE
fi
: > "$RUN_ENABLED_FILE"
elif [ "$result" = "$DISABLE" ]; then
if [ -f "$link_path" ]; then
if rm -f "$link_path"; then
debug 1 "disabled. removed existing $link_path"
else
ret=$?
debug 0 "[$ret] disable failed, remove $link_path"
fi
else
debug 1 "already disabled: no change needed [no $link_path]"
fi
if [ -e "$RUN_ENABLED_FILE" ]; then
debug 1 "removing $RUN_ENABLED_FILE and creating $RUN_DISABLED_FILE"
rm -f "$RUN_ENABLED_FILE"
fi
: > "$RUN_DISABLED_FILE"
else
debug 0 "unexpected result '$result' 'ds=$ds'"
ret=3
Expand Down
3 changes: 1 addition & 2 deletions systemd/cloud-init-local.service.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,9 @@ Before=sysinit.target
Conflicts=shutdown.target
{% endif %}
RequiresMountsFor=/var/lib/cloud
{% if variant == "rhel" %}
ConditionPathExists=!/etc/cloud/cloud-init.disabled
ConditionKernelCommandLine=!cloud-init=disabled
{% endif %}
ConditionEnvironment=!KERNEL_CMDLINE=cloud-init=disabled

[Service]
Type=oneshot
Expand Down
3 changes: 1 addition & 2 deletions systemd/cloud-init.service.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,9 @@ Conflicts=shutdown.target
Before=shutdown.target
Conflicts=shutdown.target
{% endif %}
{% if variant == "rhel" %}
ConditionPathExists=!/etc/cloud/cloud-init.disabled
ConditionKernelCommandLine=!cloud-init=disabled
{% endif %}
ConditionEnvironment=!KERNEL_CMDLINE=cloud-init=disabled

[Service]
Type=oneshot
Expand Down
3 changes: 3 additions & 0 deletions systemd/cloud-init.target
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,6 @@
[Unit]
Description=Cloud-init target
After=multi-user.target
ConditionPathExists=!/etc/cloud/cloud-init.disabled
ConditionKernelCommandLine=!cloud-init=disabled
ConditionEnvironment=!KERNEL_CMDLINE=cloud-init=disabled
Comment on lines +13 to +15
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like these additional condition checks for visibility at systemctl status cloud-init.target level, but this duplicates conditions we've decided to keep on each subordinate cloud-*.service file and we don't strictly need them if we are going to preserve the checks on the individual units. Can we measure if there is a cost to these duplicated conditions? It does make systemd dependency resolver do a little but more as far as checks for this target unit file.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll measure and report back.

Copy link
Member Author

Choose a reason for hiding this comment

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

@blackboxsw see the section in the PR description titled "Measuring with and without conditionals in .target" for the numbers

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @holmanb. This looks good to me as well as your assessment above. Also I triggered those compound conditionals directly on the systemd-analyze conditional subcommand with comparable results. The avg time seems to be +- one millisecond with or without the 3 extra checks.

root@schema-m:~# time systemd-analyze condition 'ConditionKernelCommandLine=!cloud-init=disabled'
test.service: ConditionKernelCommandLine=!cloud-init=disabled succeeded.
Conditions succeeded.

real	0m0.007s
user	0m0.000s
sys	0m0.007s
root@schema-m:~# time systemd-analyze condition 'ConditionKernelCommandLine=!cloud-init=disabled' 'ConditionEnvironment=!KERNEL_CMDLINE=cloud-init=disabled' 'ConditionPathExists=!/etc/cloud/cloud-init.disabled' 'ConditionKernelCommandLine=!cloud-init=disabled' 'ConditionEnvironment=!KERNEL_CMDLINE=cloud-init=disabled'
test.service: ConditionEnvironment=!KERNEL_CMDLINE=cloud-init=disabled succeeded.
test.service: ConditionKernelCommandLine=!cloud-init=disabled succeeded.
test.service: ConditionPathExists=!/etc/cloud/cloud-init.disabled succeeded.
test.service: ConditionEnvironment=!KERNEL_CMDLINE=cloud-init=disabled succeeded.
test.service: ConditionKernelCommandLine=!cloud-init=disabled succeeded.
Conditions succeeded.

real	0m0.007s
user	0m0.000s
sys	0m0.007s

Loading
Loading