[1/2] DHCP: Refactor dhcp client code by holmanb · Pull Request #2122 · canonical/cloud-init

holmanb · 2023-04-13T18:50:24Z

Move isc-dhclient code to dhcp.py

In support of the upcoming deprecation of
isc-dhcp-client, this code refactors current
dhcp code into classes in dhcp.py. The
primary user-visible change should be the
addition of the following log:

dhcp.py[DEBUG]: DHCP client selected: dhclient

This code lays groundwork to enable
alternate implementations to live side by
side in the codebase to be selected with
distro-defined priority fallback. Note that
maybe_perform_dhcp_discovery() now selects
which dhcp client to call, and then runs the
corresponding client's dhcp_discovery()
method. Currently only class IscDhclient is
implemented, however a yet-to-be-implemented
class Dhcpcd exists to test fallback behavior
and this will be implemented in part two of
this series.

Part of this refactor includes shifting
dhclient service management from hardcoded
calls to the distro-defined manage_service()
method in the *BSDs. Future work is required
in this area to support multiple clients via
select_dhcp_client().

Additional Context

Reviewers may find this pull request easiest to review commit by commit rather than all at once, since this change spans several files. Each commit is logically grouped - code changes and corresponding test updates share commits.

Test Steps

Boot cloud-init on a datasource that uses ephemeral network setup.

tox -e integration-test -- tests/integration_tests/test_logging.py

New Log:

2023-04-13 18:00:28,773 - util.py[DEBUG]: Read 5 bytes from /sys/class/net/ens5/operstate
2023-04-13 18:00:28,773 - dhcp.py[DEBUG]: DHCP client selected: dhclient                                                                                                                  
2023-04-13 18:00:28,773 - dhcp.py[DEBUG]: Performing a dhcp discovery on ens5

Full log

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

…ect to dhcp

…cp code

igalic

first look

igalic · 2023-04-13T19:14:03Z

+        # Generally dhclient relies on dhclient-script PREINIT action to bring
+        # the link up before attempting discovery. Since we are using
+        # -sf /bin/true, we need to do that "link up" ourselves first.
+        subp.subp(["ip", "link", "set", "dev", interface, "up"], capture=True)


this is platform specific, and should use the distro class

