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

MGS/sp-sim: Introduce location mapping #933

Merged
merged 6 commits into from
Apr 21, 2022
Merged

Conversation

jgallagher
Copy link
Contributor

This PR builds on #932 and is the second of three building toward a preliminary implementation of RFD 250-style SP and location discovery.

There are two relevant changes in this PR (plus some refactoring of error types):

  • The Ping/Pong messages are replaced with Discover/DiscoverResponse, the latter of which includes the port on which the SP received the Discover.
  • The LocationMap type is added (but not currently used - that will be in the next PR). This includes the tokio task responsible for sending Discover packets on all switch ports, collecting the responses, and using them to derive the current location based on the configuration. The configuration includes a very simple description that lets us implement RFD 250 section 4.4: it lists specific ports we want to ping, and what the responses on those ports mean in terms of which sidecar we're running on.

Copy link
Contributor

@ahl ahl left a comment

Choose a reason for hiding this comment

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

really great; very cool

Comment on lines 60 to 63
/// List of human-readable location names; the actual strings don't matter,
/// but they're used in log messages and to sync with the refined locations
/// contained in `determination`. For "rack v1" we could use something like
/// "sidecar-a"/"sidecar-b".
Copy link
Contributor

Choose a reason for hiding this comment

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

let's ask @rmustacc what the printing on the rack is going to say and match that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Robert pointed me to RFD 200 § 7.2, which refers to them as SWITCH 0/SWITCH 1 (if space permits) or SW0/SW1 (shorthand). I'm slightly inclined to stick with lowercase for log strings/config files, so maybe switch0/switch1?

Comment on lines 168 to 175
port_config.location.remove(&location).ok_or_else(|| {
StartupError::InvalidConfig {
reason: format!(
"port {} has no entry for determined location {:?}",
port.0, location,
),
}
})?;
Copy link
Contributor

Choose a reason for hiding this comment

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

might we want to see all such errors rather that just the first? Or perhaps log out all mismatches and return an error saying that the config was bad?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered doing some kind of full-config-file-validation up front; that seems better than failing here after we've already talked to some SPs. I'll do that and log all errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added config validation in 674ffc2; it's a little long/boilerplatey but I think that's probably fine in exchange for getting a complete list of errors.

Comment on lines 295 to 296
0 => determination.sp_port_0,
1 => determination.sp_port_1,
Copy link
Contributor

Choose a reason for hiding this comment

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

do I recall correctly that the KSZ numbers these 1 and 2? If that's the case we might want to stick with that to prevent confusion in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 7782fbc

@jgallagher jgallagher force-pushed the mgs-sp-discovery-prep1 branch from 3aa03c0 to a089bb4 Compare April 21, 2022 15:54
Base automatically changed from mgs-sp-discovery-prep1 to main April 21, 2022 16:55
@jgallagher jgallagher force-pushed the mgs-sp-discovery-prep2 branch from b098ac2 to 674ffc2 Compare April 21, 2022 20:28
@jgallagher jgallagher enabled auto-merge (squash) April 21, 2022 20:59
@jgallagher jgallagher merged commit 3f55d4b into main Apr 21, 2022
@jgallagher jgallagher deleted the mgs-sp-discovery-prep2 branch April 21, 2022 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants