refactor(IscDhclient): discover DHCP leases at distro-provided location #4683

phsm · 2023-12-12T07:45:03Z

Proposed Commit Message

refactor(IscDhclient): discover DHCP leases at distro-provided location

Use leases directory and lease file provided by Distro object
instead of cycling through all possible locations.

Additional Context

IscDhclient class seems to be only used by CloudStack datasource.

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

igalic

This looks good to me!

This part of the commit message,

This change adds allows discovering such lease files.

seems like it should be either "adds" or "allows", not both.

phsm · 2023-12-12T17:36:59Z

This looks good to me!

This part of the commit message,

This change adds allows discovering such lease files.

seems like it should be either "adds" or "allows", not both.

Thanks, fixed the typo in the commit message.

holmanb

Thanks for this @phsm!

Nice addition and tests, I'm leaning towards making that a (free)bsd check prior to adding to the list.

Justification below.

holmanb · 2023-12-12T19:28:18Z

cloudinit/net/dhcp.py

@@ -473,6 +473,7 @@ def get_dhclient_d():
            "/var/lib/dhclient",
            "/var/lib/dhcp",
            "/var/lib/NetworkManager",
+            "/var/db",


I'm on the fence about whether to make this a (free)bsd check before adding. On one hand, I check a couple of linuxen, and some have the directory, others don't. Why check this directory on those distros? On the other hand, there were few files in that directory and only a single listdir called, so it shouldn't be "slow".

This function is one that I've previously considered breaking out into distros/ anyways, since I think /var/lib/dhcp/ and /var/lib/dhclient/ are distro-specific iirc.

Thoughts?

I briefly checked the distros/ directory: seems its purpose is to isolate distro-specific code in there. So this means that the cloud-init has a reliable way to detect which os/distro it is running on?

If yes, then its indeed better to implement this function per-distro.
Perhaps implement a method in distros.Distro that uses a class attribute with the dhclient directory such as this and override that class attribute per child distro class.

So this means that the cloud-init has a reliable way to detect which os/distro it is running on?

Yes :) cloud-init has lots of distro-specific code. Some of it gets littered throughout shared code (which is acceptable prior to distro detection), but as much as possible keeping distro-specific code in distros/ and calling into it makes the code more modular and keeps non-distro code much cleaner.

Perhaps implement a method in distros.Distro that uses a class attribute with the dhclient directory such as this and override that class attribute per child distro class.

@phsm yes, this sounds like the right direction. I think we have a reference to the distro instance higher up that we'll just need to pass down the stack as an argument to access it in this function. Looks like in select_dhcp_client() we could pass the distro object as an argument to the client instance, assign that as an instance variable in __init__(), and then reference that later in get_dhclient_d(), which would have to change from a static method to just a normal instance method.

We can make the current directories be in the base implementation in distros/__init__.py, then the BSDs can set their own.

Therefore we can do less file checking for all distros - a win all around.

I just made another commit with an example how I see it done:

main Distro class has two attributes: dhclient_lease_directory, dhclient_lease_file_regex defining the lease directory for lease files in a distro, and a regex helping to find those lease files in there.

every class of a distro that uses dhclient has those 2 attributes defined.

Cloudstack's get_vr_address() now detects a distro and supplies dhcp.IscDhclient.get_latest_lease() with the lease_dir an a regex.

there is no need for get_dhclient_d() anymore

Although get_latest_lease() function signature is changed, I don't see it being used anywhere except Cloudstack DS so it shouldn't break anything.

The purpose of this commit is to gather your feedback on whether I'm working in the right direction.
Also, do I detect the distro correctly here?

I just updated the PR with the following:

Updated the distros files with dhclient lease director/regex for distros that support DHClient. I installed all of them on a virtual machine to verify the paths:

CentOS: updated centos.py to contain dhclient parameters for Centos 7. I assume no one uses CentOS > 7 on servers because they're EOL.

RHEL: updated rhel.py with dhclient parameters. To use dhclient in RHEL, dhcp=dhclient parameter must be set in NetworkManager config.

AlmaLinux: identical to RHEL.

Rocky: identical to RHEL.

Debian: updated debian.py with dhclient parameters.

Alpine: doesn't use dhclient, uses udhcpc.

Amazon Linux: updated amazon.py with dhclient parameters.

CloudLinux: identical to RHEL.

EuroLinux: identical to RHEL.

Fedora: identical to RHEL

FreeBSD: updated freebsd.py with dhclient parameters.

