fix(runpath): On *BSD, create /run is not ephemeral #4677

igalic · 2023-12-08T19:39:57Z

Proposed Commit Message

fix(runpath): On *BSD, /run is not ephemeral

On *BSD, `/run` is not ephemeral, but the way cloud-init behaves, it
expects it to be. To fix this, we ds-identify's RUN_* initialization after
figuring out which OS we run on.
For the Python we set the config to the expected directories.

Add and fix tests to ensure this behavior is correct and consistent.

Sponsored by: The FreeBSD Foundation
Fixes GH-4180
Fixes GH-4231

Additional Context

Test Steps

Checklist

My code follows the process laid out in the documentation
I have updated or added any unit tests accordingly
I have updated or added any documentation accordingly

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>)

On *BSD, `/run` is not ephemeral, but the way cloud-init behaves, it expects it to be. This is hack is partial fix for canonicalGH-4180 / canonicalGH-4231. But to be good Unix citizens, we should still make /run relocatable in the code and in the installer. Sponsored by: The FreeBSD Foundation Fixes canonicalGH-4180 Fixes canonicalGH-4231

let's just use an `if`! Sponsored by: The FreeBSD Foundation

sysvinit/netbsd/cloudfinal.tmpl

sysvinit/freebsd/cloudinitlocal.tmpl

sysvinit/freebsd/dsidentify.tmpl

let's just use an `if`! Sponsored by: The FreeBSD Foundation

igalic

this is some of the best (harmless) shell code I've written yet

tests/unittests/test_ds_identify.py

let's just use an `if`! Sponsored by: The FreeBSD Foundation

shuffle code-paths around such that we initialize RUN_PATH, and all the things that depend on it after figuring out which OS we're on, while still alowing these global variables to be overwritten from the outside. On BSD /run is not ephemeral, so here we set RUN_PATH to /var/run. Ensure everything is initialized for 'print_info', not just for 'main'. For the tests, we equally ensure that for not-Linux systems the data is written into `rootd + var/run` instead of `rootd + run`. Sponsored by: The FreeBSD Foundation

on BSD, /run is not ephemeral. relocate BSDs config to /var/run Sponsored by: The FreeBSD Foundation Fixes canonicalGH-4180 Fixes canonicalGH-4231 Co-authored-by: Brett Holman <brett.holman@canonical.com>

igalic · 2023-12-16T01:28:47Z

tools/ds-identify

@@ -1579,6 +1581,10 @@ collect_info() {
 }

 print_info() {
+    DI_LOG=stderr


my understanding is that this is correct
although it didn't seem to be necessary before

Why do we need the DI_LOG=stderr and ensure_sane_path calls? I don't think that those ones are required.

I think that I see why read_uname_info and set_run_path might be required - this function is the other "main" entry point of this function, thus will require the PATH_RUN* variables to be properly set, as they were before this function.

here's what happens without DI_LOG=stderr:

tools/ds-identify: 185: 3: Bad file descriptor tools/ds-identify: 185: 3: Bad file descriptor

but set_run_path is not, so i removed that.

I meant the other way around:

diff --git a/tools/ds-identify b/tools/ds-identify index 6e923a550..154668a00 100755 --- a/tools/ds-identify +++ b/tools/ds-identify @@ -1584,8 +1584,7 @@ collect_info() { } print_info() { - DI_LOG=stderr - ensure_sane_path + set_run_path read_uname_info collect_info _print_info

set_run_path sets DI_LOG, so I don't think hardcoding it here is necessary. We shouldn't unconditionally log to stderr, in case of DI_MAIN=print_info DI_LOG=/some/file tools/ds-identify or similar.

The above didn't have the bad descriptor issue you mentioned.

Also ensure_sane_path wasn't called in print_info before - I see no reason to add it.

let's just use an `if`! Sponsored by: The FreeBSD Foundation

shuffle code-paths around such that we initialize RUN_PATH, and all the things that depend on it after figuring out which OS we're on, while still alowing these global variables to be overwritten from the outside. On BSD /run is not ephemeral, so here we set RUN_PATH to /var/run. Ensure everything is initialized for 'print_info', not just for 'main'. For the tests, we equally ensure that for not-Linux systems the data is written into `rootd + var/run` instead of `rootd + run`. Sponsored by: The FreeBSD Foundation

on BSD, /run is not ephemeral. relocate BSDs config to /var/run Sponsored by: The FreeBSD Foundation Fixes canonicalGH-4180 Fixes canonicalGH-4231 Co-authored-by: Brett Holman <brett.holman@canonical.com>

holmanb

Great work here @igalic, I like the way this one is coming together. I have a couple smallish requests / questions, once resolved I'll merge.

holmanb · 2024-01-03T23:04:16Z

tools/ds-identify

@@ -1579,6 +1581,10 @@ collect_info() {
 }

 print_info() {
+    DI_LOG=stderr


Why do we need the DI_LOG=stderr and ensure_sane_path calls? I don't think that those ones are required.

I think that I see why read_uname_info and set_run_path might be required - this function is the other "main" entry point of this function, thus will require the PATH_RUN* variables to be properly set, as they were before this function.

holmanb · 2024-01-03T23:05:42Z

tools/ds-identify


-DI_LOG="${DI_LOG:-${PATH_RUN_CI}/ds-identify.log}"
+# Declare global here, we'll set it properly in main()


Lets add a comment explaining that the default values for these variables are system-specific, therefore must be assigned after collecting some information from the system.

tests/unittests/test_ds_identify.py

let's just use an `if`! Sponsored by: The FreeBSD Foundation

shuffle code-paths around such that we initialize RUN_PATH, and all the things that depend on it after figuring out which OS we're on, while still alowing these global variables to be overwritten from the outside. On BSD /run is not ephemeral, so here we set RUN_PATH to /var/run. Ensure everything is initialized for 'print_info', not just for 'main'. For the tests, we equally ensure that for not-Linux systems the data is written into `rootd + var/run` instead of `rootd + run`. Sponsored by: The FreeBSD Foundation

on BSD, /run is not ephemeral. relocate BSDs config to /var/run Sponsored by: The FreeBSD Foundation Fixes canonicalGH-4180 Fixes canonicalGH-4231

let's just use an `if`! Sponsored by: The FreeBSD Foundation

shuffle code-paths around such that we initialize RUN_PATH, and all the things that depend on it after figuring out which OS we're on, while still alowing these global variables to be overwritten from the outside. On BSD /run is not ephemeral, so here we set RUN_PATH to /var/run. Ensure everything is initialized for 'print_info', not just for 'main'. For the tests, we equally ensure that for not-Linux systems the data is written into `rootd + var/run` instead of `rootd + run`. Sponsored by: The FreeBSD Foundation

on BSD, /run is not ephemeral. relocate BSDs config to /var/run Sponsored by: The FreeBSD Foundation Fixes canonicalGH-4180 Fixes canonicalGH-4231

shuffle code-paths around such that we initialize RUN_PATH, and all the things that depend on it after figuring out which OS we're on, while still alowing these global variables to be overwritten from the outside. On BSD /run is not ephemeral, so here we set RUN_PATH to /var/run. Ensure everything is initialized for 'print_info', not just for 'main'. For the tests, we equally ensure that for not-Linux systems the data is written into `rootd + var/run` instead of `rootd + run`. Sponsored by: The FreeBSD Foundation

on BSD, /run is not ephemeral. relocate BSDs config to /var/run Sponsored by: The FreeBSD Foundation Fixes canonicalGH-4180 Fixes canonicalGH-4231

shuffle code-paths around such that we initialize RUN_PATH, and all the things that depend on it after figuring out which OS we're on, while still alowing these global variables to be overwritten from the outside. On BSD /run is not ephemeral, so here we set RUN_PATH to /var/run. Ensure everything is initialized for 'print_info', not just for 'main'. For the tests, we equally ensure that for not-Linux systems the data is written into `rootd + var/run` instead of `rootd + run`. Sponsored by: The FreeBSD Foundation

on BSD, /run is not ephemeral. relocate BSDs config to /var/run Sponsored by: The FreeBSD Foundation Fixes canonicalGH-4180 Fixes canonicalGH-4231

igalic

i think this is done now…

igalic · 2024-01-04T23:19:38Z

tools/ds-identify

@@ -1835,12 +1843,25 @@ read_uptime() {
    return
 }

+set_run_path() {
+    if [ "$DI_UNAME_KERNEL_NAME" != "Linux" ]; then


make sure this function works on Linux… even if someone forgot to call read_uname_info()…

igalic · 2024-01-04T23:20:21Z

tools/ds-identify

@@ -154,6 +159,8 @@ debug() {
    shift
    [ "$lvl" -gt "${DI_DEBUG_LEVEL}" ] && return

+    [ "$DI_LOG" = "" ] && DI_LOG="stderr"


make sure the debug() function works, even if someone forgot to set_run_path(), or DI_LOG.

Mentally walking through this:

call to debug before set_run_path causes DI_LOG set to stderr, _DI_LOGGED set to stderr, file descriptor 3 redirects to file descriptor 2

set_run_path redefines DI_LOG to whatever/cloud-init.log

subsequent call to debug sees that "$_DI_LOGGED" != "$DI_LOG", re-opens fd 3 pointing to whatever/cloud-init.log, reassigns _DI_LOGGED to $DI_LOG

subsequent calls to debug use fd 3 which points to the right file

Looks good to me!

holmanb

Great work @igalic. This looks good to me.

holmanb · 2024-01-05T00:13:59Z

tools/ds-identify

@@ -154,6 +159,8 @@ debug() {
    shift
    [ "$lvl" -gt "${DI_DEBUG_LEVEL}" ] && return

+    [ "$DI_LOG" = "" ] && DI_LOG="stderr"


Mentally walking through this:

call to debug before set_run_path causes DI_LOG set to stderr, _DI_LOGGED set to stderr, file descriptor 3 redirects to file descriptor 2

set_run_path redefines DI_LOG to whatever/cloud-init.log

subsequent call to debug sees that "$_DI_LOGGED" != "$DI_LOG", re-opens fd 3 pointing to whatever/cloud-init.log, reassigns _DI_LOGGED to $DI_LOG

subsequent calls to debug use fd 3 which points to the right file

Looks good to me!

let's just use an `if`! Sponsored by: The FreeBSD Foundation

shuffle code-paths around such that we initialize RUN_PATH, and all the things that depend on it after figuring out which OS we're on, while still alowing these global variables to be overwritten from the outside. On BSD /run is not ephemeral, so here we set RUN_PATH to /var/run. Ensure everything is initialized for 'print_info', not just for 'main'. For the tests, we equally ensure that for not-Linux systems the data is written into `rootd + var/run` instead of `rootd + run`. Sponsored by: The FreeBSD Foundation

Changelog(s): - https://github.com/canonical/cloud-init/releases/tag/23.4.1 - https://github.com/canonical/cloud-init/releases/tag/23.4 Note that net/cloud-init-devel finally relocates /run to /var/run See: canonical/cloud-init#4677 PR: 275896

igalic force-pushed the hack/ephemeral-run branch from 6482cc4 to 3035854 Compare December 8, 2023 19:40

igalic added a commit to igalic/cloud-init that referenced this pull request Dec 8, 2023

fix(sysvinit): make code a bit more consistent (canonical#4677)

3035854

let's just use an `if`! Sponsored by: The FreeBSD Foundation

holmanb self-assigned this Dec 11, 2023

holmanb reviewed Dec 11, 2023

View reviewed changes

sysvinit/netbsd/cloudfinal.tmpl Show resolved Hide resolved

sysvinit/freebsd/cloudinitlocal.tmpl Show resolved Hide resolved

sysvinit/freebsd/dsidentify.tmpl Outdated Show resolved Hide resolved

sysvinit/freebsd/dsidentify.tmpl Outdated Show resolved Hide resolved

igalic force-pushed the hack/ephemeral-run branch from 3035854 to 6ab2f83 Compare December 13, 2023 15:52

igalic added a commit to igalic/cloud-init that referenced this pull request Dec 13, 2023

fix(sysvinit): make code a bit more consistent (canonical#4677)

16aa69a

let's just use an `if`! Sponsored by: The FreeBSD Foundation

igalic marked this pull request as draft December 13, 2023 16:00

igalic changed the title ~~hack(sysvinit): On *BSD, create /run as symlink~~ fix(runpath): On *BSD, create /run is not ephemeral Dec 13, 2023

igalic force-pushed the hack/ephemeral-run branch from 6ab2f83 to 12bcdfd Compare December 13, 2023 23:42

igalic added a commit to igalic/cloud-init that referenced this pull request Dec 13, 2023

fix(sysvinit): make code a bit more consistent (canonical#4677)

7aa56ed

let's just use an `if`! Sponsored by: The FreeBSD Foundation

igalic force-pushed the hack/ephemeral-run branch from 12bcdfd to 10e6d4c Compare December 14, 2023 12:07

igalic marked this pull request as ready for review December 14, 2023 12:35

igalic commented Dec 14, 2023

View reviewed changes

holmanb reviewed Dec 15, 2023

View reviewed changes

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

igalic force-pushed the hack/ephemeral-run branch from 0a274a1 to e2c4cdb Compare December 15, 2023 10:12

igalic added a commit to igalic/cloud-init that referenced this pull request Dec 15, 2023

fix(sysvinit): make code a bit more consistent (canonical#4677)

0e71eb7

let's just use an `if`! Sponsored by: The FreeBSD Foundation

igalic commented Dec 16, 2023

View reviewed changes

igalic added a commit to igalic/cloud-init that referenced this pull request Dec 19, 2023

fix(sysvinit): make code a bit more consistent (canonical#4677)

c852e95

let's just use an `if`! Sponsored by: The FreeBSD Foundation

igalic mentioned this pull request Dec 21, 2023

Support for AIX #4713

Closed

holmanb requested changes Jan 3, 2024

View reviewed changes

igalic force-pushed the hack/ephemeral-run branch from e2c4cdb to c8098d0 Compare January 4, 2024 12:37

igalic added a commit to igalic/cloud-init that referenced this pull request Jan 4, 2024

fix(sysvinit): make code a bit more consistent (canonical#4677)

b9d7366

let's just use an `if`! Sponsored by: The FreeBSD Foundation

fix(sysvinit): make code a bit more consistent (canonical#4677)

d24dd5d

let's just use an `if`! Sponsored by: The FreeBSD Foundation

igalic force-pushed the hack/ephemeral-run branch 2 times, most recently from 2bbde27 to 13281ca Compare January 4, 2024 22:52

igalic added 2 commits January 4, 2024 23:16

fix(config): relocate /run to /var/run on BSD (canonical#4677)

8852126

on BSD, /run is not ephemeral. relocate BSDs config to /var/run Sponsored by: The FreeBSD Foundation Fixes canonicalGH-4180 Fixes canonicalGH-4231

igalic force-pushed the hack/ephemeral-run branch from 13281ca to 8852126 Compare January 4, 2024 23:18

igalic commented Jan 4, 2024

View reviewed changes

holmanb approved these changes Jan 5, 2024

View reviewed changes

holmanb merged commit 8937b5e into canonical:main Jan 5, 2024
29 checks passed

holmanb pushed a commit that referenced this pull request Jan 5, 2024

fix(sysvinit): make code a bit more consistent (#4677)

ccf5bc4

let's just use an `if`! Sponsored by: The FreeBSD Foundation

holmanb mentioned this pull request Jan 10, 2024

[bug] ephemeral directory module not used #4766

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(runpath): On *BSD, create /run is not ephemeral #4677

fix(runpath): On *BSD, create /run is not ephemeral #4677

igalic commented Dec 8, 2023 •

edited

igalic left a comment

igalic Dec 16, 2023

holmanb Jan 3, 2024 •

edited

igalic Jan 4, 2024

igalic Jan 4, 2024

holmanb Jan 4, 2024 •

edited

holmanb left a comment

holmanb Jan 3, 2024 •

edited

holmanb Jan 3, 2024

igalic left a comment

igalic Jan 4, 2024

igalic Jan 4, 2024

holmanb Jan 5, 2024

holmanb left a comment

holmanb Jan 5, 2024


		DI_LOG="${DI_LOG:-${PATH_RUN_CI}/ds-identify.log}"
		# Declare global here, we'll set it properly in main()

fix(runpath): On *BSD, create /run is not ephemeral #4677

fix(runpath): On *BSD, create /run is not ephemeral #4677

Conversation

igalic commented Dec 8, 2023 • edited

Proposed Commit Message

Additional Context

Test Steps

Checklist

Merge type

igalic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holmanb Jan 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holmanb Jan 4, 2024 • edited

Choose a reason for hiding this comment

holmanb left a comment

Choose a reason for hiding this comment

holmanb Jan 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igalic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holmanb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igalic commented Dec 8, 2023 •

edited

holmanb Jan 3, 2024 •

edited

holmanb Jan 4, 2024 •

edited

holmanb Jan 3, 2024 •

edited