service zones may have device/link requirements #1820

Nieuwejaar · 2022-10-14T18:29:34Z

No description provided.

- Add support for mapping physical devices into a service zone - Add support for mapping existing links into a service zone - Don't start the switch zone if necessary devices are missing

bnaecker

A few small comments, but overall looks good!

bnaecker · 2022-10-17T16:47:36Z

sled-agent/src/services.rs

+        // When running on a real sidecar, we need the /dev/tofino device
+        // to talk to the tofino ASIC.
+        // TODO: this really needs to be checked periodically rather
+        // than once at startup.  Since the sidecar is in a difference


Spelling nit: "difference" -> "different"

bnaecker · 2022-10-17T16:58:32Z

sled-agent/src/services.rs

+    ) -> Result<Vec<String>, Error> {
+        // When running on a real sidecar, we need the /dev/tofino device
+        // to talk to the tofino ASIC.
+        // TODO: this really needs to be checked periodically rather


This is certainly true. Is it possible to start the zone unconditionally, and add the device file in as it becomes available? I think Dendrite should be able to communicate a failure to find the device, by returning some error condition to requests which require that.

I don't believe so, but I'll dig further. Devices in a zone are part of the zone config, which is fixed at zone startup. We could add tfpkt0 to a running zone, as it is a "link" rather than a "device".

There would need to be some OS work, but I don't think it's insurmountable. We'd need to be able to trigger reevaluation of the match rules in the configuration, probably in response to devfsadm or other sysevent activity, and create any missing nodes in the zone /dev directory.

As a short-term workaround, would it work to reboot the Dendrite zone if a new tofino device appears? We could monitor for this in the GZ, and react by rebooting the zone.

When built for tofino-asic, dpd will currently crash if /dev/tofino isn't present. I can change that easily enough. With that change, then rebooting when the tofino device (dis)appears should work.

Then the question becomes: if we're not keying off the presence of the device, when do we launch a switch zone? Do we really want that to be running on every gimlet?

When built for tofino-asic, dpd will currently crash if /dev/tofino isn't present. I can change that easily enough. With that change, then rebooting when the tofino device (dis)appears should work.

Sure, though would this change even be necessary? Presumably, the GZ could wait until such a device appears before launching the switch zone.

Then the question becomes: if we're not keying off the presence of the device, when do we launch a switch zone? Do we really want that to be running on every gimlet?

I do not think so - I think we want the switch zone software to exist on all gimlets (in case one is moved to the scrimlet location), but I think we'd prefer to refrain from launching the zone until we have confirmation this is a scrimlet device.

In #1510 I added a hard-coded configuration to make this distinction (if you wanted to test this now), but I do think the decision of "scrimlet vs gimlet" should be dynamically detectable, as discussed in https://github.com/orgs/oxidecomputer/projects/19/views/1

bnaecker · 2022-10-17T17:12:31Z

sled-agent/src/illumos/vnic.rs

@@ -86,6 +86,7 @@ impl<DL: VnicSource + Clone> VnicAllocator<DL> {
 /// communicating with Oxide services.
 #[derive(Debug, Clone, Copy, PartialEq)]
 pub enum VnicKind {
+    Physical,


It seems odd to have a variant of the virtial NIC type that represents a physical NIC. This appears to be bigger than this PR, so I don't think we should block it on fixing this, but we should probably change InstalledZone::install to accept zero or more PhysicalLinks as well as the Vnics we're currently passing in.

The logic in the zone creation is identical for vnics and physical nics, so I didn't want to complicate the interface for no functional reason. In fact, the zone install() calls the argument physical_nic: Option<Vnic>. I'm going to rename Vnic -> Link, which will resolve the existing ambiguity and prevent me from making it worse.

I agree that change is beyond the scope of this PR. The addition of a Physical variant to the Vnic enum just made clear that it's not really the right type or abstraction here. Nothing for you to do, but it might make sense to create an issue to clean this up.

bnaecker · 2022-10-17T19:11:39Z

sled-agent/src/services.rs

+            let nic_needed = match self.nic_needed(req) {
+                Ok(n) => n,
+                Err(e) => {
+                    info!(self.log, "skipping zone {}: {:?}", req.zone_name, e);


Should this be at warn! or error! log level? We may also want to specifically say that this is skipped because a TF-port variant zone requires the /dev/tfport device file.

warn! would be a good idea.

The error message already tells you which zone isn't being created and why with "failed to find device {device}", which I think is all you're asking for.

Rename illumos::vnic to illumos::link to reflect addition of physical nics Update to pull the latest dendrite bits, built with omicron-package 0.5.1

smklein

Looks good to me; biggest question is the point raised here: https://github.com/oxidecomputer/omicron/pull/1820/files#r997306229

smklein · 2022-10-26T15:30:39Z

sled-agent/src/illumos/link.rs

 /// Represents the kind of a VNIC, such as whether it's for guest networking or
 /// communicating with Oxide services.


Suggested change

/// Represents the kind of a VNIC, such as whether it's for guest networking or

/// communicating with Oxide services.

/// Represents the kind of a Link, such as whether it's for guest networking or

/// communicating with Oxide services.

smklein · 2022-10-26T15:41:50Z

sled-agent/src/services.rs

+    ) -> Result<Vec<String>, Error> {
+        // When running on a real sidecar, we need the /dev/tofino device
+        // to talk to the tofino ASIC.
+        // TODO: this really needs to be checked periodically rather


As a short-term workaround, would it work to reboot the Dendrite zone if a new tofino device appears? We could monitor for this in the GZ, and react by rebooting the zone.

- Expand the service manager to support initializing the switch zone as a background task. - Reserve the Switch Zone address as the second address in the sled's subnet. The first address is already reserved for the sled agent, but this ensures that any sled can "become" a scrimlet without needing to provision a new IP through Nexus. - This has some fall-out effects - we bump all the other addresses up, which are currently statically allocated. - Miscellaneous cleanups - Identify zones by a "ZoneType" enum, rather than a name. - Incorporate the portions of #1820 responsible for verifying links and devices when launching zones. Fixes oxidecomputer/minimum-upgradable-product#16 Part of oxidecomputer/minimum-upgradable-product#18

smklein · 2022-11-10T20:16:03Z

PSA: I've incorporated the salient parts of this PR into #1933 - I think that once that merged, this PR could be marked obsolete.

- Expand the service manager to support initializing the switch zone as a background task. - Reserve the Switch Zone address as the second address in the sled's subnet. The first address is already reserved for the sled agent, but this ensures that any sled can "become" a scrimlet without needing to provision a new IP through Nexus. - This has some fall-out effects - we bump all the other addresses up, which are currently statically allocated. - Miscellaneous cleanups - Identify zones by a "ZoneType" enum, rather than a name. - Incorporate the portions of #1820 responsible for verifying links and devices when launching zones. Fixes oxidecomputer/minimum-upgradable-product#16 Part of oxidecomputer/minimum-upgradable-product#18

Nieuwejaar · 2022-11-12T19:09:48Z

Closed - obsoleted by 1918 and 1933

Nieuwejaar added 2 commits October 13, 2022 17:36

Add support for tofino-required devices

3e86c1a

- Add support for mapping physical devices into a service zone - Add support for mapping existing links into a service zone - Don't start the switch zone if necessary devices are missing

fix clippy lint error

599687b

Nieuwejaar requested review from smklein and bnaecker October 14, 2022 18:29

bnaecker reviewed Oct 17, 2022

View reviewed changes

Address Ben's code review comments

6ec4f3f

Rename illumos::vnic to illumos::link to reflect addition of physical nics Update to pull the latest dendrite bits, built with omicron-package 0.5.1

smklein approved these changes Oct 26, 2022

View reviewed changes

smklein mentioned this pull request Nov 10, 2022

[sled-agent] Launch switch zone automatically on scrimlets #1933

Merged

Nieuwejaar closed this Nov 12, 2022

Nieuwejaar deleted the tfport branch November 12, 2022 19:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

service zones may have device/link requirements #1820

service zones may have device/link requirements #1820

Nieuwejaar commented Oct 14, 2022

bnaecker left a comment

bnaecker Oct 17, 2022

bnaecker Oct 17, 2022

Nieuwejaar Oct 18, 2022

jclulow Oct 19, 2022

smklein Oct 26, 2022

Nieuwejaar Oct 26, 2022

smklein Oct 26, 2022

bnaecker Oct 17, 2022

Nieuwejaar Oct 18, 2022

bnaecker Oct 18, 2022

bnaecker Oct 17, 2022

Nieuwejaar Oct 18, 2022

smklein left a comment

smklein Oct 26, 2022

smklein Oct 26, 2022

smklein commented Nov 10, 2022

Nieuwejaar commented Nov 12, 2022

		/// Represents the kind of a VNIC, such as whether it's for guest networking or
		/// communicating with Oxide services.

service zones may have device/link requirements #1820

service zones may have device/link requirements #1820

Conversation

Nieuwejaar commented Oct 14, 2022

bnaecker 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smklein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smklein commented Nov 10, 2022

Nieuwejaar commented Nov 12, 2022