-
Notifications
You must be signed in to change notification settings - Fork 909
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
RbxCloud: Add support for FreeBSD #464
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
for dev, bdata in util.blkid().items() | ||
if bdata.get('LABEL', '').upper() == 'CLOUDMD' | ||
] | ||
devices = util.find_devs_with('LABEL=CLOUDMD') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a small change in behaviour on Linux-based systems: the previous code would match 'CLOUDMD'
case-insensitively, whereas the new code requires upper-case:
$ ls /dev/disk/by-label -lah
...
lrwxrwxrwx 1 root root 15 Jun 29 08:38 SYSTEM -> ../../nvme0n1p1
...
$ python3 -c 'from cloudinit import util; print(util.find_devs_with("LABEL=system"))'
[]
$ python3 -c 'from cloudinit import util; print(util.find_devs_with("LABEL=SYSTEM"))'
['/dev/nvme0n1p1']
Is this an issue, practically speaking?
(If this change is acceptable, we should also update ds-identify
to match the more stringent case requirements.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In our environment, we've been providing clients with images only marked as CLOUDMD for years. The documentation in this area was also consistent always. I have not encountered (but we have no right to scan customer environments) a different situation.
I wonder at what stage the case insensitivity should be implemented. I notice that:
- ConfigDrive supports both: https://github.com/canonical/cloud-init/blob/master/cloudinit/sources/DataSourceConfigDrive.py#L30
- NoCloud supports both: https://github.com/canonical/cloud-init/blob/master/cloudinit/sources/DataSourceNoCloud.py#L42-L43
At the same time:
- OpenNebula supports one format: https://github.com/canonical/cloud-init/blob/master/cloudinit/sources/DataSourceOpenNebula.py#L268
- OpenStack supports one format: https://github.com/canonical/cloud-init/blob/master/cloudinit/sources/helpers/openstack.py#L108
Considering diversity, it will adapt code of datasource to support both formats nor utils
itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Changes are made that simplify code and aim to properly support FreeBSD:
util.find_devs_with
instead call directlyblkid
, because on FreeBSD is not supported well andutil.find_devs_with
have solution for FreeBSD for that/bin/bash