Add Whippet to bottlerocket #3270

yeazelm · 2023-07-19T02:29:57Z

Issue number: 2449

Description of changes:
This is an initial commit for whippet. There is more work to be done to make this robust, but this glues together the bits so that DNS works on top of #3266. With both of these changes, we should have a basic working stack.

This relies on systemd to restart whippet often if it fails. This works well enough for now, but the overall approach needs to be hardened up for a proper daemon that we want for this purpose.

Things this doesn't do:

Handle errors nicely, much better error handling will need to be done to make whippet not die all the time.
Creates a proper loop for waiting on D-Bus signals. Instead if fires off the netdog command and relies upon restart by systemd
Nice signal handling. It will die if restarted by systemctl and more or less is functional, but there are likely edge cases that would make this less robust for production
Have proper logging with levels. Right now it just uses println!()
Have unit or integration tests

There will be more work to come to make this better, but I wanted to get a starting point out in the wild for folks to look at.

I confirmed that when enabling the image-feature of systemd-networkd, whippet shows up in the image:

find /mnt -name "*whippet*"
/mnt/x86_64-bottlerocket-linux-gnu/sys-root/usr/lib/systemd/system/whippet.service
/mnt/x86_64-bottlerocket-linux-gnu/sys-root/usr/bin/whippet

and otherwise whippet is excluded from the image.

Testing done:
I have done a lot of manual testing on QEMU and EC2. I have basic output from my EC2 instance:

bash-5.1# ping amazon.com
PING amazon.com (205.251.242.103) 56(84) bytes of data.
64 bytes from s3-console-us-standard.console.aws.amazon.com (205.251.242.103): icmp_seq=1 ttl=219 time=64.1 ms
64 bytes from s3-console-us-standard.console.aws.amazon.com (205.251.242.103): icmp_seq=2 ttl=219 time=63.2 ms

--- amazon.com ping statistics ---
2 packets transmitted, 2 received, 0% packet loss, time 1002ms
rtt min/avg/max/mdev = 63.186/63.645/64.105/0.459 ms

bash-5.1# journalctl -u whippet.service
Jul 19 02:17:46 localhost systemd[1]: Starting Bottlerocket D-Bus listener for network events...
Jul 19 02:17:46 localhost systemd[1]: Started Bottlerocket D-Bus listener for network events.
Jul 19 02:17:46 localhost whippet[1042]: Calling netdog to get primary interface
Jul 19 02:17:46 localhost whippet[1042]: Primary interface is eth0
Jul 19 02:17:46 localhost whippet[1042]: Link id: 2 Name: eth0 Path: OwnedObjectPath(ObjectPath("/org/freedesktop/network1/link/_32"))
Jul 19 02:17:46 localhost whippet[1042]: Found eth0 is configuring
Jul 19 02:17:46 localhost whippet[1042]: Link id: 1 Name: lo Path: OwnedObjectPath(ObjectPath("/org/freedesktop/network1/link/_31"))
Jul 19 02:17:46 localhost whippet[1042]: DEBUG: found lo but is not eth0
Jul 19 02:17:48 localhost whippet[1042]: org.freedesktop.network1.Link.AdministrativeState changed to `Str(Str(Borrowed("configured")))`
Jul 19 02:17:48 localhost whippet[1042]: org.freedesktop.network1.Link.IPv6AddressState changed to `Str(Str(Borrowed("degraded")))`

The output is rough for now and could be cleaned up.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

zmrow

Mostly nits since I know this is POC code.

The only thing I'd like us to investigate before merging is the fact the package is built for everything, not just variants using systemd-networkd.

packages/os/os.spec

sources/whippet/README.md

sources/whippet/src/main.rs

zmrow · 2023-07-19T22:00:30Z

sources/whippet/src/networkmanager.rs

