Skip to content

Commit

Permalink
The control plane should use libipcc (#4536)
Browse files Browse the repository at this point in the history
We should use libipcc in omicron rather than make direct
calls via `ioctl`. The library will take care of hiding the
implementation details from upstream consumers -- this becomes important
in the future when communication with the service processor from the
host OS physically changes with newer board design.

Currently the only consumer in this repo is installinator but the
control plane is about to start communicating with the RoT via IPCC as
well.

This PR introduces new bindings around `libipcc` and removes the old
`ioctl` interfaces.

---------

Co-authored-by: Andy Fiddaman <[email protected]>
  • Loading branch information
papertigers and citrus-it authored Jan 11, 2024
1 parent 0c7be4c commit 0528456
Show file tree
Hide file tree
Showing 19 changed files with 420 additions and 259 deletions.
45 changes: 41 additions & 4 deletions .cargo/config
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,50 @@
[build]
rustdocflags = "--document-private-items"

# On illumos, use `-znocompstrtab` to reduce link time.
# On illumos, use `-znocompstrtab` to reduce link time. We also add the Oxide
# specific platform directory to the RPATH where additional libraries can be
# found such as libipcc.
#
# Note that these flags are overridden by a user's environment variable, so
# things critical to correctness probably don't belong here.
# Our reasoning for including `-R/usr/platform/oxide/lib/amd64` here:
# - Oxide specific features - This path contains Oxide specific libraries such
# as libipcc and will likely grow over time to include more functionality.
# - Avoid the rpaths crate - The rpaths crate was built to deal with runtime
# paths that are dynamic such as with libraries like libpq which can live in
# different locations based on OS. This path will only ever be found on
# illumos and will be tied directly to the Oxide platform.
# - Less developer burden - Having something like the ipcc crate emit
# `DEP_IPCC_LIBDIRS` means that we end up littering the repo with Cargo.toml
# and build.rs changes whenever ipcc shows up somewhere in the dependency
# tree. While initially exploring that path we ran into a significant number
# of tests failing due to not being able to find libipcc in the runtime path
# which can be confusing or surprising to someone doing work on omicron.
#
# We could also update Helios so that a symlink is created from
# /usr/platform/oxide/lib/amd64 into /usr/lib/64 but we opted to not take
# this route forward as it meant waiting for another round of updates on
# shared build machines and to buildomat itself.
#
# As documented at:
# https://doc.rust-lang.org/cargo/reference/config.html#buildrustflags
#
# There are four mutually exclusive sources of extra flags. They are checked in
# order, with the first one being used:
# 1. `CARGO_ENCODED_RUSTFLAGS` environment variable.
# 2. `RUSTFLAGS` environment variable.
# 3. All matching target.<triple>.rustflags and target.<cfg>.rustflags config
# entries joined together.
# 4. build.rustflags config value.
#
# When overriding the defaults in this config by environment variable the user
# may need to manually pass the additional options found below.
#
# Note that other runtime paths should not be added here, but should instead
# reuse the infrastructure found in the `rpaths` crate which can be found in
# this repo. Those paths are usually platform specific and will vary based on a
# variety of things such as host OS.
[target.x86_64-unknown-illumos]
rustflags = [
"-C", "link-arg=-Wl,-znocompstrtab"
"-C", "link-arg=-Wl,-znocompstrtab,-R/usr/platform/oxide/lib/amd64"
]

# Set up `cargo xtask`.
Expand Down
9 changes: 9 additions & 0 deletions .github/buildomat/build-and-test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ set -o errexit
set -o pipefail
set -o xtrace

target_os=$1

# NOTE: This version should be in sync with the recommended version in
# .config/nextest.toml. (Maybe build an automated way to pull the recommended
# version in the future.)
Expand Down Expand Up @@ -48,6 +50,13 @@ ptime -m bash ./tools/install_builder_prerequisites.sh -y
#
banner build
export RUSTFLAGS="-D warnings"
# When running on illumos we need to pass an additional runpath that is
# usually configured via ".cargo/config" but the `RUSTFLAGS` env variable
# takes precedence. This path contains oxide specific libraries such as
# libipcc.
if [[ $target_os == "illumos" ]]; then
RUSTFLAGS="-D warnings -C link-arg=-R/usr/platform/oxide/lib/amd64"
fi
export RUSTDOCFLAGS="-D warnings"
export TMPDIR=$TEST_TMPDIR
export RUST_BACKTRACE=1
Expand Down
7 changes: 4 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ members = [
"installinator",
"internal-dns-cli",
"internal-dns",
"ipcc-key-value",
"ipcc",
"key-manager",
"nexus",
"nexus/authz-macros",
Expand Down Expand Up @@ -105,7 +105,7 @@ default-members = [
"installinator",
"internal-dns-cli",
"internal-dns",
"ipcc-key-value",
"ipcc",
"key-manager",
"nexus",
"nexus/authz-macros",
Expand Down Expand Up @@ -225,7 +225,7 @@ installinator-artifactd = { path = "installinator-artifactd" }
installinator-artifact-client = { path = "clients/installinator-artifact-client" }
installinator-common = { path = "installinator-common" }
internal-dns = { path = "internal-dns" }
ipcc-key-value = { path = "ipcc-key-value" }
ipcc = { path = "ipcc" }
ipnetwork = { version = "0.20", features = ["schemars"] }
itertools = "0.12.0"
key-manager = { path = "key-manager" }
Expand Down
2 changes: 1 addition & 1 deletion gateway/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ hex.workspace = true
http.workspace = true
hyper.workspace = true
illumos-utils.workspace = true
ipcc-key-value.workspace = true
ipcc.workspace = true
omicron-common.workspace = true
once_cell.workspace = true
schemars.workspace = true
Expand Down
13 changes: 6 additions & 7 deletions gateway/src/http_entrypoints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -443,11 +443,11 @@ pub struct ImageVersion {
pub version: u32,
}

// This type is a duplicate of the type in `ipcc-key-value`, and we provide a
// This type is a duplicate of the type in `ipcc`, and we provide a
// `From<_>` impl to convert to it. We keep these types distinct to allow us to
// choose different representations for MGS's HTTP API (this type) and the wire
// format passed through the SP to installinator
// (`ipcc_key_value::InstallinatorImageId`), although _currently_ they happen to
// (`ipcc::InstallinatorImageId`), although _currently_ they happen to
// be defined identically.
#[derive(
Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, JsonSchema,
Expand Down Expand Up @@ -1292,7 +1292,7 @@ async fn sp_power_state_set(

/// Set the installinator image ID the sled should use for recovery.
///
/// This value can be read by the host via IPCC; see the `ipcc-key-value` crate.
/// This value can be read by the host via IPCC; see the `ipcc` crate.
#[endpoint {
method = PUT,
path = "/sp/{type}/{slot}/ipcc/installinator-image-id",
Expand All @@ -1302,14 +1302,13 @@ async fn sp_installinator_image_id_set(
path: Path<PathSp>,
body: TypedBody<InstallinatorImageId>,
) -> Result<HttpResponseUpdatedNoContent, HttpError> {
use ipcc_key_value::Key;
use ipcc::Key;

let apictx = rqctx.context();
let sp_id = path.into_inner().sp.into();
let sp = apictx.mgmt_switch.sp(sp_id)?;

let image_id =
ipcc_key_value::InstallinatorImageId::from(body.into_inner());
let image_id = ipcc::InstallinatorImageId::from(body.into_inner());

sp.set_ipcc_key_lookup_value(
Key::InstallinatorImageId as u8,
Expand All @@ -1330,7 +1329,7 @@ async fn sp_installinator_image_id_delete(
rqctx: RequestContext<Arc<ServerContext>>,
path: Path<PathSp>,
) -> Result<HttpResponseUpdatedNoContent, HttpError> {
use ipcc_key_value::Key;
use ipcc::Key;

let apictx = rqctx.context();
let sp_id = path.into_inner().sp.into();
Expand Down
2 changes: 1 addition & 1 deletion gateway/src/http_entrypoints/conversions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,7 @@ impl From<StartupOptions> for HostStartupOptions {
}
}

impl From<InstallinatorImageId> for ipcc_key_value::InstallinatorImageId {
impl From<InstallinatorImageId> for ipcc::InstallinatorImageId {
fn from(id: InstallinatorImageId) -> Self {
Self {
update_id: id.update_id,
Expand Down
2 changes: 1 addition & 1 deletion installinator/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ http.workspace = true
illumos-utils.workspace = true
installinator-artifact-client.workspace = true
installinator-common.workspace = true
ipcc-key-value.workspace = true
ipcc.workspace = true
itertools.workspace = true
libc.workspace = true
omicron-common.workspace = true
Expand Down
4 changes: 2 additions & 2 deletions installinator/src/artifact.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use clap::Args;
use futures::StreamExt;
use installinator_artifact_client::ClientError;
use installinator_common::EventReport;
use ipcc_key_value::{InstallinatorImageId, Ipcc};
use ipcc::{InstallinatorImageId, Ipcc};
use omicron_common::update::{ArtifactHash, ArtifactHashId};
use tokio::sync::mpsc;
use uuid::Uuid;
Expand Down Expand Up @@ -47,7 +47,7 @@ pub(crate) struct ArtifactIdOpts {
impl ArtifactIdOpts {
pub(crate) fn resolve(&self) -> Result<InstallinatorImageId> {
if self.from_ipcc {
let ipcc = Ipcc::open().context("error opening IPCC")?;
let ipcc = Ipcc::new().context("error opening IPCC")?;
ipcc.installinator_image_id()
.context("error retrieving installinator image ID")
} else {
Expand Down
126 changes: 0 additions & 126 deletions ipcc-key-value/src/ioctl.rs

This file was deleted.

Loading

0 comments on commit 0528456

Please sign in to comment.