Skip to content

Improve ignition target selection in faux-mgs #467

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Nov 5, 2025
Merged

Improve ignition target selection in faux-mgs #467

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
157f0e9
Working on better ignition target selection
mkeeter Oct 28, 2025
623aacc
Also use selector for other commands
mkeeter Oct 28, 2025
e85b7bc
Backwards compatibility
mkeeter Oct 28, 2025
735c920
Small cleanup
mkeeter Oct 28, 2025
dc5e155
Fix compatibility shim
mkeeter Nov 5, 2025
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
322 changes: 270 additions & 52 deletions faux-mgs/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,20 +172,20 @@ enum Command {
/// Ask SP for its current state.
State,

/// Get the ignition state for a single target port (only valid if the SP is
/// an ignition controller).
/// Get the ignition state for a port (only valid if the SP is an ignition
/// controller).
Ignition {
#[clap(
help = "integer of a target, or 'all' for all targets",
value_parser = IgnitionLinkEventsTarget::parse,
)]
target: IgnitionLinkEventsTarget,
#[clap(flatten)]
sel: IgnitionBulkSelectorWithCompatibilityShim,
},

/// Send an ignition command for a single target port (only valid if the SP
/// is an ignition controller).
IgnitionCommand {
target: u8,
// We can't use the compatibility shim here because it has an optional
// positional argument, which can't be followed by `command`
#[clap(flatten)]
sel: IgnitionSingleSelector,
#[clap(
help = "'power-on', 'power-off', or 'power-reset'",
value_parser = ignition_command_from_str,
Expand All @@ -196,21 +196,17 @@ enum Command {
/// Get bulk ignition link events (only valid if the SP is an ignition
/// controller).
IgnitionLinkEvents {
#[clap(
help = "integer of a target, or 'all' for all targets",
value_parser = IgnitionLinkEventsTarget::parse,
)]
target: IgnitionLinkEventsTarget,
#[clap(flatten)]
sel: IgnitionBulkSelectorWithCompatibilityShim,
},

/// Clear all ignition link events (only valid if the SP is an ignition
/// controller).
ClearIgnitionLinkEvents {
#[clap(
help = "integer of a target, or 'all' for all targets",
value_parser = IgnitionLinkEventsTarget::parse,
)]
target: IgnitionLinkEventsTarget,
// We can't use the compatibility shim here because it has an optional
// positional argument, which can't be followed by `transceiver_select`
#[clap(flatten)]
sel: IgnitionBulkSelector,
#[clap(
help = "'controller', 'target-link0', 'target-link1', or 'all'",
value_parser = IgnitionLinkEventsTransceiverSelect::parse,
Expand Down Expand Up @@ -643,6 +639,223 @@ impl std::fmt::Display for CfpaSlot {
}
}

#[derive(Copy, Clone, Debug, clap::Args)]
pub struct IgnitionSelector {
/// Ignition target
#[clap(short, long)]
target: Option<u8>,

/// Sled cubby (0-31)
#[clap(short, long, conflicts_with_all = ["target"])]
cubby: Option<u8>,

/// Sidecar ('local' or 'peer')
#[clap(
short, long, conflicts_with_all = ["target", "cubby"],
value_parser = parse_sidecar_selector
)]
sidecar: Option<SidecarSelector>,

/// PSC index (0-1)
#[clap(short, long, conflicts_with_all = ["target", "cubby", "sidecar"])]
psc: Option<u8>,
}

#[derive(Clone, Debug, clap::Args)]
#[clap(group(clap::ArgGroup::new("select")
.required(true)
.args(&["target", "cubby", "sidecar", "psc"]))
)]
pub struct IgnitionSingleSelector {
#[clap(flatten)]
sel: IgnitionSelector,
}

impl IgnitionSingleSelector {
fn get_target(&self, log: &Logger) -> Result<u8> {
// presence of at least one is enforced by clap
Ok(self.sel.get_target(log)?.unwrap())
}
}