Gentoo: doesn't use dhcpcd, uses dhcpcd.

Mariner: uses systemd-networkd.

Miracle: identical to RHEL

Open Cloud OS: I couldn't even download it an ISO image to test. I assume its identical to RHEL.

OpenEuler: identical to RHEL

OpenSUSE: doesn't use dhclient, uses wicked.

SLES: I don't have a copy of SLES, I assume it also uses wicked.

Virtuozzo: I don't have a copy of it, I assume its identical to RHEL.

Tencentos: didn't test, assume its identical to RHEL.

Tests:

Added tests for finding a lease in different distros that can use dhclient.

Removed TestGetLatestLease testcase from test_cloudstack.py in its entirety: Cloudstack tests shouldn't test obtaining DHCP lease, its already covered in test_dhcp.py

Thanks @phsm for the update. I think that there is a missing detail here.

doesn't use dhclient

Use of wicked / networkd / networkmanager / etc doesn't necessarily mean that dhclient isn't used. In fact, cloud-init itself uses dhclient during the local stage. Cloud-init sets up an ephemeral network so that it can reach out to the IMDS to get configuration. This allows cloud-init to write the networking configuration locally before the distro's built-in network manager is even started. That said, since CloudStack was never checking cloud-init's dedicated lease path, I think we can go ahead with what you have here.

We may want to also check the ephemeral lease if no data is found by the other checks, but I'll take care of that in a followup PR that I'm working on for dhcpcd.

cloudinit/sources/DataSourceCloudStack.py

igalic

❓

cloudinit/distros/rhel.py

phsm · 2024-01-05T09:54:30Z

Sorry for commit storm, I was struggling with the linters and the tests.

I also changed the PR description as its not just fixing Dhclient at FreeBSD anymore.

igalic

this looks good, here's some requests for changes

cloudinit/sources/DataSourceCloudStack.py

igalic

I like this.
thank you for this monumental effort

holmanb

@phsm nice work here - thanks for putting in this effort.

I left a couple of comments that I think we need to address.

cloudinit/distros/centos.py

holmanb · 2024-01-05T18:49:29Z

cloudinit/distros/rhel.py

@@ -29,6 +29,12 @@ class Distro(distros.Distro):
    network_script_tpl = "/etc/sysconfig/network-scripts/ifcfg-%s"
    tz_local_fn = "/etc/localtime"
    usr_lib_exec = "/usr/libexec"
+    # RHEL and derivatives use NetworkManager DHCP client by default.


True, but dhclient is still ran manually by cloud-init. I'd say that this is a reasonable directory to check, but if no leases exist there then we should fall back to the default dhclient directory (/var/lib/dhclient, I think).

Since NetworkManager starts after cloud-init runs dhclient, the more recent lease would be any NetworkManager lease

Implemented:

if the distro-provided dir and regex yield nothing, then try to use the fallback dir and regex. Regex is specifically made to be quite loose (search for any file ending with .lease or .leases).

Tests for that behavior

@phsm Just looking at this change now, I realize that you made /var/lib/dhclient/ default for all distros. Sorry I missed this before. With the change as it stands, /var/lib/dhclient will be the only directory checked for distros that do not inherit from FreeBSD / RHEL / Debian, so I think that we need to account for the following:

From what I can tell, the dhclient subdirectory is a feature of RHEL derivatives and Arch.

The following distros use /var/lib/dhcp/: including Alpine, Gentoo (and derivatives I assume), Suse (and derivatives), Debian, etc.

No clue what Photon or Mariner use.

@igalic is /var/lib/dhclient is a reasonable directory for the non-FreeBSD BSDs?

I'll need to check.

OpenBSD puts it in the same place as FreeBSD:
https://man.openbsd.org/dhclient.8#FILES

NetBSD had switched to dhcpcd since 9.0. before that, they put it in the same place, https://man.netbsd.org/NetBSD-8.2/dhclient.8#FILES but with a different name

cloudinit/sources/DataSourceCloudStack.py

holmanb

Nice work - getting close! A couple more changes requested, I'm testing this on Ec2 now.

cloudinit/net/dhcp.py

holmanb

Great work @phsm, thanks for putting this together.

Looks good to me, so I'll merge.

If I understand correctly, cloudstack might also benefit from parsing the default lease that cloud-init gets during the ephemeral stage. I'll probably include that as part of the work I'm doing with dhcpcd.

PR canonical#4683 changes missed Alpine. This PR addresses this.

PR #4683 changes missed Alpine. This PR addresses this.

