ProcMan: dedicated gRPC message types#501
Merged
PawelPlesniak merged 32 commits intodevelopfrom Aug 5, 2025
Merged
Conversation
jamesturner246
requested review from
PawelPlesniak,
ccaveayland,
Copilot and
wanyunSu
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR removes generic Any-packed messages from the ProcessManager gRPC interface in favor of dedicated message types. The change simplifies the gRPC communication by using strongly-typed messages directly instead of wrapping them in generic Any containers.
Key changes:
- Updated ProcessManager gRPC service methods to use dedicated message types instead of Any-packed payloads
- Refactored driver classes to directly handle typed gRPC requests and responses
- Modified return types to consistently use ProcessInstanceList for batch operations
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/drunc/utils/shell_utils.py | Removed aio_channel support and simplified response handling |
| src/drunc/utils/grpc_utils.py | Added handle_grpc_error and copy_token utility functions |
| src/drunc/unified_shell/shell.py | Updated to handle typed responses without data wrapping |
| src/drunc/unified_shell/commands.py | Modified to access values directly from ProcessInstanceList |
| src/drunc/session_manager/session_manager_driver.py | Refactored to use dedicated message types and direct gRPC calls |
| src/drunc/session_manager/session_manager.py | Removed session field from Description response |
| src/drunc/process_manager/ssh_process_manager.py | Updated to return properly structured ProcessInstanceList responses |
| src/drunc/process_manager/process_manager_driver.py | Complete refactoring to use typed gRPC requests/responses |
| src/drunc/process_manager/process_manager.py | Removed Any message unpacking and simplified service methods |
| src/drunc/process_manager/k8s_process_manager.py | Updated to return ProcessInstanceList consistently |
| src/drunc/process_manager/interface/shell.py | Modified to access Description fields directly |
| src/drunc/process_manager/interface/context.py | Disabled aio_channel usage |
| src/drunc/process_manager/interface/commands.py | Updated to handle ProcessInstanceList responses |
| src/drunc/controller/controller_driver.py | Simplified constructor and removed create_stub method |
Contributor
|
Some PM commands do not work in unified shell, ie:
I am using ssh-standalone and local-1x1-config |
Contributor
Author
Thanks @wanyunSu. Good spot. Should be fixed now. |
Contributor
|
Thanks, PM commands work now, but sequence commands do not work:
|
Contributor
Author
|
Cheers again. A sneaky one. Fixed. Might be worth us having a chat to discuss your testing process. Seems mine is a bit lacking! ED: Oh, and the accompanying druncschema change: DUNE-DAQ/druncschema#59 |
Contributor
|
LGTM. My testing process is simply to execute the Process manager commands and FSM commands, using both the local and ehn1 configurations, to ensure that all commands behave as expected. |
wanyunSu
approved these changes
Jul 31, 2025
Collaborator
|
@wanyunSu @jamesturner246 has the testing finished succesfully? |
Contributor
Author
|
All good my end. |
Contributor
|
LGTM |
Collaborator
|
I ran the MSQT which passed, but when I run the unified shell manually there are some issues. Using the following commands I see issues: Before this gets merged, I think it would be useful to define additional tests - the basic tests do not cover all the bases |
Collaborator
|
The reported issues were found to be independent of this PR, they are being investigated now |
Collaborator
|
Thanks @jamesturner246 |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Resolves #495.
This is the second in a series of changes replacing the generic Any-packed messages with dedicated ones. In this change, the
ProcessManageris addressed. Took a bit longer than expected -- I wanted to be sure that I wasn't accidentally leaving something out. Might need a quick chat to step through the changes.I am working on separating the 'drivers' out into separate classes, since caveats in each driver type make subclassing more messier than not. Even the gRPC stub classes are not covariant.
NB: As for which messages, I just used existing ones for now, and left deciding on a proper set of messages for later. Token can be omitted from these messages as we can pass this information in via metadata, as intended.