Good point. This is how it works currently on main, but you're right, this isn't going to work without iproute2. I think that means that much of ephemeral.py needs to be reworked too (which should finally be reasonable to do now that we're getting distros plumbed through that module).

igalic · 2023-04-13T19:38:29Z

+            pid_file,
+            interface,
+            "-sf",
+            "/bin/true",


these options are, unfortunately, not available in freebsd, https://man.freebsd.org/cgi/man.cgi?query=dhclient&apropos=0&sektion=8&manpath=FreeBSD+14.0-CURRENT&arch=default&format=html
it's one of the many reasons why we're looking into migrating to dhcpcd

I'll propose something that uses the equivalents available in freebsd

igalic · 2023-04-13T21:12:49Z

+
+    @staticmethod
+    def get_dhclient_d():
+        # find lease files directory


why isn't this just reaching into distro to ask where to find it?

Good question. This is basically a direct copy/paste from DataSourceCloudStack, but modified to be in a class.

From the git history I see that:

6 years ago the network manager directory was added. I suspect that this is no longer required, since recently when digging around in upstream networkmanager code I recall seeing that isc-dhclient was dropped in favor of an internal implementation. A hasty grep of my local (ubuntu) NetworkManager-based system sees nothing that matches dhcp-server-identifier, which is the whole point of this function.

12 years ago Ubuntu support was added, Fedora was already supported

I'm a bit hesitant to split this up, however, because of the risk of accidentally breaking one distro or another by making this more exact. Unless someone wants to audit all of the distros, I'd be more comfortable leaving it as is for now.

igalic · 2023-04-13T21:14:14Z

+        #   dhclient-<iface>.leases, dhclient6.leases
+        # centos7: ('--' is not a typo)
+        #   dhclient--<iface>.lease, dhclient6.leases
+        for fname in lease_files:


is this going to be moved to distro in the next revision of this pr?

It could be, but I'm not going to prioritize it right now. Right now my focus is getting functionality correct without risking breakage of things that currently work.

holmanb · 2023-04-13T22:43:08Z

        print("Machine is not a Vultr instance")
        sys.exit(1)

+    # It should probably be safe to try to detect distro via stages.Init(),


@eb3095 I'm not quite sure what __main__ in this module gets used for, or if it is even still used at all. Would you mind looking at this section if you get a chance?

With this change, piping distros throughout the dhcp and ephemeral setup code means that vultr.get_metadata() will now require a distro object, which will add flexibility across distros. The approach in this PR uses the same strategy that cloud-init normally uses. Will that work here?

Based on the is_vultr() check above, I assume that this should be running on an image with cloud-init. Therefore, I assume that this approach would work, but I want to double check my assumption.

Yeah this should work fine. main i believe was an early discussion about how to debug. I dont think we have ever really used it, but that was the intended purpose.

eb3095 · 2023-04-15T18:19:07Z

        print("Machine is not a Vultr instance")
        sys.exit(1)

+    # It should probably be safe to try to detect distro via stages.Init(),


Yeah this should work fine. main i believe was an early discussion about how to debug. I dont think we have ever really used it, but that was the intended purpose.

blackboxsw

Quick first pass. Wanted to get my BSD manage_service question out there as it may involve a bit of refactor to retain previous behavior. I'll continue the review today.

blackboxsw · 2023-04-17T15:08:04Z

+    return latest_file
+
+
+def parse_dhcp_server_from_lease_file(lease_file):


Is there opportunity to leverage parse_dhcp_lease_file here?

We could also speed up this code potentially by reversing the order of leases processed to break on the first case of dhcp-server-identifier instead of processing the whole file and keeping the last case. It may not matter much in practice as the lease file processed in most cases may only have 1 lease in it though.

Is there opportunity to leverage parse_dhcp_lease_file here?

It looks like it. This code came from a different file and looks like a case of partial code duplication. For future code archeology I think that refactoring that to deduplicate code might be better in a separate (smaller) PR, since this one is already touching and changing a lot of things.

We could also speed up this code potentially...

Agreed. I'd like for that to be a separate PR for a cleaner git history, since it doesn't fit the scope of this change.

blackboxsw · 2023-04-17T15:17:53Z

        return ["usermod", "-G", group_name, member_name]

-    def manage_service(self, action: str, service: str):
+    def manage_service(self, action: str, service: str, rcs=None):


Forgot the @classmethod

I see it in the latest version of this branch. I'm guessing this got fixed in a later commit than the one that you reviewed.

blackboxsw · 2023-04-17T15:40:14Z

            # the routes are not recreated.
-            subp.subp(
-                ["service", "dhclient", "stop", dhcp_interface],
-                rcs=[0, 1],
-                capture=True,
+            net.dhcp.IscDhclient.start_service(
+                dhcp_interface, distros.freebsd.Distro
            )


I don't think we can make this shift as-written. These definitions are no longer the same, the new IscDhclient.start_service method completely ignores the dhcp_interface because we are using distro.manage_interface and dropping the supplemental dhcp_interface argument in the refactor.

Instead of running:
service dhclient stop eth0
we now run only the following on FreeBSD
service dhclient stop

Nice catch, thanks. This should be fixed in f1eb8ac.

blackboxsw · 2023-04-17T15:51:42Z

+
+    @staticmethod
+    def parse_dhcp_lease_file(lease_file):
+        """Parse the given dhcp lease file for the most recent lease.


This returns a list of all leases. Let's use type annotations on the function definitions here to guide our expectations about params and return types
And let's also either:

fix the logic to return only one lease and correct the one call-site in ephemeral.py to assume most recent lease
-- or --

Fix the docstr header

fixed in c65dfe7

Thanks. Still missing either docstr fix

Suggested change

"""Parse the given dhcp lease file for the most recent lease.

"""Parse the given dhcp lease file returning all leases as dicts.

Sorry, I must have accidentally dropped that in a rebase. Thanks!

holmanb · 2023-04-17T18:26:00Z

Quick first pass. Wanted to get my BSD manage_service question out there as it may involve a bit of refactor to retain previous behavior. I'll continue the review today.

Thanks for the initial review @blackboxsw. I think that I have addressed all of your comments.

Additionally I just pushed up the refactor (commit 9d16846) that makes network ops in ephemeral.py distro-agnostic (cc @igalic).

blackboxsw

Looks good @holman. Just to make sure I'm not lost in refactor intents, if we can separate net_ops work from the IscDhclient work it'd make me more certain we aren't missing something in review.

blackboxsw · 2023-04-17T22:56:58Z

+
+    @staticmethod
+    def parse_dhcp_lease_file(lease_file):
+        """Parse the given dhcp lease file for the most recent lease.


Thanks. Still missing either docstr fix

Suggested change

"""Parse the given dhcp lease file for the most recent lease.

"""Parse the given dhcp lease file returning all leases as dicts.

blackboxsw · 2023-04-17T23:00:59Z


    @staticmethod
-    def parse_dhcp_lease_file(lease_file):
+    def parse_dhcp_lease_file(lease_file: str) -> list:


I think we may want to from typing import Any, Dict, List and have to following:

Suggested change

def parse_dhcp_lease_file(lease_file: str) -> list:

def parse_dhcp_lease_file(lease_file: str) -> List[Dict[str, Any]]:

blackboxsw · 2023-04-17T23:01:56Z

    # This is used by self.shutdown_command(), and can be overridden in
    # subclasses
    shutdown_options_map = {"halt": "-H", "poweroff": "-P", "reboot": "-r"}
+    net_ops = iproute2


This net_ops work feels like a bolt-on ideal that represents a bit of scope creep for this PR. Would it be okay to separate this to another PR to ease review? Otherwise, we can spend a bit more time going through this in this PR but I worry a bit about missing something here in the refactor dance.

That's fair. I'll break it out.

blackboxsw · 2023-04-17T23:06:17Z

-                ["service", "dhclient", "stop", dhcp_interface],
-                rcs=[0, 1],
-                capture=True,
+            net.dhcp.IscDhclient.start_service(


@holmanb it looks like this PR unintentionally inverts the operations, the intent here was to stop any existing dhcp services on any dhcp interfaces before starting them again (instead of just using the systemctl restart operation). But it looks like we are now starting and then stopping. Let's flip them back.

Nice catch! Fixed.

holmanb · 2023-04-18T02:49:23Z

Looks good @holman.

Thanks! However, Zach might be confused by the Github notification from your comment ;)

Just to make sure I'm not lost in refactor intents, if we can separate net_ops work from the IscDhclient work it'd make me more certain we aren't missing something in review.

Nice attention to detail in the review. And I agree - breaking that out into a separate commit will make this a more coherent commit, in addition to helping review burden. I've moved that work here.

I believe I've addressed all of your comments.

blackboxsw

@holmanb LGTM, thanks for separating the PRs into something easier to review. Refactor looks good.

blackboxsw

~~hold merge on this for a moment, I want to check that Vultr invocation of Init I had missed.~~
I realize now this was just when calling python3 -m cloudinit.sources.DataSourceVultr. Nothing to see here. I'm good with us loading stages.Init in one-off test/debug invocations of the module itself.

blackboxsw

Clear, If we have to call stages.Init() to get the distro object for callers of __main__. I think that's ok. We could source this from the sources.pkl_load(/var/lib/cloud/instance/obj.pkl), but I think stages.Init() is probably an easier entry point.

holmanb added 5 commits April 13, 2023 12:24

shift dhclient behavior into cloudinit/net, start plumbing distro obj…

4105cfa

…ect to dhcp

use manage_service to manage dhcp service

f604ed2

refactor isc-dhclient usage into a class

5c88fec

pipe distro object from datasource definition through ephemeral to dh…

de9506a

…cp code

only discover other clients when first isn't found, add failover test

3e1592c

holmanb force-pushed the holmanb/dhcpcd branch 2 times, most recently from e4a14ec to aab1652 Compare April 13, 2023 18:59

format

5f499c2

holmanb force-pushed the holmanb/dhcpcd branch from aab1652 to 5f499c2 Compare April 13, 2023 19:02

holmanb requested a review from igalic April 13, 2023 21:40

igalic reviewed Apr 13, 2023

View reviewed changes

pylint

455af74

holmanb commented Apr 13, 2023

View reviewed changes

jfroche mentioned this pull request Apr 14, 2023

net/dhcp: add udhcpc support #2125

Closed

3 tasks

eb3095 approved these changes Apr 15, 2023

View reviewed changes

blackboxsw reviewed Apr 17, 2023

View reviewed changes

fix FreeBSD service call

f1eb8ac

holmanb force-pushed the holmanb/dhcpcd branch 5 times, most recently from 340851b to d8b6bbe Compare April 17, 2023 18:19

holmanb requested a review from blackboxsw April 17, 2023 18:28

holmanb assigned blackboxsw Apr 17, 2023

blackboxsw requested changes Apr 17, 2023

View reviewed changes

holmanb mentioned this pull request Apr 18, 2023

Support Ephemeral Networking for BSD #2127

Merged

comments

9323898

holmanb force-pushed the holmanb/dhcpcd branch from d8b6bbe to 9323898 Compare April 18, 2023 02:35

comments

f04209f

holmanb requested a review from blackboxsw April 18, 2023 02:55

comments

b7330d6

blackboxsw approved these changes Apr 19, 2023

View reviewed changes

blackboxsw requested changes Apr 19, 2023

View reviewed changes

blackboxsw approved these changes Apr 19, 2023

View reviewed changes

holmanb merged commit 5942f40 into canonical:main Apr 19, 2023

igalic mentioned this pull request Jun 20, 2023

CloudStack Datasource Fails to find Default Gateway on FreeBSD due to no proc filesystem #4191

Open

		return latest_file


		def parse_dhcp_server_from_lease_file(lease_file):

	"""Parse the given dhcp lease file for the most recent lease.
	"""Parse the given dhcp lease file returning all leases as dicts.

	def parse_dhcp_lease_file(lease_file: str) -> list:
	def parse_dhcp_lease_file(lease_file: str) -> List[Dict[str, Any]]:

Conversation

holmanb commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Additional Context

Test Steps

Checklist:

Uh oh!

igalic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holmanb Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blackboxsw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holmanb Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holmanb Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holmanb commented Apr 17, 2023

Uh oh!

blackboxsw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holmanb Apr 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holmanb Apr 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

holmanb commented Apr 13, 2023 •

edited

Loading

holmanb Apr 13, 2023 •

edited

Loading

holmanb Apr 17, 2023 •

edited

Loading

holmanb Apr 17, 2023 •

edited

Loading

holmanb Apr 18, 2023 •

edited

Loading

holmanb Apr 18, 2023 •

edited

Loading

holmanb commented Apr 18, 2023 •

edited

Loading

blackboxsw left a comment •

edited

Loading

blackboxsw left a comment •

edited

Loading