holmanb assigned holmanb and igalic Dec 12, 2023

igalic approved these changes Dec 12, 2023

View reviewed changes

holmanb reviewed Dec 12, 2023

View reviewed changes

holmanb reviewed Dec 27, 2023

View reviewed changes

cloudinit/sources/DataSourceCloudStack.py Outdated Show resolved Hide resolved

igalic reviewed Dec 28, 2023

View reviewed changes

cloudinit/distros/rhel.py Outdated Show resolved Hide resolved

phsm changed the title ~~fix: IscDhclient discover DHCP leases on FreeBSD~~ refactor(IscDhclient): discover DHCP leases at distro-provided location Jan 5, 2024

igalic suggested changes Jan 5, 2024

View reviewed changes

cloudinit/sources/DataSourceCloudStack.py Outdated Show resolved Hide resolved

cloudinit/sources/DataSourceCloudStack.py Outdated Show resolved Hide resolved

cloudinit/sources/DataSourceCloudStack.py Outdated Show resolved Hide resolved

igalic approved these changes Jan 5, 2024

View reviewed changes

holmanb requested changes Jan 5, 2024

View reviewed changes

holmanb requested changes Jan 8, 2024

View reviewed changes

cloudinit/net/dhcp.py Outdated Show resolved Hide resolved

Refactor IscDhclient.get_latest_lease() to use distro-specific data

650d224

holmanb approved these changes Jan 11, 2024

View reviewed changes

holmanb merged commit 1baa9ff into canonical:main Jan 11, 2024
29 checks passed

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

Alpine: fix location of dhclient leases file

404a458

PR canonical#4683 changes missed Alpine. This PR addresses this.

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

Alpine: fix location of dhclient leases file (#4782)

5b6d08d

PR #4683 changes missed Alpine. This PR addresses this.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(IscDhclient): discover DHCP leases at distro-provided location #4683

refactor(IscDhclient): discover DHCP leases at distro-provided location #4683

phsm commented Dec 12, 2023 •

edited

Loading

igalic left a comment

phsm commented Dec 12, 2023

holmanb left a comment •

edited

Loading

holmanb Dec 12, 2023

phsm Dec 13, 2023 •

edited

Loading

holmanb Dec 14, 2023 •

edited

Loading

phsm Dec 21, 2023 •

edited

Loading

phsm Jan 5, 2024

holmanb Jan 10, 2024 •

edited

Loading

igalic left a comment

phsm commented Jan 5, 2024

igalic left a comment

igalic left a comment

holmanb left a comment

holmanb Jan 5, 2024 •

edited

Loading

phsm Jan 8, 2024 •

edited

Loading

holmanb Jan 9, 2024 •

edited

Loading

igalic Jan 9, 2024

igalic Jan 9, 2024

holmanb left a comment

holmanb left a comment

refactor(IscDhclient): discover DHCP leases at distro-provided location #4683

refactor(IscDhclient): discover DHCP leases at distro-provided location #4683

Conversation

phsm commented Dec 12, 2023 • edited Loading

Proposed Commit Message

Additional Context

Test Steps

Checklist

Merge type

igalic left a comment

Choose a reason for hiding this comment

phsm commented Dec 12, 2023

holmanb left a comment • edited Loading

Choose a reason for hiding this comment

holmanb Dec 12, 2023

Choose a reason for hiding this comment

phsm Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

holmanb Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

phsm Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

phsm Jan 5, 2024

Choose a reason for hiding this comment

holmanb Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

igalic left a comment

Choose a reason for hiding this comment

phsm commented Jan 5, 2024

igalic left a comment

Choose a reason for hiding this comment

igalic left a comment

Choose a reason for hiding this comment

holmanb left a comment

Choose a reason for hiding this comment

holmanb Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

phsm Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

holmanb Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

igalic Jan 9, 2024

Choose a reason for hiding this comment

igalic Jan 9, 2024

Choose a reason for hiding this comment

holmanb left a comment

Choose a reason for hiding this comment

holmanb left a comment

Choose a reason for hiding this comment

phsm commented Dec 12, 2023 •

edited

Loading

holmanb left a comment •

edited

Loading

phsm Dec 13, 2023 •

edited

Loading

holmanb Dec 14, 2023 •

edited

Loading

phsm Dec 21, 2023 •

edited

Loading

holmanb Jan 10, 2024 •

edited

Loading

holmanb Jan 5, 2024 •

edited

Loading

phsm Jan 8, 2024 •

edited

Loading

holmanb Jan 9, 2024 •

edited

Loading