feat: Refactor VSOCK - #11322
Conversation
ceacf18 to
6a179fa
Compare
| match response { | ||
| Ok(Payload::HostOSVsockVersion(version)) => println!("{version}"), | ||
| Ok(Payload::HostOSVersion(version)) => println!("{version}"), | ||
| Err(error) => bail!("Server responded with error: '{error}'"), |
There was a problem hiding this comment.
When printing anyhow errors. use {:?}, otherwise only the top error is printed, never the cause (in our case "sending command", nothing else)
There was a problem hiding this comment.
It's actually annoying to wire this up. Can I argue that this level is only transport errors, so the command itself matters less, and it's in the binary, so the command that was run should be more obvious?
| Ok(()) | ||
| } | ||
|
|
||
| // Adapted from rs/http_endpoints/async_utils/src/lib.rs |
There was a problem hiding this comment.
nit: Is it useful as a comment? If the don't have to be kept in sync, maybe we don't really need to know where it originated from (especially because it's a pretty common pattern).
There was a problem hiding this comment.
It feels like it might be nice to re-use the lib at some point, I just didn't want to pull in the logger. How about I turn this into a TODO with that note instead?
| } | ||
|
|
||
| // Skip logging to host if manual recovery TUI is running to avoid interfering with the display | ||
| if procfs::process::all_processes().is_ok_and(|processes| { |
There was a problem hiding this comment.
Uh, this is a bit ugly 🫣 (but no need to fix it now)
| use upgrade::{start_upgrade_guest_vm, upgrade_hostos}; | ||
|
|
||
| use vsock::{VMADDR_CID_ANY, VsockAddr, VsockListener, VsockStream}; | ||
| use ic_http_utils::file_downloader::FileDownloadError; |
There was a problem hiding this comment.
nit: any reason for splitting the imports into groups?
There was a problem hiding this comment.
I mostly follow the groups here, but maybe it's just a (bad) habit at this point 😬.
| Err(error) => { | ||
| println!("Error converting bytes to string: {error}"); | ||
| return Err(io::Error::new(io::ErrorKind::InvalidData, error)); | ||
| drop(self.listener); |
There was a problem hiding this comment.
Is it important to control the drop order? If so, could you add a comment why it's important to drop the listener first?
There was a problem hiding this comment.
It needs to be dropped so that the socket is closed & new tasks are not added to the tracker. Otherwise it might hang forever.
There was a problem hiding this comment.
Since the above loop no longer runs, no more tasks are added to the tracker. The only thing the Drop implementation does is closing the vsock socket fd and thereby rejecting connection requests right away (instead of making the kernel buffer them until the end of the method is reached when the listener is dropped anyway). I don't mind having it but if we actually use it as an optimization, it's worth mentioning it in a comment.
There was a problem hiding this comment.
I started basing this off ic-bn-lib so I'm sure it's a little overkill. Still, I've left it with a comment because a few of the timeouts are pretty long.
| } | ||
| .map_err(|e| { | ||
| let error = e.to_string(); | ||
| println!("{error}"); |
There was a problem hiding this comment.
Nit: this can lead to double logging. Usually it's recommended to do exactly one of logging or returning an error.
There was a problem hiding this comment.
Any errors from the command execution we don't return up the stack. Here we print for the server, and write the Response back to the client, so both sides can log. Is that fine?
| Ok(_) => {} // systemctl failed, fallthrough to error handling below | ||
| Err(err) => return Err(format!("Could not start {GUESTOS_UPGRADER_SERVICE}: {err}")), | ||
| }; | ||
| .output()?; |
There was a problem hiding this comment.
Consider mapping this to an UpgraderService error, too (and perhaps add some context string, like could not start upgrader service)
| .output(); | ||
|
|
||
| handle_command_output(command_output) | ||
| .output()?; |
| } | ||
|
|
||
| for handle in handles { | ||
| handle.await??; |
There was a problem hiding this comment.
Add error context here too?
| Err(error) => { | ||
| println!("Error converting bytes to string: {error}"); | ||
| return Err(io::Error::new(io::ErrorKind::InvalidData, error)); | ||
| drop(self.listener); |
There was a problem hiding this comment.
It needs to be dropped so that the socket is closed & new tasks are not added to the tracker. Otherwise it might hang forever.
| .take(MAX_MESSAGE_SIZE) | ||
| .read_to_end(&mut buffer), | ||
| ) | ||
| .await??; |
| // As a sanity check, we request that the sender adds its own CID to the message, and that CID must match the CID in the stream peer address. | ||
| // NOTE: The kernel vhost driver also enforces this. Any packet with a forged source is dropped. | ||
| fn verify_sender_cid(stream: &mut VsockStream, guest_cid: u32) -> Result<(), VsockServerError> { | ||
| if stream.peer_addr()?.cid() != guest_cid { |
| let command_output = std::process::Command::new(INSTALL_UPGRADE_FILE_PATH) | ||
| .arg(UPGRADE_FILE_PATH) | ||
| .output(); | ||
| .output()?; |
|
|
||
| let mut stream = VsockStream::connect_with_cid_port(VMADDR_CID_HOST, self.port) | ||
| .map_err(|e| e.to_string())?; | ||
| let mut stream = VsockStream::connect_with_cid_port(VMADDR_CID_HOST, self.port)?; |
There was a problem hiding this comment.
Context :) and below too
|
Overall, please add as much context to errors as possible - would ease debuggng in the future. |
3 participants
Restructure the VSOCK code to make it easier to test and build off of, for future use in the type 4.X nodes with more than one GuestOS.