Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import maghemite mg-ddm service #1249

Merged
merged 5 commits into from
Jun 23, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions deploy/src/bin/thing-flinger.rs
Original file line number Diff line number Diff line change
Expand Up @@ -480,19 +480,16 @@ fn overlay_sled_agent(

// TODO do we need any escaping here? this will definitely break if any dir
// names have spaces
let dirs = sled_agent_dirs
.iter()
.map(|dir| format!(" --directories {}", dir.display()))
.collect::<String>();
let dirs = sled_agent_dirs.iter().map(|dir| format!(" {}", dir.display()));

let cmd = format!(
"sh -c 'for dir in {}; do mkdir -p $dir; done' && \
cd {} && \
cargo run {} --bin sled-agent-overlay-files -- {}",
dirs,
dirs.clone().collect::<String>(),
config.builder.omicron_path.to_string_lossy(),
config.release_arg(),
dirs
dirs.map(|dir| format!(" --directories {}", dir)).collect::<String>(),
);
ssh_exec(builder, &cmd, false)
}
Expand Down
11 changes: 11 additions & 0 deletions package-manifest.toml
Original file line number Diff line number Diff line change
Expand Up @@ -102,3 +102,14 @@ commit = "d2b8184d11d5d5f56424fe0bee13ca03d576604e"
# The SHA256 digest is automatically posted to:
# https://buildomat.eng.oxide.computer/public/file/oxidecomputer/propolis/image/<commit>/propolis-server.sha256.txt
sha256 = "b192fbaaee850e63adaf9f7fe5ff2054a9de338d70198b1d1670285057a047be"

