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.

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

Fix ds-identify's read_fs_info() to work on FreeBSD #625

Merged
merged 2 commits into from
Nov 4, 2020

Conversation

igalic
Copy link
Collaborator

@igalic igalic commented Oct 23, 2020

previously, ds-identify would mostly just guess that the system in question is maybe OpenStack.

This patch makes it correctly recognize NoCloud.

Proposed Commit Message

Fix ds-identify's read_fs_info() to work on FreeBSD

FreeBSD doesn't have blkid, so we want to use geom to list devices and their fstypes and labels.

This PR also adds jail to the list of is_container()
And we now also properly cache geom and blkid output!

Finally, A test is added to verify the new behaviour by correctly identifying NoCloud on FreeBSD.

Additional Context

This fixes LP 1901174

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

tools/ds-identify Outdated Show resolved Hide resolved
tools/ds-identify Outdated Show resolved Hide resolved
Copy link
Collaborator

@smoser smoser left a comment

Choose a reason for hiding this comment

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

needs tests in test_dsidentify.

also... maybe we consider trying to render different versions ? freebsd and linux ?

rather than trying linux then trying freebsd.

probably that is not necessary if we're doing it based on uname.

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

read_fs_info() {
cached "${DI_BLKID_OUTPUT}" && return 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

bah. this needs fixing. we dont actually do anything to set that variable.
the intent of the variable is to say we've already run blkid and cached its output.

this isn't your fault. the intent of DI_BLKID_OUTPUT was to store the result of calling 'blkid', so that we didn't call that external program more than once. So I guess what I'd like from you:

a.) a comment in read_fs_info stating what variables it sets (DI_FS_LABELS, DI_ISO9660_DEVS, DI_FS_UUIDS)
b.) drop of the DI_BLKID_OUTPUT cache here
c.) add caching of call to 'geom'. the easiest way to do this might be just adding a variable 'DI_GEOM' and put the call to 'geom' in a function, something like:

geom_label_status_as() {
    # call the bsd program geom '-as', set DI_GEOM_LABEL_STATUS_OUT
    cached "$DI_GEOM_LABEL_STATUS_OUT" && return 0
    local out=""
    out=$(geom -as label status) &&  DI_GEOM_LABEL_STATUS="$out" ||
        DI_GEOM_LABEL_STATUS="$UNAVAILABLE"
    return
 }

d.) fixing of the caching of 'blkid' output (i realize i'm asking you to fix a bug that isn't your fault... go ahead and git blame this and see that it is mine. 😉 ), but having this variable/cache that doesn't do anything is just confusing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i realize i'm asking you to fix a bug that isn't your fault...

i'm perfectly happy to do that, it just means this patch will take as long as all my other patches usually do 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i think this is done now

tools/ds-identify Outdated Show resolved Hide resolved
read_geom() {
local oifs="$IFS" line="" delim=","
local ret=0 out="" labels="" dev="" label="" ftype="" isodevs=""
out=$(geom label status -as) || {
Copy link
Collaborator

Choose a reason for hiding this comment

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

after adding the 'geom_label_status_as' function, then you'll just use the variable, so here this would look like:

geom_label_status_as
local out="$DI_GEOM_LABEL_STATUS_OUT"
...
[ "$out" ] = "$UNAVAILABLE" && return 0

and the rest of your function here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you mean

[ "$out" = "$UNAVAILABLE" ] && return 0

right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes. good lint'ing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmmmm…
for now i'm passing on the return value instead of return 0… not sure that's a sensible thing to do

@igalic
Copy link
Collaborator Author

igalic commented Oct 26, 2020

@smoser any comments on the test?

@smoser
Copy link
Collaborator

smoser commented Oct 26, 2020

@smoser any comments on the test?

Sure, 2 comments:
a.) i dont think you actually added a test. (need to add a function name test_)
b.) other than that i thin your changes there look fine. the 'blkid_out' method that you mimic'd in "geom_out" was an attempt to be less verbose in tests. it feels like you did the right thing here in translating the data to geom output.

@@ -807,6 +830,19 @@ def _print_run_output(rc, out, err, cfg, files):
'dev/vdb': 'pretend iso content for cidata\n',
}
},
'NoCloud-fbsd': {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@smoser i naïvely assumed this would add a test case

Copy link
Collaborator

Choose a reason for hiding this comment

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

still no test, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

gimme a day or two, while i work my way thru https://pythontesting.net/start-here/

Copy link
Collaborator Author

@igalic igalic left a comment

Choose a reason for hiding this comment

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

ℹ️

tools/ds-identify Show resolved Hide resolved
tools/ds-identify Show resolved Hide resolved
@igalic igalic force-pushed the fix-ds-identify-fbsd branch 2 times, most recently from 6ae4ef4 to 1630980 Compare October 27, 2020 20:56
tools/ds-identify Outdated Show resolved Hide resolved
@igalic igalic force-pushed the fix-ds-identify-fbsd branch 2 times, most recently from 11891e6 to a53e001 Compare October 30, 2020 20:58
Copy link
Collaborator

@smoser smoser left a comment

Choose a reason for hiding this comment

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

I like it. good work.

FreeBSD doesn't have blkid, so we want to use geom to list devices and
their fstypes and labels.

This PR also adds `jail` to the list of is_container()
And we now also properly cache geom and blkid output!

A test is added to verify the new behaviour by correctly identifying
NoCloud on FreeBSD.
@smoser smoser merged commit cf6c36a into canonical:master Nov 4, 2020
@igalic igalic deleted the fix-ds-identify-fbsd branch November 4, 2020 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants