Remove dependency on bash #5120

holmanb · 2024-03-29T23:59:41Z

Context

This change removes cloud-init's dependency on bash in both runtime code and in unittests.

There are two runtime changes in this PR:

It standardizes the failure path and tests for systems that don't include GNU date. This fallback path to GNU date isn't used in 99.99% of cases. It is only exercised when a user has set a custom datetime format in their logs which Python code doesn't understand. GNU date offers a parsing capability which is used only in these cases.
The main change in this PR is adding support for posix shell in the OpenNebula datasource. It is of course possible for the OpenNebula cloud (or the end user) to pass non-posix shell data to cloud-init, but with this change at least cloud-init codepaths exist for posix shell.

Test Steps

incus create images:alpine/edge alpine
incus start alpine
incus shell alpine
$ apk add py3-tox git
$ git clone --branch holmanb/no-bash-deps https://github.com/holmanb/cloud-init.git
$ cd cloud-init
$ tox -e py3

also see the ci report in this pr which adds alpine to ci

Merge type

Squash merge using "Proposed Commit Message"
Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

igalic

first bleary eyed look, found only a typo

tests/unittests/test_subp.py

TheRealFalcon

This generally LGTM. I think there's some further cleanup we could do here around the old implementation, but it's not necessary for this PR.

Only comment I think needs to be addressed here is the extra unused variable and IFS in the new implementation.

cloudinit/sources/DataSourceOpenNebula.py

TheRealFalcon · 2024-04-09T13:54:03Z

cloudinit/sources/DataSourceOpenNebula.py

@@ -356,29 +448,18 @@ def varprinter(vlist):
        + varprinter(allvars)
        + "{\n%s\n\n:\n} > /dev/null\n" % content
        + "unset IFS\n"
-        + varprinter(keylist)
+        + varprinter(allvars)


Does this version even work? If I look at the printf on line 428, it's missing a newline so we'll get a bunch of variables crammed onto a single line. Is there a reason we can't replace the whole varprinter mess with set -o posix; set?

Does this version even work?

I think so. I had to tear it apart to rebuild it in posix shell, and I recall that this did what I think it was supposed to. I think that the line containing: for line in output.split("\x00"): is crucial to null-delimited parsing stuff.

Is there a reason we can't replace the whole varprinter mess with set -o posix; set?

I was trying to avoid disturbing sleeping dragons with excessive changes, but I just checked[1] and bash's set output multi-line strings rather than ansi-c strings, so it probably would work. We'd just need to unset that -o posix if it wasn't already set. Bash stores shell options in SHELLOPTS, so I think we have all of the bits required to make this a single codepath.

I previously thought that ansi-c multi-line strings was technically posix-compliant[2] and possible output from set as "appropriate quoting". Posix shells like dash and ash certainly understand them. However, digging further it seems that maybe it's actually an extension that was backported into these shells for support, which is probably why -o posix disables this output format.

[1] bash with posix -o

$ bash -c "TEST=$'multi-\nline'; set" | grep TEST ... TEST=$'multi-\nline' $ bash -c "TEST=$'multi-\nline'; set -o posix; set" | grep -A 1 TEST ... TEST='multi- line'

[2] the spec

If no options or arguments are specified, set shall write the names and values of all shell variables in the collation sequence of the current locale. Each name shall start on a separate line, using the format:

"%s=%s\n", ,

The value string shall be written with appropriate quoting; see the description of shell quoting in Quoting. The output shall be suitable for reinput to the shell, setting or resetting, as far as possible, the variables that are currently set; read-only variables cannot be reset.

Is there a reason we can't replace the whole varprinter mess with set -o posix; set?

It looks like that works. See the latest commit which consolidates the bash and posix shell codepaths into a single one using set -o posix

cloudinit/sources/DataSourceOpenNebula.py

holmanb · 2024-04-09T23:55:06Z

@TheRealFalcon I think I've addressed all of your comments (and I tested on alpine), ready for re-review.

TheRealFalcon

