Add SQS to mirrord operator status reporting. #2928
Conversation
| OperatorCommand::Status { config_file } => operator_status(config_file.as_deref()).await, | ||
| OperatorCommand::Status { config_file } => { | ||
| StatusCommandHandler::new(config_file) | ||
| .and_then(StatusCommandHandler::handle) | ||
| .await | ||
| } |
There was a problem hiding this comment.
Why is creating a one-time object and calling its handle method better than the simple function call we currently have?
To me that seems slightly more complicated, and I don't see the advantages, but I'm asking for help understanding, not for changes.
There was a problem hiding this comment.
When moving code around and also changing it, please try to separate the moving from the changing and have them in different commits, otherwise it's pretty hard to review. There are 225 new lines here, but only a few of them are actually new or changed, and it's very complicated to find out which ones, in order to understand what actually changed in this code.
| .inspect_err(|_| { | ||
| status_progress.failure(Some("failed to get status")); | ||
| })? | ||
| .ok_or(CliError::OperatorNotInstalled)?; |
There was a problem hiding this comment.
In the old code we also call
status_progress.failure(Some("operator not found"));in this case. Did you remove that on purpose?
| .. | ||
| }, | ||
| .. | ||
| } = api.operator().spec.clone(); |
There was a problem hiding this comment.
new clone introduced here - fine if it's on purpose.
| let mut reporter = NullReporter::default(); | ||
| let api = api.prepare_client_cert(&mut reporter).await; | ||
| api.inspect_cert_error( | ||
| |error| tracing::error!(%error, "failed to prepare client certificate"), | ||
| ); | ||
|
|
||
| let seeker = KubeResourceSeeker { | ||
| client: api.client(), | ||
| namespace: Some(default_namespace.as_str()), | ||
| }; |
There was a problem hiding this comment.
This seems unnecessarily complicated.
Also, you're only listing resources from the default namespace and I don't think that's what we want.
Maybe try something like this:
let registry_api = Api::<MirrordWorkloadQueueRegistry>::all(client.clone());
let list = registry_api
.list(&ListParams::default())
.await?;BTW, I'm still not sure we want to show the Registries' status and not the sessions. @aviramha might have an opinion there.
Currently the SQS session resources have finalizers and the registries do not (I think when we have time we should add finalizers for them too, and reconcile them etc.) so the session resources are more truthful.
There was a problem hiding this comment.
We want to see active split sessions
| .await | ||
| .map_err(OperatorApiError::from)? | ||
| .into_iter() | ||
| .filter_map(|sqs| { |
There was a problem hiding this comment.
sqs is not a good name here, would go with registry
| let names = details | ||
| .output_queue_names() | ||
| .into_iter() | ||
| .map(|name| row![name, &consumer]); |
There was a problem hiding this comment.
would also show the original queue name and the queue-id.
Also, it probably makes more sense to have 1 table per registry.
We could even go with 1 table per queue-id.
Just to clarify:
A registry contains many queues.
For each queue there is 1 queue-id.
Each session creates a new temporary queue for each original queue, and that temporary queue's name is called output queue in this status.
Adds SQS to
mirrord operator statusby fetchingMirrordWorkloadQueueRegistryresources.