IPv6 static address creation fails sometimes

[andrewjstone]

During multi-sled deployment testing with falcon, the static unique local underlay0/bootstrap6 address fails to properly initialize and we end up with the following (note the question mark):

underlay0/linklocal addrconf ok         fe80::8:20ff:fe95:d1b7/10
underlay0/?       static   ok           fdb0:208:20c7:ea0f::1/64

The following error message gets written to the sled-agent svc log:

sled-agent: Failed to initialize bootstrap address: Failed to create address fdb0:208:20c7:ea0f::1 with name bootstrap6 in the GZ on "underlay0": Failed to create address Static(V6(Ipv6Network { addr: fdb0:208:20c7:ea0f::1, prefix: 64 })) with name underlay0/bootstrap6 in global: Zone execution error: Command [/usr/sbin/ipadm create-addr -t -T static -a fdb0:208:20c7:ea0f::1/64 underlay0/bootstrap6] executed and failed with status: exit status: 1. Stdout:

The creation of the static IPv6 address must occur after the creation of the link-local address on the same interface (underlay0 in this case). After some testing, and based on prior discoveries, @rcgoodfellow realized that there appears to be a race condition. The call to ipadm to create the link-local address is not exactly synchronous, and so it returns before the kernel is done with initialization. When the call to create the static address occurs too soon after the creation of the link-local, we get the failure above.

This theory was further verified by inserting a 10 second sleep in the call to zone::ensure_has_global_zone_v6_address between the call to create the link-local address and the static address. With that sleep inserted, the static address is created successfully on the underlay0/bootstrap6 network.

The ideal fix for this is to make the call to ipadm synchronous inside illumos. As a workaround though, we can ensure that we see that the link-local address has been successfully created before we try to create the underlay0 address.

[smklein]

Good catch; I didn't realize the ipadm call was not synchronous for create-addr (it's ipadm that's the problem, not dladm, right?).

To avoid this from biting us elsewhere, we could alter:

omicron/sled-agent/src/illumos/zone.rs

Lines 507 to 542 in 2737f24

	/// Ensures a link-local IPv6 exists with the name provided in `addrobj`.
	///
	/// A link-local address is necessary for allocating a static address on an
	/// interface on illumos.
	///
	/// For more context, see:
	/// <https://ry.goodwu.net/tinkering/a-day-in-the-life-of-an-ipv6-address-on-illumos/>
	#[allow(clippy::needless_lifetimes)]
	pub fn ensure_has_link_local_v6_address<'a>(
	zone: Option<&'a str>,
	addrobj: &AddrObject,
	) -> Result<(), crate::illumos::ExecutionError> {
	if let Ok(()) = Self::has_link_local_v6_address(zone, &addrobj) {
	return Ok(());
	}
	// No link-local address was found, attempt to make one.
	let mut command = std::process::Command::new(PFEXEC);
	let prefix =
	if let Some(zone) = zone { vec![ZLOGIN, zone] } else { vec![] };
	let create_addr_args = &[
	IPADM,
	"create-addr",
	"-t",
	"-T",
	"addrconf",
	&addrobj.to_string(),
	];
	let args = prefix.iter().chain(create_addr_args);
	let cmd = command.args(args);
	execute(cmd)?;
	Ok(())
	}

by adding a "retry loop", checking for existence of the created address before we return.

However this isn't the only spot where we make an ipadm call. Do we need to add this to all spots where ipadm is invoked?

This is not the first time we've struggled with the "it's not clear which commands are synchronous vs asynchronous" issue, and in particular, the "if it is asynchronous, how should we actually check when it's done" problem.

[andrewjstone]

Good catch; I didn't realize the ipadm call was not synchronous for create-addr (it's ipadm that's the problem, not dladm, right?).

Yes, I mistyped. It's ipadm. I updated the description to clarify. Thanks for catching that.

However this isn't the only spot where we make an ipadm call. Do we need to add this to all spots where ipadm is invoked?

Yes, I believe we should.

[bnaecker]

I suspect that even if we wait for the link-local to show up in the output of ipadm, that will only shorten the race window, not eliminate the race entirely. That's just because the actual call to create the static address address does not fail with an error like: Can't assign requested address, which is what you see if you try to create a static IPv6 address when there is definitely no LL on the same interface. Since we don't see that error, presumably ipadm thinks there is such a link-local address already. My guess is that it'll still be possible for us to see that address in the ipadm show-addr output, but that the initialization hasn't yet really completed. It's probably still practically useful to add a loop checking for the existence of the LL address, but we might expect to still see this error, if more rarely.

