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

Fixes improves OPTE installation and improves errors #1052

Merged
merged 8 commits into from
May 10, 2022
Merged

Conversation

bnaecker
Copy link
Collaborator

  • Fixes mismerge in tools/install_opte.sh that prevented installing
    OPTE package on a system that didn't previously have it
  • Improves error messages when either xde driver fails or the expected
    virtual networking devices don't exist

- Fixes mismerge in `tools/install_opte.sh` that prevented installing
  OPTE package on a system that didn't previously have it
- Improves error messages when either xde driver fails or the expected
  virtual networking devices don't exist
@bnaecker bnaecker requested a review from smklein May 10, 2022 03:47
@bnaecker
Copy link
Collaborator Author

Should resolve #1049

If we try to run the sled agent without the expected xde driver config file at /kernel/drv/xde.conf, we get errors like:

May 10 03:02:16.766 WARN failed to start sled agent, error: Error Response, component: RSS, server: fb0f7546-4d46-40ca-9d56-cbb810684ca7, component: BootstrapAgent
May 10 03:02:18.027note: configured to log to "/var/oxide/sled-agent.log"
 INFO Loading Sled Agent: SledAgentRequest { subnet: Ipv6Subnet { net: Ipv6Net(Ipv6Network { addr: fd00:1122:3344:101::, prefix: 64 }) } }, server: fb0f7546-4d46-40ca-9d56-cbb810684ca7, component: BootstrapAgent
May 10 03:02:18.064 INFO request completed, error_message_external: Internal Server Error, error_message_internal: Error starting sled agent: Could not start sled agent server: Error managing guest networking: No xde driver configuration file exists at '/kernel/drv/xde.conf', response_code: 500, uri: /start_sled, method: PUT, req_id: 7cd69b26-002c-4ff2-8bc9-16224f39a543, remote_addr: [fdb0:b42e:99fe:859::1]:50098, local_addr: [fdb0:b42e:99fe:859::1]:12346, component: dropshot

If we've not run ./tools/create_virtual_hardware.sh, and in particular, we don't have exactly two VNICs named net{0,1}, we get errors like this:

 INFO Loading Sled Agent: SledAgentRequest { subnet: Ipv6Subnet { net: Ipv6Net(Ipv6Network { addr: fd00:1122:3344:101::, prefix: 64 }) } }, server: fb0f7546-4d46-40ca-9d56-cbb810684ca7, component: BootstrapAgent
May 10 03:04:29.038 INFO request completed, error_message_external: Internal Server Error, error_message_internal: Error starting sled agent: Could not start sled agent server: Error managing guest networking: Failure interacting with the OPTE ioctl(2) interface: invalid argument There must be at least two underlay NICs for the xde driver to operate. These are currently created by `./tools/create_virtual_hadware.sh`. Please ensure that script has been run, and that two VNICs named `net{0,1}` exist on the system., response_code: 500, uri: /start_sled, method: PUT, req_id: 94c7a01b-2df0-4ebd-baaf-b4269d1a2340, remote_addr: [fdb0:b42e:99fe:859::1]:58929, local_addr: [fdb0:b42e:99fe:859::1]:12346, component: dropshot
May 10 03:04:29.039 WARN failed to start sled agent, error: Error Response, component: RSS, server: fb0f7546-4d46-40ca-9d56-cbb810684ca7, component: BootstrapAgent

Copy link
Collaborator

@smklein smklein left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the quick fixes!

@bnaecker
Copy link
Collaborator Author

Something actually occurred to me after posting this. This expands the set of VNICs returned by Dladm::get_vnics() to include the underlay devices net{0,1} and any VNICs for guests over xde devices. The sled agent currently deletes those with the ox prefix here, but this feels a bit dangerous. Without that .filter() call, we'd delete the guest VNICs and the underlay VNICs. Any thoughts on whether we should make that more safe? Maybe split things into a Dladm::guest_vnics() and Dladm::system_vnics() call? Or provide the prefix as an argument to Dladm::get_vnics()?

Any thoughts here @smklein?

@smklein
Copy link
Collaborator

