replace usage of dmidecode with kenv on FreeBSD #621

igalic · 2020-10-22T11:22:57Z

Proposed Commit Message

FreeBSD lets us read out kernel parameters with kenv(1), a user-space utility that's shipped in "base"
We can use it in place of dmidecode(8), thus removing the dependency on sysutils/dmidecode, and the restrictions to i386 and x86_64 architectures that this utility imposes on FreeBSD.

Additional Context

I added this code, and while it opened the doors for more BSD stuff to be added, I'd like to remedy this restriction.
I'd also like to remind that @goneri already pointed this path out a year ago: #42 (comment)

Test Steps

run kenv | grep smbios on your favourite FreeBSD machine, and notice that unlike dmidecode you can just run it without being root!

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

goneri · 2020-10-22T12:20:21Z

You can also drop dmidecode from the dependency list in tools/build-on-freebsd.

igalic · 2020-10-22T14:03:59Z

this is far from done…

cloudinit/util.py

smoser

Fix the hard coded path and the exception. Humor me and think about the DMIDECODE map rework.

cloudinit/dmi.py

smoser

looks good. some comments inline.

one thought... I think we could use functools.lru_cache on is_container. In real life, the result is never going to change... we don't go from being inside a container to not inside a container in the same python process (at least cloud-init doesn't). And that will save us several calls to subprocess.

I did a quick test, and adding lru_cache didn't seem to break any unit tests. Probably because we don't actually have unit tests for the is_container function.

smoser · 2020-11-05T21:30:09Z

cloudinit/util.py

+        _is_container_systemd,
+        _is_container_upstart,
+        _is_container_old_lxc,
+        _is_container_freebsd)


i just threw this together, with no test at all. i'm surprised that it apparently passes tests.

I think it is probably sane to put _is_container_freebsd before upstart or old_lxc... as i think its probably more likely for this new thing in cloud-init to run on freebsd than a 2014 era linux.

smoser · 2020-11-05T21:31:43Z

cloudinit/util.py

+        return False
+    try:
+        subp.subp(cmd)
+        return True


i dont know on convention, but it seems maybe move 'return True' to outside the try. as it is probably more than a little bit unlikely for 'return True' to raise a subprocesserror. (and yes, i'm fully aware that I wrote that).

cloudinit/dmi.py

smoser · 2020-11-06T13:29:26Z

cloudinit/dmi.py

@@ -111,6 +140,9 @@ def read_dmi_data(key):
    if syspath_value is not None:
        return syspath_value

+    if is_FreeBSD():
+        return _read_kenv(key)
+


seems like you might as well bump this up above the '_read_dmi_syspath' call.
theres no point in looking in /sys on freebsd, right? So that would always be a wasted stat (inside _read_dmi_syspath) on freebsd. And on linux we'd just pay the '@lru_cache' call is_FreeBSD.

I'm not sure if that is clear...
short summary: just move this up above the call to _read_dmi_syspath.

smoser · 2020-11-06T13:34:11Z

tools/ds-identify

+        warn "No kenv program. Cannot read $sys_field."
+        return 1
+    }
+    case "$1" in


you shoudl use $sys_field rather than $1.

smoser · 2020-11-06T13:35:24Z

tools/ds-identify

+           return 1;;
+    esac
+    val=$(kenv -q $kenv_field 2>/dev/null) || return 1
+    _RET="$val"


i think this looks good,. thanks.
I suggest shortening kenv_field and sys_field to ename and sname.

smoser · 2020-11-06T13:36:59Z

tools/ds-identify

+        *) error "Unknown field $sys_field. Cannot call kenv."
+           return 1;;
+    esac
+    val=$(kenv -q $kenv_field 2>/dev/null) || return 1


you should quote $kenv_field here just to be pedantic.

igalic · 2020-11-06T17:48:28Z

looks good. some comments inline.

one thought... I think we could use functools.lru_cache on is_container. In real life, the result is never going to change... we don't go from being inside a container to not inside a container in the same python process (at least cloud-init doesn't). And that will save us several calls to subprocess.

I did a quick test, and adding lru_cache didn't seem to break any unit tests. Probably because we don't actually have unit tests for the is_container function.

yeah… i noticed that, but didn't wanna go there yet…

my brave suggestion is to add @lru_cache on top of def read_dmi_data(key).
I don't know how often we duplicate calls throughout the code-base, but yeah… it "might" help