impl IgnitionSelector {
/// Decodes CLI arguments to an ignition target
fn get_target(&self, log: &Logger) -> Result<Option<u8>> {
// At this point, we assume that the various `Option` values are
// mutually exclusive (enforced by `clap`).
let t = if let Some(t) = self.target {
t
} else if let Some(c) = self.cubby {
// See RFD 144 § 6.1 (Switch Port Map) for this mapping
let t = match c {
0 => 14,
1 => 30,
2 => 15,
3 => 31,
4 => 13,
5 => 29,
6 => 12,
7 => 28,
8 => 10,
9 => 26,
10 => 11,
11 => 27,
12 => 9,
13 => 25,
14 => 8,
15 => 24,
16 => 4,
17 => 20,
18 => 5,
19 => 21,
20 => 7,
22 => 6,
24 => 0,
25 => 16,
26 => 1,
27 => 17,
28 => 3,
29 => 19,
30 => 2,
31 => 18,
i => bail!("cubby must be in the range 0-31, not {i}"),
};
debug!(log, "decoded cubby {c} => target {t}");
t
} else if let Some(s) = self.sidecar {
let t = match s {
SidecarSelector::Peer => 34,
SidecarSelector::Local => 35,
};
debug!(log, "decoded {s:?} => target {t}");
t
} else if let Some(p) = self.psc {
let t = match p {
0 => 32,
1 => 33,
i => bail!("psc must be in the range 0-1, not {i}"),
};
debug!(log, "decoded psc {p} => target {t}");
t
} else {
return Ok(None);
};
Ok(Some(t))
}
}

/// `IgnitionSelector` that also supports `--all`
#[derive(Clone, Debug, clap::Args)]
#[clap(group(clap::ArgGroup::new("select")
.required(true)
.args(&["target", "cubby", "sidecar", "psc", "all"]))
)]
pub struct IgnitionBulkSelector {
#[clap(flatten)]
Copy link
Collaborator

@jgallagher jgallagher Oct 28, 2025

Choose a reason for hiding this comment

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

Does this preclude having a conficts_with_all = ["all"]? Could we get that effect by making IgnitionBulkSelector an enum, or would that force us to have a subcommand to specify the variant?

Copy link
Contributor Author

@mkeeter mkeeter Oct 28, 2025

Choose a reason for hiding this comment

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

I switched to using argument groups, so you get exactly-one semantics:

Get the ignition state for a port (only valid if the SP is an ignition controller)

Usage: faux-mgs --interface <INTERFACE> ignition <--target <TARGET>|--cubby <CUBBY>|--sidecar <SIDECAR>|--psc <PSC>|--all|COMPAT>

Arguments:
  [COMPAT]  'all' or a target number (backwards-compatibility shim)

Options:
  -t, --target <TARGET>    Ignition target
  -c, --cubby <CUBBY>      Sled cubby (0-31)
  -s, --sidecar <SIDECAR>  Sidecar ('local' or 'peer')
  -p, --psc <PSC>          PSC index (0-1)
  -a, --all                All targets
  -h, --help               Print help

sel: IgnitionSelector,

/// All targets
#[clap(short, long)]
all: bool,
}

impl IgnitionBulkSelector {
fn get_targets(&self, log: &Logger) -> Result<IgnitionBulkTargets> {
// Delegate to the fancier representation
IgnitionBulkSelectorWithCompatibilityShim {
sel: self.sel,
all: self.all,
compat: None,
}
.get_targets(log)
}
}

/// `IgnitionBulkSelector` that also supports a positional argument
///
/// This is for backwards compatibility. However, it can't be used in
/// combination with later positional arguments, because optional positional
/// arguments are only allowed at the end of an argument list.
#[derive(Clone, Debug, clap::Args)]
#[clap(group(clap::ArgGroup::new("select")
.required(true)
.args(&["target", "cubby", "sidecar", "psc", "all", "compat"]))
)]
pub struct IgnitionBulkSelectorWithCompatibilityShim {
#[clap(flatten)]
sel: IgnitionSelector,

/// All targets
#[clap(short, long)]
all: bool,

/// 'all' or a target number
/// (deprecated, present for backwards-compatibility)
#[clap(value_parser = parse_bulk_targets_shim)]
compat: Option<IgnitionBulkTargets>,
}

impl IgnitionBulkSelectorWithCompatibilityShim {
fn get_targets(&self, log: &Logger) -> Result<IgnitionBulkTargets> {
match (self.sel.get_target(log)?, self.all, self.compat) {
(Some(_), true, _) | (_, true, Some(_)) => {
bail!("cannot specify a target and `--all`")
}
(Some(_), _, Some(_)) => {
bail!(
"cannot specify a target with both flags \
and positional arguments"
)
}
(Some(target), false, None) => {
Ok(IgnitionBulkTargets::Single(target))
}
(None, false, Some(t)) => {
match t {
IgnitionBulkTargets::All => warn!(
log,
"positional `all` argument is deprecated, \
please switch to `--all`"
),
IgnitionBulkTargets::Single(..) => warn!(
log,
"positional target argument is deprecated, \
please switch to `--target`, `--cubby`, \
`--sidecar`, or `--psc`"
),
}
Ok(t)
}
(None, true, None) => Ok(IgnitionBulkTargets::All),
(None, false, None) => {
// enforced by clap
panic!("must specify either a target or `--all`")
}
}
}
}

