[sled-agent] Add support for Support Bundle command execution #7011
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Example output from the two endpoints below. The goal here is to have nexus collect this raw data and dump it out to a file that will end up in the final support bundle. |
smklein
reviewed
Nov 7, 2024
Comment on lines
+790
to
+794
| .into_iter() | ||
| .map(|cmd| cmd.get_output()) | ||
| .collect::<Vec<_>>() | ||
| .as_slice() | ||
| .join("\n\n"); |
There was a problem hiding this comment.
Why are we joining these with \n\n when we aren't doing so for the zoneadm command?
There was a problem hiding this comment.
This is the spacing between multiple commands as the ipadm variant currently runs 3 commands. I can change it to a single new line if you would like, I just thought giving the output some space was easier on the eyes.
There was a problem hiding this comment.
Ah, gotcha, didn't realize the outputs were also different here (Vec of Results vs one Result). This seems fine!
sled-agent/src/support_bundle.rs
Outdated
Comment on lines
239
to
242
| let mut command = Command::new("sleep"); | ||
| command.env_clear().arg("10"); | ||
|
|
||
| match execute_command_with_timeout(command, Duration::from_secs(5)) |
There was a problem hiding this comment.
Nit: Can we use smaller timeouts here? just would be nice to shorten the latency of this test to < 5 seconds
Comment on lines
+250
to
+290
| #[tokio::test] | ||
| async fn test_command_stdout_is_correct() { | ||
| let mut command = Command::new("echo"); | ||
| command.env_clear().arg("No Cables. No Assembly. Just Cloud."); | ||
|
|
||
| let res = execute_command_with_timeout(command, Duration::from_secs(5)) | ||
| .await | ||
| .unwrap(); | ||
| let expected_output = | ||
| "No Cables. No Assembly. Just Cloud.\n".to_string(); | ||
| assert_eq!(expected_output, res.stdio); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_command_stderr_is_correct() { | ||
| let mut command = Command::new("bash"); | ||
| command.env_clear().args(&["-c", "echo oxide computer > /dev/stderr"]); | ||
|
|
||
| let res = execute_command_with_timeout(command, Duration::from_secs(5)) | ||
| .await | ||
| .unwrap(); | ||
| let expected_output = "oxide computer\n".to_string(); | ||
| assert_eq!(expected_output, res.stdio); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_command_stdout_stderr_are_interleaved() { | ||
| let mut command = Command::new("bash"); | ||
| command.env_clear().args(&[ | ||
| "-c", | ||
| "echo one > /dev/stdout \ | ||
| && echo two > /dev/stderr \ | ||
| && echo three > /dev/stdout", | ||
| ]); | ||
|
|
||
| let res = execute_command_with_timeout(command, Duration::from_secs(5)) | ||
| .await | ||
| .unwrap(); | ||
| let expected_output = "one\ntwo\nthree\n".to_string(); | ||
| assert_eq!(expected_output, res.stdio); | ||
| } |
smklein
approved these changes
Nov 8, 2024
hawkw
reviewed
Nov 8, 2024
sled-agent/src/support_bundle.rs
Outdated
| } | ||
|
|
||
| /// Takes a given `Command` and returns a lossy representation of the program | ||
| // and it's arguments. |
There was a problem hiding this comment.
teensy obnoxious copyedit: "it's" is an abbreviation for "it is", while "its" means "belonging to it)
Suggested change
| // and it's arguments. | |
| // and its arguments. |
There was a problem hiding this comment.
Fixed in c3b5c52
sled-agent/src/support_bundle.rs
Outdated
Comment on lines
61
to
69
| // Grab the command arguments | ||
| let mut res = command | ||
| .get_args() | ||
| .map(|a| a.to_string_lossy().into()) | ||
| .collect::<Vec<String>>(); | ||
|
|
||
| // Grab the command itself | ||
| res.insert(0, command.get_program().to_string_lossy().into()); | ||
| res.join(" ") |
There was a problem hiding this comment.
nitpick: performance probably does not matter here, but we are doing this basically the least efficient way possible with regards to allocations:
to_string_lossy()returns aCow, because it allocates a newStringonly if it's invalid UTF8, which is unlikely. but calling callingInto<String>on thatCowforces an allocation unconditionally.- we
collectthose strings, allocating aVec. Vec::insertat 0 forces the wholeVecto be copied one index forward to make space for the inserted item --- if we instead usediter::chain()or something to just put the program name first, we wouldn't have to shift the entire contents of the rest of theVec's contentsVec::joinallocates a new string, so all the previous string allocations that we made during this function are now deallocated --- most of them didn't really need heap allocations
personally, i might consider doing something like this:
Suggested change
| // Grab the command arguments | |
| let mut res = command | |
| .get_args() | |
| .map(|a| a.to_string_lossy().into()) | |
| .collect::<Vec<String>>(); | |
| // Grab the command itself | |
| res.insert(0, command.get_program().to_string_lossy().into()); | |
| res.join(" ") | |
| // Grab the command itself | |
| let mut res = command.get_program().to_string_lossy().into(); | |
| // Grab the command arguments | |
| for arg in command.get_args() { | |
| let arg = arg.to_string_lossy(); | |
| write!(&mut res, " {arg}").expect("write! to strings never fails"); | |
| } | |
| res |
which is maybe less attractive than Vec::join, but doesn't create a temporary allocation for each argument and for a Vec that all end up getting thrown away --- instead, everything is written directly to the returned String.
There was a problem hiding this comment.
Thanks for the detailed explanation! The insert at offset 0 was going to cause everything to shift and I wasn't too worried about the allocations here but overall your suggestion is much nicer and more efficient!!
| cmd: Command, | ||
| oneshot: oneshot::Sender<()>, | ||
| ) -> Result<SupportBundleCmdOutput, SupportBundleCmdError> { | ||
| let cmd_string = command_to_string(&cmd); |
There was a problem hiding this comment.
nit, take it or leave it: AFAICT this is only ever called when formatting an error message, so perhaps we should call it lazily in the various map_err closures, instead of doing it unconditionally at the top of the function?
There was a problem hiding this comment.
I originally intended to do this but Command::from consumes the StdCommand, but in the end we store it in the SupportBundleCmdOutput struct anyways so it get's used regardless of an error.
sled-agent/src/support_bundle.rs
Outdated
| res.join(" ") | ||
| } | ||
|
|
||
| /// Spawn a command asynchronously and collect it's output by interleaving |
There was a problem hiding this comment.
Suggested change
| /// Spawn a command asynchronously and collect it's output by interleaving | |
| /// Spawn a command asynchronously and collect its output by interleaving |
There was a problem hiding this comment.
Fixed in c3b5c52
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR lays the groundwork for executing system commands and returning their output as raw data (both on success and failure) via the internal sled-agent HTTP API. Command execution collects both stdout and stdout in an interleaved fashion so that the command output is exactly how an operator would have seen it if they were sitting at the terminal.