smoser · 2020-11-06T17:57:06Z

tools/ds-identify

 dmi_decode() {
    local sys_field="$1" dmi_field="" val=""
    command -v dmidecode >/dev/null 2>&1 || {
        warn "No dmidecode program. Cannot read $sys_field."
        return 1
    }
-    case "$1" in
+    case "$sys_field" in


thank you for fixing that.

FreeBSD lets us read out kernel parameters with kenv(1), a user-space utility that's shipped in "base" We can use it in place of dmidecode(8), thus removing the dependency on sysutils/dmidecode, and the restrictions to i386 and x86_64 architectures that this utility imposes on FreeBSD. Co-authored-by: Scott Moser <smoser@brickies.net>

igalic changed the title ~~add translation table for dmidecode to kenv~~ replace usage of dmidecode with kenv on FreeBSD Oct 22, 2020

smoser reviewed Oct 22, 2020

View reviewed changes

cloudinit/util.py Outdated Show resolved Hide resolved

igalic force-pushed the fix-regression/dmidecode-freebsd branch 3 times, most recently from 1dd9a74 to 40e58d0 Compare October 26, 2020 18:06

igalic force-pushed the fix-regression/dmidecode-freebsd branch 2 times, most recently from 5f552de to 66f7751 Compare November 3, 2020 10:00

TheRealFalcon self-assigned this Nov 3, 2020

igalic force-pushed the fix-regression/dmidecode-freebsd branch from faee463 to 963690e Compare November 5, 2020 11:03

smoser requested changes Nov 5, 2020

View reviewed changes

cloudinit/dmi.py Outdated Show resolved Hide resolved

cloudinit/dmi.py Outdated Show resolved Hide resolved

cloudinit/dmi.py Show resolved Hide resolved

igalic force-pushed the fix-regression/dmidecode-freebsd branch 2 times, most recently from e8dddce to 37ca15c Compare November 6, 2020 10:03

igalic requested a review from smoser November 6, 2020 10:07

smoser requested changes Nov 6, 2020

View reviewed changes

smoser reviewed Nov 6, 2020

View reviewed changes

igalic force-pushed the fix-regression/dmidecode-freebsd branch 2 times, most recently from cc5fb80 to 432e709 Compare November 6, 2020 18:10

smoser approved these changes Nov 6, 2020

View reviewed changes

igalic force-pushed the fix-regression/dmidecode-freebsd branch from 432e709 to c557eb0 Compare November 6, 2020 18:21

smoser merged commit d83c0bb into canonical:master Nov 6, 2020

igalic deleted the fix-regression/dmidecode-freebsd branch November 6, 2020 19:45

This was referenced May 12, 2023

cloud-init regenerating ssh-keys #3753

Closed

fix regression: remove dependency on dmidecode #3795

Closed

Release 20.4 #3811

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace usage of dmidecode with kenv on FreeBSD #621

replace usage of dmidecode with kenv on FreeBSD #621

igalic commented Oct 22, 2020 •

edited

Loading

goneri commented Oct 22, 2020

igalic commented Oct 22, 2020

smoser left a comment

smoser left a comment

smoser Nov 5, 2020

smoser Nov 5, 2020

smoser Nov 6, 2020

smoser Nov 6, 2020

smoser Nov 6, 2020

smoser Nov 6, 2020

igalic commented Nov 6, 2020

smoser Nov 6, 2020

replace usage of dmidecode with kenv on FreeBSD #621

replace usage of dmidecode with kenv on FreeBSD #621

Conversation

igalic commented Oct 22, 2020 • edited Loading

Proposed Commit Message

Additional Context

Test Steps

Checklist:

goneri commented Oct 22, 2020

igalic commented Oct 22, 2020

smoser left a comment

Choose a reason for hiding this comment

smoser left a comment

Choose a reason for hiding this comment

smoser Nov 5, 2020

Choose a reason for hiding this comment

smoser Nov 5, 2020

Choose a reason for hiding this comment

smoser Nov 6, 2020

Choose a reason for hiding this comment

smoser Nov 6, 2020

Choose a reason for hiding this comment

smoser Nov 6, 2020

Choose a reason for hiding this comment

smoser Nov 6, 2020

Choose a reason for hiding this comment

igalic commented Nov 6, 2020

smoser Nov 6, 2020

Choose a reason for hiding this comment

igalic commented Oct 22, 2020 •

edited

Loading