-
Notifications
You must be signed in to change notification settings - Fork 235
Improvements to the process and network management commands #381
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
base: main
Are you sure you want to change the base?
Conversation
9e93b6c to
146e6c5
Compare
# Conflicts: # vminitd/Sources/vminitd/Server+GRPC.swift
7e66d78 to
8633fe6
Compare
| switch err.code { | ||
| case .invalidArgument: | ||
| throw GRPCStatus(code: .invalidArgument, message: "createProcess: failed to create process: \(err)") | ||
| default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we getting lower fidelity on these error codes? Since we are catching a ContainerizationError it's going to have the original code already as that actually happened. Throwing a new error causes us to lose a bit of information. Is there a reason to do this through this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are catching ContainerizationError to convert it to GRPCStatus. Without this conversion, all errors would become generic GRPC internal errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems reasonable to implement toGRPCStatus in ContainerizationError to reduce the amount of code for error conversion and to convert all possible errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see now. Many of the codes should map well from cz error to grpc status. You could do something like:
extension ContainerizationError {
/// Convert the ContainerizationError into a GRPCStatus result.
var status: GRPCStatus {
let code: GRPCStatus.Code = {
switch self.code {
case .notFound: .notFound
case .exists: .alreadyExists
case .unsupported: .unimplemented
case .unknown: .unknown
case .internalError: .internalError
case .interrupted: .unavailable
case .invalidState: .failedPrecondition
case .invalidArgument: .invalidArgument
case .timeout: .deadlineExceeded
default: .internalError
}
}()
return GRPCStatus(
code: code,
message: self.description,
cause: self
)
}
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach loses the operation context compared to what I've already implemented. However, we can handle more error codes.
32d6efb to
22d508b
Compare
Improves the process and network management commands.