+    default_path = "/org/freedesktop/network1",
+    assume_defaults = false
+)]
+trait NetworkManager {


I'm on the fence about including all of these. I do appreciate the effort it took to gather this information and the fact we could use them, but I think we should probably whittle this down to what we do use. I would like to save them for future reference and use, though... Perhaps we comment them out with a message "for potential future use" or something?

I agree we should trim this down. These are autogenerated from zbus_xmlgen so we can always go back and get them again. I think I can trim these down as long as I document where we got them and how one would efficiently add things back in.

I pulled out many of the things we are unlikely to use (the write stuff) but left some of the other read functions since its still early and I realized we might have nicer ways to do this. For example, we might shift to get_link_by_name vs list_links. I also anticipate us finding edge cases where the *_state calls might be needed. We can trim as we discover they are not needed as we test this code more.

Whippet is a D-Bus listener that subscribes to the network1 D-Bus service on the system bus. This listens for events that happen to the primary interface and call netdog to notify it so netdog can update anything that has changed with the primary interface configuration. This commit adds a rudimentary but functional version of whippet. There is more work to be done to make it a robust, reliable daemon. Signed-off-by: Matthew Yeazel <[email protected]>

This adds the whippet service file to os when compiling with systemd-networkd support enabled. This should only affect variants that are using systemd-networkd.

zmrow

I'm good to merge this as POC code that won't be included in current variants. I'll pick up the torch once it's merged and can continue the outstanding items.

stmcginnis

Looks good to me. And if we find anything wrong, we have some time to correct and adjust. Probably better to have this in and usable to find out if that is the case.

bcressey · 2023-07-21T20:34:47Z

packages/os/os.spec

+%package -n %{_cross_os}whippet
+Summary: D-Bus listener and marshaller
+%description -n %{_cross_os}whippet
+%{summary}.
+


nit: you should make the whole sub-package conditional rather than creating an empty rpm with no files

bcressey · 2023-07-21T20:35:33Z

packages/os/whippet.service

@@ -0,0 +1,15 @@
+[Unit]
+Description=Bottlerocket D-Bus listener for network events


nit: doesn't necessarily need to be Bottlerocket-specific

Suggested change

Description=Bottlerocket D-Bus listener for network events

Description=D-Bus listener for network events

bcressey · 2023-07-21T20:41:49Z

sources/whippet/Cargo.toml

+
+[dependencies]
+snafu = "0.7"
+zbus = "3.8.0"


I'd rather not mix async runtimes:

Suggested change

zbus = "3.8.0"

zbus = { version = "3.8.0", default-features = false, features = ["tokio"] }

bcressey · 2023-07-21T20:43:03Z

sources/deny.toml

+    # gptman is locked to older nix but is fine to move up
+    # https://github.com/rust-disk-partition-management/gptman/pull/113


That pull request was closed and says it's not fine to move up.

bcressey · 2023-07-21T20:43:29Z

sources/whippet/src/main.rs

@@ -0,0 +1,164 @@
+/*!
+whippet is a D-Bus listener that reponds to events on D-Bus.


Suggested change

whippet is a D-Bus listener that reponds to events on D-Bus.

whippet is a D-Bus listener that responds to events on D-Bus.

bcressey · 2023-07-21T20:49:48Z

sources/whippet/src/main.rs

+    // Call netdog primary-interface to get the name of the primary interface
+    println!("Calling netdog to get primary interface");
+    let primary_interface_name_result = Command::new(NETDOG)
+        .arg("primary-interface")
+        .output()
+        .context(error::NetdogExecutionSnafu)?;
+    ensure!(
+        primary_interface_name_result.status.success(),
+        error::FailedNetdogSnafu {
+            stderr: String::from_utf8_lossy(&primary_interface_name_result.stderr)
+        }
+    );
+
+    let primary_interface_output_str = String::from_utf8(primary_interface_name_result.stdout)
+        .context(error::PrimaryInterfaceStringSnafu {})?;
+    let primary_interface: String = primary_interface_output_str
+        .trim()
+        .to_lowercase()
+        .trim_matches('"')
+        .to_string();
+    println!("Primary interface is {}", &primary_interface);
+
+    // Put the path in an option since we might not find it
+    let mut path_to_primary: Option<&OwnedObjectPath> = None;


whippet should have a config file that tells it what interface(s) to watch and what commands to run in response.

bcressey · 2023-07-21T20:52:59Z

sources/whippet/src/main.rs

+                println!(
+                    "Calling netdog write-primary-interface-status for {}",
+                    link_status.name
+                );
+                let primary_interface_status_result = Command::new(NETDOG)
+                    .arg("write-primary-interface-status")
+                    .output()
+                    .context(error::NetdogExecutionSnafu)?;
+                ensure!(
+                    primary_interface_status_result.status.success(),
+                    error::FailedNetdogSnafu {
+                        stderr: String::from_utf8_lossy(&primary_interface_status_result.stderr)
+                    }
+                );


I expect you'll want to use tokio::process::Command to avoid causing problems with the async runtime.

This seems like it should be a log message, either a warning or an error, and not cause the process to terminate.

bcressey · 2023-07-21T20:54:32Z

sources/whippet/src/main.rs

+        } else {
+            println!("DEBUG: found {} but is not {}", name, &primary_interface);
+        }


This should all be standard logging statements - debug!, warn! etc.

bcressey · 2023-07-21T21:04:59Z

sources/whippet/src/main.rs

+            if link_status.administrative_state == "configured" {
+                // call netdog now since its already configured, then the polling for changes can block
+                println!(
+                    "Calling netdog write-primary-interface-status for {}",
+                    link_status.name
+                );


This is racy because the interface may not be configured here, but may become configured before we are listening for the signal that tells us it's now configured.

yeazelm requested a review from zmrow July 19, 2023 02:30

yeazelm force-pushed the whippet branch from c10777b to 6af81e4 Compare July 19, 2023 19:35

zmrow reviewed Jul 19, 2023

View reviewed changes

yeazelm force-pushed the whippet branch from 6af81e4 to bba4ac6 Compare July 20, 2023 15:12

packages/os: Add Whippet to os

9e2b971

This adds the whippet service file to os when compiling with systemd-networkd support enabled. This should only affect variants that are using systemd-networkd.

yeazelm force-pushed the whippet branch from bba4ac6 to 9e2b971 Compare July 20, 2023 18:50

zmrow approved these changes Jul 20, 2023

View reviewed changes

yeazelm marked this pull request as ready for review July 20, 2023 22:01

stmcginnis approved these changes Jul 21, 2023

View reviewed changes

etungsten approved these changes Jul 21, 2023

View reviewed changes

bcressey reviewed Jul 21, 2023

View reviewed changes

yeazelm closed this Aug 31, 2023

yeazelm deleted the whippet branch December 19, 2023 21:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Whippet to bottlerocket #3270

Add Whippet to bottlerocket #3270

yeazelm commented Jul 19, 2023 •

edited

Loading

zmrow left a comment

zmrow Jul 19, 2023

yeazelm Jul 20, 2023

yeazelm Jul 20, 2023

zmrow left a comment

stmcginnis left a comment

bcressey Jul 21, 2023

bcressey Jul 21, 2023

bcressey Jul 21, 2023

bcressey Jul 21, 2023

bcressey Jul 21, 2023

bcressey Jul 21, 2023

bcressey Jul 21, 2023

bcressey Jul 21, 2023

bcressey Jul 21, 2023

		@@ -0,0 +1,15 @@
		[Unit]
		Description=Bottlerocket D-Bus listener for network events

	Description=Bottlerocket D-Bus listener for network events
	Description=D-Bus listener for network events

	zbus = "3.8.0"
	zbus = { version = "3.8.0", default-features = false, features = ["tokio"] }

		# gptman is locked to older nix but is fine to move up
		# https://github.com/rust-disk-partition-management/gptman/pull/113

		@@ -0,0 +1,164 @@
		/*!
		whippet is a D-Bus listener that reponds to events on D-Bus.

	whippet is a D-Bus listener that reponds to events on D-Bus.
	whippet is a D-Bus listener that responds to events on D-Bus.

Add Whippet to bottlerocket #3270

Add Whippet to bottlerocket #3270

Conversation

yeazelm commented Jul 19, 2023 • edited Loading

zmrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmrow left a comment

Choose a reason for hiding this comment

stmcginnis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeazelm commented Jul 19, 2023 •

edited

Loading