[external_package.maghemite]
service_name = "mg-ddm"
zone = false
[external_package.maghemite.source]
type = "prebuilt"
repo = "maghemite"
commit = "2be097ddd1d3fd8e7f56bc0a4bfd696253b11454"
# The SHA256 digest is automatically posted to:
# https://buildomat.eng.oxide.computer/public/file/oxidecomputer/maghemite/image/<commit>/mg-ddm.sha256.txt
sha256 = "94218915ec6fed75dcc81d736bd5e9ef62c9eb651a67a943b297d94d6b390941"
20 changes: 14 additions & 6 deletions package/src/bin/omicron-package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -330,12 +330,16 @@ fn do_install(
uninstall_all_packages(config);
uninstall_all_omicron_zones()?;

// Extract and install the bootstrap service, which itself extracts and
// installs other services.
if let Some(package) = config.packages.get("omicron-sled-agent") {
let tar_path =
install_dir.join(format!("{}.tar", package.service_name));
let service_path = install_dir.join(&package.service_name);
// Extract all global zone services.
let global_zone_service_names = config
.packages
.values()
.chain(config.external_packages.values().map(|p| &p.package))
.filter_map(|p| if p.zone { None } else { Some(&p.service_name) });

for service_name in global_zone_service_names {
let tar_path = install_dir.join(format!("{}.tar", service_name));
let service_path = install_dir.join(service_name);
println!(
"Unpacking {} to {}",
tar_path.to_string_lossy(),
Expand All @@ -347,7 +351,11 @@ fn do_install(
std::fs::create_dir_all(&service_path)?;
let mut archive = tar::Archive::new(tar_file);
archive.unpack(&service_path)?;
}

// Install the bootstrap service, which itself extracts and
// installs other services.
if let Some(package) = config.packages.get("omicron-sled-agent") {
let manifest_path = install_dir
.join(&package.service_name)
.join("pkg")
Expand Down
60 changes: 60 additions & 0 deletions sled-agent/src/bootstrap/maghemite.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at https://mozilla.org/MPL/2.0/.

//! Starting the mg-ddm service.

use crate::illumos::addrobj::AddrObject;
use slog::Logger;
use thiserror::Error;

const SERVICE_FMRI: &str = "svc:/system/illumos/mg-ddm";
const MANIFEST_PATH: &str = "/opt/oxide/mg-ddm/pkg/ddm/manifest.xml";

#[derive(Debug, Error)]
pub enum Error {
#[error("Error configuring service: {0}")]
Config(#[from] smf::ConfigError),

#[error("Error administering service: {0}")]
Adm(#[from] smf::AdmError),

#[error("Error starting service: {0}")]
Join(#[from] tokio::task::JoinError),
}

pub async fn enable_mg_ddm_service(
log: Logger,
interface: AddrObject,
) -> Result<(), Error> {
tokio::task::spawn_blocking(|| {
enable_mg_ddm_service_blocking(log, interface)
})
.await?
}

fn enable_mg_ddm_service_blocking(
log: Logger,
interface: AddrObject,
) -> Result<(), Error> {
info!(log, "Importing mg-ddm service"; "path" => MANIFEST_PATH);
smf::Config::import().run(MANIFEST_PATH)?;

// TODO-cleanup mg-ddm supports multiple interfaces, but `smf` currently
// doesn't expose an equivalent of `svccfg addpropvalue`. If we need
// multiple interfaces we'll need to extend smf.
let interface = interface.to_string();
info!(log, "Setting mg-ddm interface"; "interface" => interface.as_str());
smf::Config::set_property(SERVICE_FMRI).run(smf::Property::new(
smf::PropertyName::new("config", "interfaces").unwrap(),
smf::PropertyValue::Astring(interface),
))?;
Comment on lines +43 to +51
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this library is just a wrapper around the /usr/sbin/svccfg command - if it's too inflexible, it's totally okay to call out to that command manually.

(extending the library is also a viable option, as you suggested).


info!(log, "Enabling mg-ddm service");
smf::Adm::new()
.enable()
.temporary()
.run(smf::AdmSelection::ByPattern(&[SERVICE_FMRI]))?;

Ok(())
}
1 change: 1 addition & 0 deletions sled-agent/src/bootstrap/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ pub mod agent;
pub mod client;
pub mod config;
pub mod discovery;
mod maghemite;
pub mod multicast;
pub(crate) mod params;
pub(crate) mod rss_handle;
Expand Down
20 changes: 20 additions & 0 deletions sled-agent/src/bootstrap/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ use super::params::RequestEnvelope;
use super::trust_quorum::ShareDistribution;
use super::views::Response;
use super::views::ResponseEnvelope;
use crate::bootstrap::maghemite;
use crate::config::Config as SledConfig;
use crate::illumos::addrobj::AddrObject;
use crate::illumos::dladm::VnicSource;
use crate::sp::AsyncReadWrite;
use crate::sp::SpHandle;
use crate::sp::SprocketsRole;
Expand Down Expand Up @@ -63,6 +66,23 @@ impl Server {
debug!(log, "registered DTrace probes");
}

// Turn on the maghemite routing service.
info!(log, "Starting mg-ddm service");
let link = sled_config
.get_link()
.map_err(|err| format!("Failed to find physical link: {err}"))?;

// TODO-correctness (a) Is "linklocal" always right? (b) Do we need
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be correct with regards to RFD 63 section 6.3.1:

Peering and traffic forwarding between routers takes place over link-local addresses. When a router first starts, it sends out neighbor discovery protocol (NDP) router solicitations over link-local multicast to discover neighboring routers. Neighboring routers respond to multicast solicitations sourced from a link-local unicast address over unicast thereby establishing bi-directional unicast link-local communication. From there, the routers engage in a peering protocol and subsequently begin to exchange prefix advertisements.

I don't see why it would break single node behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should've been more specific. IIUC the name "linklocal" is just convention and not special, but maybe that's good enough? E.g., when I create fresh VMs, I don't actually have a $PHYS/linklocal address object without creating one manually.

The breakage is that starting the service fails if the address object we specify here doesn't exist. I think that failure is correct (since we won't be able to route off node without mg-ddm), but I don't want it to fail based on a bad assumption about this name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah. I was assuming this created the addrconf object. In production we should definitely fail if the link-local address doesn't exist right? I'd prefer not to qualify this for test only in that case as it's mighty easy to create a link-local address.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely have to have a link-local address, yes. I don't know who or when should create that - should bootstrap-agent do that here if doesn't exist?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure as to who creates the address, but I believe the intention is for that to be over the loopback interface lo0. (See this for some notes.)

There are multiple Chelsio physical links on the sled, both of which provide a potential path to talk to the bootstrap server. Maghemite is responsible for advertising the prefix over both Chelio links, and updating the OS's routing tables accordingly. Although @rcgoodfellow can talk in much more depth about it, my understanding is that traffic will be dynamically routed through one or the other Chelsio depending on the expected delay sending a packet down each of them. But it expects to be able to reach the bootstrap address through either link.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maghemite needs link-local/addrconf addresses to be set up for both sled physical interfaces it will route over. So if our interfaces on the sled are

  • cxgbe0
  • cxgbe1

Then the addrconf addresses

  • cxgbe0/linklocal
  • cxgbe1/linklocal

should be set up (not advocating specifically for "linklocal" there, but that seems to be the trend so rolling with it, I don't have a good name to suggest) early in the sled initialization process.

I don't have an opinion on who sets those up, only that they are set up before Maghemite starts so the address object names (i.e. "cxgbe0/linklocal") can be provided to Maghemite on startup. If the addrconf addresses are set up as temporary, this ordering of addresses before Maghemite will need to be ensured on each boot.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although @rcgoodfellow can talk in much more depth about it, my understanding is that traffic will be dynamically routed through one or the other Chelsio depending on the expected delay sending a packet down each of them. But it expects to be able to reach the bootstrap address through either link.

That's correct, but in order to do that, we need a distinct link-local address on each physical interface which provides a path to the interface-independent underlay ULA /64 prefix of the host.

Copy link
Contributor Author

@jgallagher jgallagher Jun 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rcgoodfellow!

I think the main followup question comes from

I don't have an opinion on who sets those up, only that they are set up before Maghemite starts

i.e., should sled-agent set up the addrconf addresses itself, or assume they already exist? Here in the code I'm assuming both that (a) it already exists and (b) it has the name $DATALINK/linklocal, and perhaps both of those assumptions are wrong. Could we extend AddressRequest, adding AddrConf, and then use ensure_address()? At that point we could pick whatever name we want, since sled-agent would be responsible for setting it up (and therefore knowing it to pass to Maghemite).

Copy link
Contributor Author

@jgallagher jgallagher Jun 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Poking around, we don't need to extend AddressRequest at all; we already have a function that will ensure an AddrObject exists with a link-local address. Added a call to it in 6309c7b. I believe this will fail if the interface already has an addrconf address of a different name, but hopefully that's okay? If something else is creating addrconf addresses before sled-agent, we should sync up on the name.

// mg-ddm to listen on multiple addresses?
let mg_interface = AddrObject::new(link.name(), "linklocal")
.expect("unexpected failure creating AddrObject");

// TODO-cleanup Should we `tokio::spawn()` this so we can proceed
jgallagher marked this conversation as resolved.
Show resolved Hide resolved
// concurrently with mg-ddm starting up?
maghemite::enable_mg_ddm_service(log.clone(), mg_interface)
.await
.map_err(|err| format!("Failed to start mg-ddm: {err}"))?;

info!(log, "detecting (real or simulated) SP");
let sp = SpHandle::detect(
config.sp_config.as_ref().map(|c| &c.local_sp),
Expand Down