#[derive(Copy, Clone, Debug)]
enum IgnitionBulkTargets {
Single(u8),
All,
}

#[derive(Copy, Clone, Debug)]
enum SidecarSelector {
Local,
Peer,
}

fn parse_sidecar_selector(s: &str) -> Result<SidecarSelector> {
let out = match s.to_ascii_lowercase().as_str() {
"local" => SidecarSelector::Local,
"peer" => SidecarSelector::Peer,
_ => bail!("invalid sidecar selector '{s}', must be 'local' or 'peer'"),
};
Ok(out)
}

impl Command {
// If the user didn't specify a listening port, what should we use? We
// allow this to vary by command so that client commands (most of them) can
Expand Down Expand Up @@ -674,19 +887,14 @@ fn parse_sp_component(component: &str) -> Result<SpComponent> {
.map_err(|_| anyhow!("invalid component name: {component}"))
}

#[derive(Debug, Clone, Copy)]
struct IgnitionLinkEventsTarget(Option<u8>);

impl IgnitionLinkEventsTarget {
fn parse(s: &str) -> Result<Self> {
match s {
"all" | "ALL" => Ok(Self(None)),
_ => {
let target = s
.parse()
.with_context(|| "must be an integer (0..256) or 'all'")?;
Ok(Self(Some(target)))
}
fn parse_bulk_targets_shim(s: &str) -> Result<IgnitionBulkTargets> {
match s {
"all" | "ALL" => Ok(IgnitionBulkTargets::All),
_ => {
let target = s
.parse()
.with_context(|| "must be an integer (0..256) or 'all'")?;
Ok(IgnitionBulkTargets::Single(target))
}
}
}
Expand Down Expand Up @@ -1212,17 +1420,20 @@ async fn run_command(
Ok(Output::Lines(lines))
}
}
Command::Ignition { target } => {
Command::Ignition { sel } => {
let mut by_target = BTreeMap::new();
if let Some(target) = target.0 {
let state = sp.ignition_state(target).await?;
by_target.insert(usize::from(target), state);
} else {
let states = sp.bulk_ignition_state().await?;
for (i, state) in states.into_iter().enumerate() {
by_target.insert(i, state);
match sel.get_targets(&log)? {
IgnitionBulkTargets::Single(target) => {
let state = sp.ignition_state(target).await?;
by_target.insert(usize::from(target), state);
}
}
IgnitionBulkTargets::All => {
let states = sp.bulk_ignition_state().await?;
for (i, state) in states.into_iter().enumerate() {
by_target.insert(i, state);
}
}
};
if json {
Ok(Output::Json(serde_json::to_value(by_target).unwrap()))
} else {
Expand All @@ -1233,7 +1444,8 @@ async fn run_command(
Ok(Output::Lines(lines))
}
}
Command::IgnitionCommand { target, command } => {
Command::IgnitionCommand { sel, command } => {
let target = sel.get_target(&log)?;
sp.ignition_command(target, command).await?;
info!(log, "ignition command {command:?} send to target {target}");
if json {
Expand All @@ -1244,15 +1456,18 @@ async fn run_command(
)]))
}
}
Command::IgnitionLinkEvents { target } => {
Command::IgnitionLinkEvents { sel } => {
let mut by_target = BTreeMap::new();
if let Some(target) = target.0 {
let events = sp.ignition_link_events(target).await?;
by_target.insert(usize::from(target), events);
} else {
let events = sp.bulk_ignition_link_events().await?;
for (i, events) in events.into_iter().enumerate() {
by_target.insert(i, events);
match sel.get_targets(&log)? {
IgnitionBulkTargets::Single(target) => {
let events = sp.ignition_link_events(target).await?;
by_target.insert(usize::from(target), events);
}
IgnitionBulkTargets::All => {
let events = sp.bulk_ignition_link_events().await?;
for (i, events) in events.into_iter().enumerate() {
by_target.insert(i, events);
}
}
}
if json {
Expand All @@ -1265,9 +1480,12 @@ async fn run_command(
Ok(Output::Lines(lines))
}
}
Command::ClearIgnitionLinkEvents { target, transceiver_select } => {
sp.clear_ignition_link_events(target.0, transceiver_select.0)
.await?;
Command::ClearIgnitionLinkEvents { sel, transceiver_select } => {
let target = match sel.get_targets(&log)? {
IgnitionBulkTargets::Single(t) => Some(t),
IgnitionBulkTargets::All => None,
};
sp.clear_ignition_link_events(target, transceiver_select.0).await?;
info!(log, "ignition link events cleared");
if json {
Ok(Output::Json(json!({ "ack": "cleared" })))
Expand Down
Loading