LGTM! Great cleanup here.

holmanb · 2024-04-10T16:01:07Z

LGTM! Great cleanup here.

Thanks @TheRealFalcon! I'll wait to merge in case @dermotbradley has feedback.

dermotbradley · 2024-04-12T19:43:15Z

I'll wait to merge in case @dermotbradley has feedback.

It seems fine to me and the modified testcases still pass on Alpine.

A hard dependency on bash is undesirable for many reasons. Use 'set' for getting environment variables, rather than bash features. This also works with bash in posix mode. Note: The choice to make the datasource configuration implementation a bash script was highly unusual. Unfortunately the format is in OpenNebula's control, not cloud-init's. Still, adding posix shell support to cloud-init's implementation is an incremental improvement. [1] by outputting ansi-c strings - $'' - rather than multi-line strings

holmanb · 2024-04-12T20:37:00Z

Thanks @dermotbradley! I just squashed the extra commits and updated commit messages, will merge shortly.

holmanb mentioned this pull request Apr 1, 2024

Add alpine unittests to ci #5121

Merged

2 tasks

igalic suggested changes Apr 4, 2024

View reviewed changes

tests/unittests/test_subp.py Outdated Show resolved Hide resolved

holmanb added the Needs Review label Apr 6, 2024

holmanb force-pushed the holmanb/no-bash-deps branch from ca18d74 to 0654f15 Compare April 8, 2024 19:28

aciba90 self-assigned this Apr 9, 2024

TheRealFalcon requested changes Apr 9, 2024

View reviewed changes

aciba90 removed their assignment Apr 9, 2024

TheRealFalcon removed the Needs Review label Apr 9, 2024

TheRealFalcon self-assigned this Apr 9, 2024

holmanb force-pushed the holmanb/no-bash-deps branch 2 times, most recently from 62bf14f to 53ef8a5 Compare April 9, 2024 23:41

holmanb requested a review from TheRealFalcon April 9, 2024 23:55

TheRealFalcon approved these changes Apr 10, 2024

View reviewed changes

holmanb added 2 commits April 12, 2024 13:51

test: Eliminate bash dependency from subp tests

49914bc

test: Make analyze tests not depend on GNU date

21450b2

holmanb force-pushed the holmanb/no-bash-deps branch from 53ef8a5 to e866104 Compare April 12, 2024 19:52

holmanb force-pushed the holmanb/no-bash-deps branch from e866104 to 41fc899 Compare April 12, 2024 20:31

holmanb merged commit 197409e into canonical:main Apr 12, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dependency on bash #5120

Remove dependency on bash #5120

holmanb commented Mar 29, 2024 •

edited

Loading

igalic left a comment

TheRealFalcon left a comment

TheRealFalcon Apr 9, 2024

holmanb Apr 9, 2024 •

edited

Loading

holmanb Apr 9, 2024 •

edited

Loading

holmanb commented Apr 9, 2024 •

edited

Loading

TheRealFalcon left a comment

holmanb commented Apr 10, 2024

dermotbradley commented Apr 12, 2024

holmanb commented Apr 12, 2024

Remove dependency on bash #5120

Remove dependency on bash #5120

Conversation

holmanb commented Mar 29, 2024 • edited Loading

Context

Test Steps

Merge type

igalic left a comment

Choose a reason for hiding this comment

TheRealFalcon left a comment

Choose a reason for hiding this comment

TheRealFalcon Apr 9, 2024

Choose a reason for hiding this comment

holmanb Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

holmanb Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

holmanb commented Apr 9, 2024 • edited Loading

TheRealFalcon left a comment

Choose a reason for hiding this comment

holmanb commented Apr 10, 2024

dermotbradley commented Apr 12, 2024

holmanb commented Apr 12, 2024

holmanb commented Mar 29, 2024 •

edited

Loading

holmanb Apr 9, 2024 •

edited

Loading

holmanb Apr 9, 2024 •

edited

Loading

holmanb commented Apr 9, 2024 •

edited

Loading