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

Add Netbsd support #62

Merged
merged 12 commits into from Mar 12, 2020
Merged

Add Netbsd support #62

merged 12 commits into from Mar 12, 2020

Conversation

goneri
Copy link
Contributor

@goneri goneri commented Nov 23, 2019

Add support for the NetBSD Operating System.

Features in this branch:

  • Add BSD distro parent class from which NetBSD and FreeBSD can specialize
  • Add *bsd util functions to cloudinit.net and cloudinit.net.bsd_utils
  • subclass cloudinit.distro.freebsd.Distro from bsd.Distro
  • Add new cloudinit.distro.netbsd and cloudinit.net.renderer for netbsd
  • Add lru_cached util.is_NetBSD functions
  • Add NetBSD detection for ConfigDrive and NoCloud datasources

This branch has been tested with:

  • NoCloud and OpenStack (with and without config-drive)
  • NetBSD 8.1. and 9.0
  • FreeBSD 11.2 and 12.1
  • Python 3.7 only, because of the dependency oncrypt.METHOD_BLOWFISH.
    This version is available in NetBSD 7, 8 and 9 anyway.

@blackboxsw blackboxsw added the wip label Nov 26, 2019
@goneri goneri force-pushed the netbsd branch 2 times, most recently from 142770f to 0397ebc Compare Dec 19, 2019
Copy link
Collaborator

@igalic igalic left a comment

i literally only looked at half a commit here

cloudinit/net/__init__.py Outdated Show resolved Hide resolved
@goneri goneri force-pushed the netbsd branch 4 times, most recently from 021c311 to 12bd49f Compare Dec 19, 2019
Copy link
Collaborator

@igalic igalic left a comment

👍

@goneri goneri force-pushed the netbsd branch 4 times, most recently from f7a3ac6 to a2e1d2f Compare Dec 19, 2019
@OddBloke OddBloke added CLA not signed CLA signed and removed CLA not signed labels Dec 19, 2019
@goneri goneri force-pushed the netbsd branch 2 times, most recently from dd2312f to 36d59dd Compare Dec 22, 2019
@goneri goneri changed the title [WIP] add Netbsd support Add Netbsd support Dec 23, 2019
@goneri goneri force-pushed the netbsd branch 3 times, most recently from 9489b64 to 2c001cd Compare Dec 23, 2019
Copy link
Collaborator

@igalic igalic left a comment

first pass 👀

cloudinit/distros/netbsd.py Outdated Show resolved Hide resolved
cloudinit/distros/bsd_util.py Outdated Show resolved Hide resolved
cloudinit/distros/netbsd.py Outdated Show resolved Hide resolved
return bsd_util.get_rc_config_value('hostname')