smklein commented May 10, 2022

Something actually occurred to me after posting this. This expands the set of VNICs returned by Dladm::get_vnics() to include the underlay devices net{0,1} and any VNICs for guests over xde devices. The sled agent currently deletes those with the ox prefix here, but this feels a bit dangerous. Without that .filter() call, we'd delete the guest VNICs and the underlay VNICs. Any thoughts on whether we should make that more safe? Maybe split things into a Dladm::guest_vnics() and Dladm::system_vnics() call? Or provide the prefix as an argument to Dladm::get_vnics()?

Any thoughts here @smklein?

Fortunately, that's the only spot where we actually query for all sled-agent managed VNICs, so this change should be easy.

The implementation (on main) is a little redundant - get_vnics() filters by prefix...

/// Returns all VNICs that may be managed by the Sled Agent.
pub fn get_vnics() -> Result<Vec<String>, GetVnicError> {
let mut command = std::process::Command::new(PFEXEC);
let cmd = command.args(&[DLADM, "show-vnic", "-p", "-o", "LINK"]);
let output = execute(cmd).map_err(|err| GetVnicError { err })?;
let vnics = String::from_utf8_lossy(&output.stdout)
.lines()
.filter(|vnic| vnic.starts_with(VNIC_PREFIX))
.map(|s| s.to_owned())
.collect();
Ok(vnics)
}

... but then we filter again in sled_agent.rs, after making that call:

let vnics = Dladm::get_vnics()?;
for vnic in vnics
.iter()
.filter(|vnic| vnic.starts_with(crate::illumos::dladm::VNIC_PREFIX))
{
warn!(log, "Deleting VNIC: {}", vnic);
Dladm::delete_vnic(&vnic)?;
}

Before OPTE integration, "get_vnics" was fairly unambiguous - we only managed guest vnics. Now that we have more complex categories, you're right, it's probably worthwhile disambiguating.

I'd be fine with either making a Dladm::underlay_vnics function, or with passing an enum specifying the flavor of VNIC to return - as long as it's strongly typed (lets avoid boolean arguments), those seem equivalent to me.

@bnaecker
Copy link
Collaborator Author

Cool, will add shortly. Thanks!

- Adds a `VnicKind` enum for tracking the flavor of each VNIC the sled
  agent is responsible for.
- Adds parameter to the `Dladm::get_vnics()` call which filters the
  returned list to a particular kind. The goal here is to be more
  explicit about which VNICs we're looking for and ultimately operating
  on in the sled agent.
- The sled agent now cleans up guest VNICs and the underlying xde
  devices (OPTE ports) when it starts up, similar to how it clears out
  any extant control VNICs, to ensure things are in a reliable state
  before accepting any requests from Nexus
- Improves the `tools/install_opte.sh` script, trying to be less
  intrusive and only modifying the state we need to change when adding
  the OPTE / xde package repositories.
@bnaecker
Copy link
Collaborator Author

@smklein This could use another once-over, if you don't mind. I've added an enum to represent the kind of each VNIC, and use that explicitly when clearing out either Oxide control VNICs or guest VNICs when the sled agent starts up. I'm also clearing out the OPTE ports / xde devices at startup, which should resolve #1048 as well.

@bnaecker bnaecker requested a review from smklein May 10, 2022 21:08
@bnaecker
Copy link
Collaborator Author

Ok, a few more straggler changes required. I opted to return an Option when trying to get the VnicKind for a VNIC that the sled agent should not be managing, e.g., net0. This means we have to also return a Result from things like Vnic::wrap_existing, since that just takes a name as a &str at this point. Hopefully this resolves the above @smklein.

As a sidenote, here's a snippet from the sled agent logs, when restarting and cleaning up the old state:

{"msg":"Deleting existing VNIC","v":0,"name":"sled-agent","level":40,"time":"2022-05-10T20:53:13.921022577Z","hostname":"feldspar","pid":834,"sled_id":"fb0f7546-4d46-40ca-9d56-cbb810684ca7","component":"SledAgent","kind":"OxideControl","name":"oxControlStorage0"}
{"msg":"Deleting existing VNIC","v":0,"name":"sled-agent","level":40,"time":"2022-05-10T20:53:13.926981394Z","hostname":"feldspar","pid":834,"sled_id":"fb0f7546-4d46-40ca-9d56-cbb810684ca7","component":"SledAgent","kind":"OxideControl","name":"oxControlStorage1"}
{"msg":"Deleting existing VNIC","v":0,"name":"sled-agent","level":40,"time":"2022-05-10T20:53:13.93262576Z","hostname":"feldspar","pid":834,"sled_id":"fb0f7546-4d46-40ca-9d56-cbb810684ca7","component":"SledAgent","kind":"OxideControl","name":"oxControlStorage2"}
{"msg":"Deleting existing VNIC","v":0,"name":"sled-agent","level":40,"time":"2022-05-10T20:53:13.937900856Z","hostname":"feldspar","pid":834,"sled_id":"fb0f7546-4d46-40ca-9d56-cbb810684ca7","component":"SledAgent","kind":"OxideControl","name":"oxControlStorage3"}
{"msg":"Deleting existing VNIC","v":0,"name":"sled-agent","level":40,"time":"2022-05-10T20:53:13.943852525Z","hostname":"feldspar","pid":834,"sled_id":"fb0f7546-4d46-40ca-9d56-cbb810684ca7","component":"SledAgent","kind":"OxideControl","name":"oxControlService0"}
{"msg":"Deleting existing VNIC","v":0,"name":"sled-agent","level":40,"time":"2022-05-10T20:53:13.948241326Z","hostname":"feldspar","pid":834,"sled_id":"fb0f7546-4d46-40ca-9d56-cbb810684ca7","component":"SledAgent","kind":"OxideControl","name":"oxControlStorage4"}
{"msg":"Deleting existing VNIC","v":0,"name":"sled-agent","level":40,"time":"2022-05-10T20:53:13.952736083Z","hostname":"feldspar","pid":834,"sled_id":"fb0f7546-4d46-40ca-9d56-cbb810684ca7","component":"SledAgent","kind":"OxideControl","name":"oxControlService1"}
{"msg":"Deleting existing VNIC","v":0,"name":"sled-agent","level":40,"time":"2022-05-10T20:53:13.958774583Z","hostname":"feldspar","pid":834,"sled_id":"fb0f7546-4d46-40ca-9d56-cbb810684ca7","component":"SledAgent","kind":"OxideControl","name":"oxControlService2"}
{"msg":"Deleting existing VNIC","v":0,"name":"sled-agent","level":40,"time":"2022-05-10T20:53:13.9637238Z","hostname":"feldspar","pid":834,"sled_id":"fb0f7546-4d46-40ca-9d56-cbb810684ca7","component":"SledAgent","kind":"OxideControl","name":"oxControlInstance0"}
{"msg":"Deleting existing VNIC","v":0,"name":"sled-agent","level":40,"time":"2022-05-10T20:53:13.968212689Z","hostname":"feldspar","pid":834,"sled_id":"fb0f7546-4d46-40ca-9d56-cbb810684ca7","component":"SledAgent","kind":"OxideControl","name":"oxControlInstance1"}
{"msg":"Deleting existing VNIC","v":0,"name":"sled-agent","level":40,"time":"2022-05-10T20:53:13.973246358Z","hostname":"feldspar","pid":834,"sled_id":"fb0f7546-4d46-40ca-9d56-cbb810684ca7","component":"SledAgent","kind":"Guest","name":"vopte0"}
{"msg":"Deleting existing VNIC","v":0,"name":"sled-agent","level":40,"time":"2022-05-10T20:53:13.97776523Z","hostname":"feldspar","pid":834,"sled_id":"fb0f7546-4d46-40ca-9d56-cbb810684ca7","component":"SledAgent","kind":"Guest","name":"vopte1"}
{"msg":"deleting existing OPTE port and xde device","v":0,"name":"sled-agent","level":30,"time":"2022-05-10T20:53:13.982193032Z","hostname":"feldspar","pid":834,"sled_id":"fb0f7546-4d46-40ca-9d56-cbb810684ca7","component":"SledAgent","name":"opte0"}
{"msg":"deleting existing OPTE port and xde device","v":0,"name":"sled-agent","level":30,"time":"2022-05-10T20:53:13.982635276Z","hostname":"feldspar","pid":834,"sled_id":"fb0f7546-4d46-40ca-9d56-cbb810684ca7","component":"SledAgent","name":"opte1”}

Copy link
Collaborator

@smklein smklein left a comment

Choose a reason for hiding this comment

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

Mod my last (nit) comment, this still LGTM!

@bnaecker bnaecker enabled auto-merge (squash) May 10, 2022 22:22
@bnaecker bnaecker merged commit ce3102b into main May 10, 2022
@bnaecker bnaecker deleted the fix-opte-install branch May 10, 2022 23:15
leftwo pushed a commit that referenced this pull request Jan 10, 2024
Propolis changes since the last update:
Gripe when using non-raw block device
Update zerocopy dependency
nvme: Wire up GetFeatures command
Make Viona more robust in the face of errors
bump softnpu (#577)
Modernize 16550 UART

Crucible changes since the last update:
Don't check ROP if the scrub is done (#1093)
Allow crutest cli to be quiet on generic test (#1070)
Offload write encryption (#1066)
Simplify handling of BlockReq at program exit (#1085)
Update Rust crate byte-unit to v5 (#1054)
Remove unused fields in match statements, downstairs edition (#1084)
Remove unused fields in match statements and consolidate (#1083)
Add logger to Guest (#1082)
Drive hash / decrypt tests from Upstairs::apply
Wait to reconnect if auto_promote is false
Change guest work id from u64 -> GuestWorkId
remove BlockOp::Commit (#1072)
Various clippy fixes (#1071)
Don't panic if tasks are destroyed out of order
Update Rust crate reedline to 0.27.1 (#1074)
Update Rust crate async-trait to 0.1.75 (#1073)
Buffer should destructure to Vec when single-referenced
Don't fail to make unencrypted regions (#1067)
Fix shadowing in downstairs (#1063)
Single-task refactoring (#1058)
Update Rust crate tokio to 1.35 (#1052)
Update Rust crate openapiv3 to 2.0.0 (#1050)
Update Rust crate libc to 0.2.151 (#1049)
Update Rust crate rusqlite to 0.30 (#1035)
leftwo added a commit that referenced this pull request Jan 11, 2024
Propolis changes since the last update:
Gripe when using non-raw block device
Update zerocopy dependency
nvme: Wire up GetFeatures command
Make Viona more robust in the face of errors
bump softnpu (#577)
Modernize 16550 UART

Crucible changes since the last update:
Don't check ROP if the scrub is done (#1093)
Allow crutest cli to be quiet on generic test (#1070)
Offload write encryption (#1066)
Simplify handling of BlockReq at program exit (#1085)
Update Rust crate byte-unit to v5 (#1054)
Remove unused fields in match statements, downstairs edition (#1084)
Remove unused fields in match statements and consolidate (#1083)
Add logger to Guest (#1082)
Drive hash / decrypt tests from Upstairs::apply
Wait to reconnect if auto_promote is false
Change guest work id from u64 -> GuestWorkId
remove BlockOp::Commit (#1072)
Various clippy fixes (#1071)
Don't panic if tasks are destroyed out of order
Update Rust crate reedline to 0.27.1 (#1074)
Update Rust crate async-trait to 0.1.75 (#1073)
Buffer should destructure to Vec when single-referenced
Don't fail to make unencrypted regions (#1067)
Fix shadowing in downstairs (#1063)
Single-task refactoring (#1058)
Update Rust crate tokio to 1.35 (#1052)
Update Rust crate openapiv3 to 2.0.0 (#1050)
Update Rust crate libc to 0.2.151 (#1049)
Update Rust crate rusqlite to 0.30 (#1035)

---------

Co-authored-by: Alan Hanson <[email protected]>
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