[andrewjstone]

A sleep of 10 seconds was added between creation of the link-local address and the static address with the assumption that the problem was the link-local address not being fully created before the static address was created. This actually bypassed the issue every time leading to the static address with underlay0/bootstrap6 addrobj to be created successfully.

However, looking at the logs again, the error message itself seemed to indicate that the static address was being created twice, which does not really track with the assumption that this was related to the prior call to creating the link-local address not completing in time.

sled-agent: Failed to initialize bootstrap address: Failed to create address fdb0:208:2085:d1ac::1 with name bootstrap6 in the GZ on "underlay0": Failed to create address Static(V6(Ipv6Network { addr: fdb0:208:2085:d1ac::1, prefix: 64 })) with name underlay0/bootstrap6 in global: Zone execution error: Command [/usr/sbin/ipadm create-addr -t -T static -a fdb0:208:2085:d1ac::1/64 underlay0/bootstrap6] executed and failed with status: exit status: 1. Stdout:
, Stderr: ipadm: Could not create address: Address object already exists. 

@rmustacc was curious about this and why we got the zombie addrobj underlay0/? with the correct address in the first place. We went spelunking through the illumos code and inspecting my running VM with mdb, and saw that the kernel had the interface with the right IP address. So the problem was in the library part of the system where the addrobj names (like underlay0/bootstrap6)get attached (ipmgmtd?). Keeping with our speculation that creating the link-local address was not synchronous, Robert hypothesized that this was due to duplicate address detection (DAD) that must talk over the local link to see that there aren't any other interfaces with the same IP addresses on the network. From reading the code though, we determined that DAD was indeed synchronous and ipadm create-addr wouldn't return before it completed, thus again contradicting the initial assumption that the link-local address wasn't fully created before the static address creation attempt.

I then reproduced the issue on fresh VMs to try to see if there was another error before the Address object already exists for the static ip. When doing this I went to the beginning of the sled-agent log and noticed that there was actually a service restart in there before the address creation got logged. This reminded me that thing-flinger restarts the sled-agent service with svcadm restart sled-agent, because when it runs omicron-package deploy install that starts the service, but the server specific files haven't been overlayed yet on the installed package directory. We must restart sled-agent to pick them up. However, the logs don't show that address creation was attempted, but this is could be due to async logging in Omicron. So a new speculation is that at exactly the wrong time, when the static address is being created, the service is restarted. Then when sled-agent starts up again it tries to recreate it and we get the Address object already exists error. In this scenario the sleep works to prevent the issue by delaying the creation of the static address so it never occurs before the sled-agent restart, not because it waits for an asynchronous link-local address creation to complete.

We wanted to verify this assumption and so Robert came up with a dtrace script that could be run before the thing-flinger deploy install command was run. The goal was to see if a the ipadm create-addr -t -T static ... call was being killed and therefore leaving us with a zombie. Running that showed, that indeed this is the case!

root@om1:~# pfexec dtrace -n 'proc:::signal-handle/execname == "ipadm"/{ printf("%d %s: 0x%x\n", pid, curpsinfo->pr_psargs, args[0]); }'
dtrace: description 'proc:::signal-handle' matched 1 probe
CPU     ID                    FUNCTION:NAME
  7    688               psig:signal-handle 1683 /usr/sbin/ipadm create-addr -t -T static -a fdb0:208:204a:f493::1/64 underlay0/: 0x9

The 0x9 at the end there is the SIGKILL getting sent to ipadm.

The immediate fix to this is to modify omicron-package deploy install and add a flag that doesn't start sled-agent and then use that flag in thing-flinger deploy install. That will fix the deliberate sabotage causing the problem.

Unfortunately, ipadm can always be killed in other, non-deliberate ways and trigger this issue. The deep problem here is the split brain between the kernel creating the interface and ip address, and the registration in user space of the addrobj used by ipadm. This is non-atomic and thus we must delete the interface and try again if we run into this situation, barring any fixes to illumos. The question now becomes: should we bother with worrying about this perfect kill storm now and perform cleanup in sled-agent if the problem occurs, or should we punt and perform the proper fix to illumos in the future.

[andrewjstone]

@rcgoodfellow was also able to reproduce the zombie directly with the following script:

#!/bin/bash

ipadm create-addr -t -T static -a fd00::1/64 vnic0/v6 &
PID=$!
sleep 0.3
kill -9 $PID