def _write_hostname(self, hostname, filename):
bsd_util.set_rc_config_value('hostname', hostname,
Copy link
Collaborator

@igalic igalic Dec 23, 2019

Choose a reason for hiding this comment

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

in freebsd, we use rhel_util, how is this different?

Copy link
Contributor Author

@goneri goneri Dec 24, 2019

Choose a reason for hiding this comment

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

on NetBSD, the /etc/rc.conf comes with a if block. As a result, it's not a simple list of key/value anymore. The rhel_util parser cannot parse the file properly. And so I decided to write these two light setter/getter functions. The code is rather naive, but I think it's enough for now.

We can potentially find the same situation on FreeBSD, so I updated the FreeBSD files to use them too.

Copy link
Collaborator

@blackboxsw blackboxsw Jan 17, 2020

Choose a reason for hiding this comment

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

Shallow person question: Is there enough alignment between netbsd and freebsd that one could a be a subclass of the other distro? (like ubuntu.py is a subclass of debian distro as it is a derivative work that can share the same distro class methods). As you've implemented this netbsd is a subclass only of top-level Distro class and thus has to redefine all methods.

cloudinit/distros/netbsd.py Outdated Show resolved Hide resolved
cloudinit/distros/netbsd.py Outdated Show resolved Hide resolved
util.subp(cmd, env=e, capture=False)

def apply_locale(self, locale, out_fn=None):
pass
Copy link
Collaborator

@igalic igalic Dec 23, 2019

Choose a reason for hiding this comment

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

eehhh?

Copy link
Contributor Author

@goneri goneri Dec 24, 2019

Choose a reason for hiding this comment

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

I don't really plan to implement that. I don't think it makes any sense in 2019 to change the locale.

cloudinit/net/__init__.py Outdated Show resolved Hide resolved
config/cloud.cfg.tmpl Show resolved Hide resolved
tools/build-on-netbsd Show resolved Hide resolved
@goneri goneri force-pushed the netbsd branch 2 times, most recently from 5cc3629 to 8069556 Compare Dec 24, 2019
Copy link
Collaborator

@igalic igalic left a comment

🙋

cloudinit/distros/netbsd.py Outdated Show resolved Hide resolved
@goneri goneri force-pushed the netbsd branch 3 times, most recently from 517531a to 23ca438 Compare Dec 29, 2019
goneri added a commit to goneri/cloud-init that referenced this issue Mar 4, 2020
Add a comment in the module to explain the difference with the other
rc file parse: cloudinit.distros.parsers.sys_conf

On NetBSD, /etc/rc.conf comes with a if block:
  if [ -r /etc/defaults/rc.conf ]; then
as a consequence, the file is not a regular key/value list
anymore and we cannot use cloudinit.distros.parsers.sys_conf
The module comes with a more naive parser, but is able to
preserve these if blocks.

See: canonical#62 (comment)
goneri added a commit to goneri/cloud-init that referenced this issue Mar 4, 2020
goneri added a commit to goneri/cloud-init that referenced this issue Mar 6, 2020
Add a comment in the module to explain the difference with the other
rc file parse: cloudinit.distros.parsers.sys_conf

On NetBSD, /etc/rc.conf comes with a if block:
  if [ -r /etc/defaults/rc.conf ]; then
as a consequence, the file is not a regular key/value list
anymore and we cannot use cloudinit.distros.parsers.sys_conf
The module comes with a more naive parser, but is able to
preserve these if blocks.

See: canonical#62 (comment)
goneri added a commit to goneri/cloud-init that referenced this issue Mar 6, 2020
@goneri goneri requested a review from blackboxsw Mar 9, 2020
goneri added a commit to goneri/cloud-init that referenced this issue Mar 11, 2020
Add a comment in the module to explain the difference with the other
rc file parse: cloudinit.distros.parsers.sys_conf

On NetBSD, /etc/rc.conf comes with a if block:
  if [ -r /etc/defaults/rc.conf ]; then
as a consequence, the file is not a regular key/value list
anymore and we cannot use cloudinit.distros.parsers.sys_conf
The module comes with a more naive parser, but is able to
preserve these if blocks.

See: canonical#62 (comment)
goneri added a commit to goneri/cloud-init that referenced this issue Mar 11, 2020
goneri and others added 12 commits Mar 12, 2020
Add support for the NetBSD Operating System. This branch has been tested
with:

- NoCloud and OpenStack (with and without config-drive)
- and NetBSD 7.1, 8.0 and 8.1.
- Python 3.7 only, because of the dependency oncrypt.METHOD_BLOWFISH.
  This version is available in NetBSD 7, 8 and 9 anyway.
Add a comment in the module to explain the difference with the other
rc file parse: cloudinit.distros.parsers.sys_conf

On NetBSD, /etc/rc.conf comes with a if block:
  if [ -r /etc/defaults/rc.conf ]; then
as a consequence, the file is not a regular key/value list
anymore and we cannot use cloudinit.distros.parsers.sys_conf
The module comes with a more naive parser, but is able to
preserve these if blocks.

See: canonical#62 (comment)
Ensure `members` is an optional argument, like in the master class.
This was just a duplicated copy of the original `create_user()`.
In the original `create_group()`, `members` is an optional argument.
The original `create_user()` method should work just fine on FreeBSD.
The BSD distros will now inherite from this base BSD class.
There is a lot of duplication of code that could be shared between
the FreeBSD and NetBSD classes. Let's avoid as much boilerplate
cut-n-paste as possible.

Move the following methods up into the parent BSD class:
 * create_group which now uses group_add_cmd_prefix and
   _get_add_member_to_group_cmd to specialize subclass commands
 * install_packages can be generalized since NetBSD.update_package_sources
   is nothing
 * package_command is generalized with minor command diffs sourced from
   class attributes pkg_cmd_install_prefix and pkg_cmd_remove_prefix and
   custom environment given by _get_pkg_cmd_environ

Also in this commit:
 * A bug fix in existing Freebsd which was switching group name and
   and username parameters
 * I left a TODO message for your review as I don't think either BSD class
 handles a distro.package_command('upgrade') call and it might be needed
 in a followup PR
Copy link
Collaborator

@blackboxsw blackboxsw left a comment

Looks really good.
I've rebased against current master and tried consolidating a couple of more common methods up from Freebsd and Netbsd into the parent BSD class.

Please see what you think. The git show 7ee08d3 should show the set of diffs I was thinking of:

Move the following methods up into the parent BSD class:
 * create_group which now uses group_add_cmd_prefix and
   _get_add_member_to_group_cmd to specialize subclass commands
 * install_packages can be generalized since NetBSD.update_package_sources
   is nothing
 * package_command is generalized with minor command diffs sourced from
   class attributes pkg_cmd_install_prefix and pkg_cmd_remove_prefix and
   custom environment given by _get_pkg_cmd_environ

Also in this commit:
 * A bug fix in existing Freebsd which was switching group name and
   and username parameters
 * I left a TODO message for your review as I don't think either BSD class
 handles a distro.package_command('upgrade') call and it might be needed
 in a followup PR

@blackboxsw
Copy link
Collaborator

@blackboxsw blackboxsw commented Mar 12, 2020

@goneri I'm ok with this if you are ok with the patch I added. the one question is when you'd like to handle package_command('upgrade')

@goneri
Copy link
Contributor Author

@goneri goneri commented Mar 12, 2020

Looks good to me:

Copy link
Collaborator

@blackboxsw blackboxsw left a comment

Schweet!

@blackboxsw blackboxsw merged commit 94838de into canonical:master Mar 12, 2020
2 checks passed
goneri added a commit to goneri/cloud-init that referenced this issue Mar 24, 2020
Add a comment in the module to explain the difference with the other
rc file parse: cloudinit.distros.parsers.sys_conf

On NetBSD, /etc/rc.conf comes with a if block:
  if [ -r /etc/defaults/rc.conf ]; then
as a consequence, the file is not a regular key/value list
anymore and we cannot use cloudinit.distros.parsers.sys_conf
The module comes with a more naive parser, but is able to
preserve these if blocks.

See: canonical#62 (comment)
goneri added a commit to goneri/cloud-init that referenced this issue Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA signed
Projects
None yet
4 participants