Skip to content

Split shell improvements - #912

Merged
PawelPlesniak merged 26 commits into
PawelPlesniakEmuhammad/SplitShellFixingfrom
emmuhamm/split-shell-improvements
May 14, 2026
Merged

Split shell improvements#912
PawelPlesniak merged 26 commits into
PawelPlesniakEmuhammad/SplitShellFixingfrom
emmuhamm/split-shell-improvements

Conversation

@emmuhamm

@emmuhamm emmuhamm commented May 6, 2026

Copy link
Copy Markdown
Member

Description

Fixes #709
Fixes #910
Fixes #326
Fixes #904

This PR improves the split shell usage in drunc, see the above issues for details.

Main changes

1. Shell ID Support in Contexts

  • Standardized the attribute shell_id across the key shell context classes (ControllerContext, ProcessManagerContext, UnifiedShellContext). This allows the codebase and logs to always distinguish which shell user interaction is occurring through, improving auditability and clarity in message passing between shells and the process manager.

2. Logging and Operator Feedback

New Logging Features & Operator Visibility:

  • Introduced the new log_pm_cmd utility: Any explicit shell commands a user runs (plus explicit/overridden arguments) are communicated to the process manager, tagged with user/session/shell type. This allows messages to be sent and saved by the process managers.

    • this logs the command executed, arguments if any, sessions if any, and the shell context.

    • The logging mechanism is designed for rich, structured feedback: it logs the command executed, its arguments (but only those the user explicitly set), the session (if available), and the shell context.

    • See example log messages here, which should be self explanatory

      • emmuhamm sent ps via process_manager_shell; from np04-srv-028.cern.ch:52606
      • emmuhamm sent logs with arguments {'name': "('root-controller',)", 'session': "'emir-test-2'"} via process_manager_shell; from np04-srv-028.cern.ch:52606
      • emmuhamm sent logs with arguments {'name': "('mlt',)"} for session emir-test-1 via unified_shell; from np04-srv-028.cern.ch:41896
  • Both the process manager shell and unified shell now send explicit connect/disconnect messages to the process manager via the new gRPC send_msg endpoint.

    • Eg emmuhamm connected from unified shell; from np04-srv-028.cern.ch:41896
  • Added resolve_context_peer utility. This function parses gRPC peer strings, extracts and tries to converts raw IP addresses into hostnames. This means a networked command or interaction will now show something like np04-srv-028.cern.ch:41750 instead of a less informative ipv4:10.73.136.70:41750.

How it Works—gRPC Enhancements

  • The process manager’s gRPC service adds a new general-purpose send_msg endpoint, fully implemented for both SSH- and Kubernetes-backed managers. All connect, disconnect, and command logs are routed through this, ensuring comprehensive auditability. Failures in this endpoint do not block user actions but are logged for troubleshooting.
  • The internal proxy/driver API is also updated to pack and transmit messages as structured notifications, with robust handling for serialization errors and gRPC network issues.
  • See Add logging endpoint druncschema#85 for relevant changes

3. Improved Session and Database Path Logic

More Reliable File Resolution:

  • In oks_parser.py, when multiple possible configuration file matches are found in the DUNEDAQ_DB_PATH, the code now:
    • Preferentially selects the first file that is writable (i.e., one that's not on a read-only filesystem such as cvmfs).
    • Only falls back to a read-only file if there is no writable candidate.
    • Previously, the code used the first match—which could be a CVMFS or otherwise unwritable path—leading to confusing failures for users wanting to modify configs.

Connection/Config Defaults:

  • Where no framework is explicit in a configuration filepath (conf_file), the code now assumes and prepends oksconflibs:, ensuring that new users and scripts do not fail silently due to missing protocol specifiers.

4. Query Logic Corrections

  • In _match_processes_against_query, the logic for filtering processes against user-specified queries in the process manager has been corrected:
    • Before: Checks for UUID, name, session, and user were combined in a way that effectively worked with "OR" logic—meaning it would accept a process if any condition matched.
    • Now: All query selectors are combined as an "AND": a process must match all provided selectors (UUID, name, session, user) to be included.
    • This eliminates over-broad query matches and makes CLI/user queries much more precise and reliable.

5. CLI/UX Improvements and Refactors

  • CLI argument decorators for commands like kill, flush, logs, and restart have been unified to ensure consistent parameter usage and reduce duplicated boilerplate.
  • Added wait command for the process manager shell.
  • In the unified shell, a new session_injector automatically ensures the session context is injected into any relevant query, making script writing and interactive usage much simpler and less error-prone.

6. Backend and Test/Mock Improvements

  • All process managers (SSH and Kubernetes) now expose their type explicitly with a pm_type attribute and fully implement the send_msg interface for gRPC messaging.
  • The Python driver/proxy layer is updated to take advantage of this messaging, with robust serialization and error management.
  • Mock/fake process manager implementations for CI and unit tests have been updated to cover the new send_msg logic, improving test reliability.
  • Non-essential “critical” logging has been eliminated in both test and production code, leading to more actionable logs and cleaning up test/debug output.

Type of change

  • New feature / enhancement
  • Optimization
  • Bug fix

List of required branches from other repositories

DUNE-DAQ/druncschema#85

Suggested manual testing checklist

Requires three shells. Currently tested on NFD_DEV_260504_A9 on np04-srv-028.

logging related testing

  • Open a new shell with dunedaq env and run the process manager terminal

    • drunc-process-manager-shell grpc://localhost:50000
    • Watch the correct commands pop up here
  • Open a new shell with dunedaq env and run the process manager shell terminal

    • drunc-process-manager-shell grpc://localhost:50000
  • Open a new shell with dunedaq env and run the unified shell with session name of your choice (this command uses its own example)

    • drunc-unified-shell grpc://np04-srv-028:50000 config/daqsystemtest/example-configs.data.xml local-1x1-config emir-test-1
    • Bug fixing testing
      • Replace np04-srv-028 with localhost. see it works
      • Note that the oksconflibs: is not required when passing the config in the unified shell, showing it works. Also test by adding in oksconflibs: in the above config, eg oksconflibs:config/daqsystemtest/example-configs.data.xml
  • Boot sessions:

    • In the unified shell: boot
    • In the PM Shell: boot oksconflibs:config/daqsystemtest/example-configs.data.xml local-1x1-config emir-test-3
  • Try the different commands that exist in both shells

    • pm should work as expected in both. Note that it will only show the relevant session names in the unified shell
    • Logs should also work. Similar behaviour above.
      • Eg try running logs --name root-controller in both shells. it should only work in the unified shell since there are multiple root-controllers in the PM shell
      • Specifying the correct session should work for both
      • Specifying a different session than the one that the US is connected to should fail
    • Do the same for every other PM commands that exist
    • wait, restart, kill, flush, terminate

TESTING NOTES

Note so far has only been tested on ssh-local. Have not tried on k8s or similar. I dont think I've broken the main way we do it which is via unified shell but lets see..

Also pytests will pass, but there is one that fails. This is clearly documented in the PR changes though, and will have to pass before it can go in.

Developer checklist

Prior to marking this as "Ready for Review"

Tests ran on: WHAT HOSTNAME from release RELEASE_NAME

Unit tests - some tests can't be ran on the CI. This is documented. If this PR checks a feature that can't be tested with CI, this has been marked appropriately.

Integration tests - the daqsystemtest_integtest_bundle requires a lot of resources, and connections to the EHN1 infrastructure. Check the cross referenced list if you can't run these. The developer needs to run at least the .

  • Unit tests (pytest --marker) passed
    • With relevant marker
    • Without marker
  • Integration tests passed
    • Only daqsystemtest_integtest_bundle.sh -k minimal_system_quick_test.py
    • Full daqsystemtest_integtest_bundle.sh
  • Testing skipped as there are no core code changes in this PR, this only relates to documentation/CI workflows
  • Drunc integration tests pass (./scripts/drunc_integtest_bundle.sh)

Final checklist prior to marking this as "Ready for Review"

  • Code is clearly commented.
  • New unit tests have been added, or is documented in # ISSUE NUMBER
  • A suitable reviewer has been chosen from this list.

Reviewer checklist

  • This branch has been rebased with develop prior to testing.
  • Suggested manual tests show changes.
  • CI workflows fails documented (if present)
  • Integration tests passed
    • Only concern yourself if failures related to drunc are in the log files
    • If non-drunc failure appears:
      • Validate failure in fresh working area
      • Contact Pawel if unsure

Once the features are validated and both the unit and integration tests pass, the PRs is ready to be merged.

Prior to merging

Choose one of the following an complete all substeps
  • Changes only affect the Run Control, are in a single repository, and do not affect the end user.
    • Changes are documented in docstrings and code comments
    • Wiki has been updated if architectural or endpoint changes
  • Otherwise
    • Workflow changes demonstrated in the Change Log (if necessary)
    • Wiki has been updated (if necessary)
    • #daq-sw-librarians Slack channel notified (see below)

Once completed, the reviewer can merge the PR.

Notification message for a Slack channel

Note - this should be to #dunedaq-integration for general workflow that isn't during a release candidate period, and to #daq-release-prep otherwise.

For an single merge that changes the user workflow

The CCM WG has an isolated PR ready to merge that affects user workflows. The PR is:

_URL_

I will leave time for any comments, otherwise will merge these at the end of the work day _Insert your time zone_.

For co-ordinated merge

The CCM WG has a set of co-ordinated merges ready to merge. The PRs are:

_URL_

_URL_


I will leave time for any comments, otherwise will merge these at the end of the day.

@emmuhamm emmuhamm self-assigned this May 6, 2026
@emmuhamm

emmuhamm commented May 6, 2026

Copy link
Copy Markdown
Member Author

buzz off copilot

@emmuhamm
emmuhamm force-pushed the emmuhamm/split-shell-improvements branch from 6452a4d to e03689b Compare May 6, 2026 16:19
@emmuhamm emmuhamm added the gRPC label May 6, 2026
@emmuhamm

Copy link
Copy Markdown
Member Author

Keep this targetting Pawel's branch so we have a 'split shell / multi user' branch to test on

@emmuhamm emmuhamm linked an issue May 13, 2026 that may be closed by this pull request
@emmuhamm
emmuhamm requested a review from PawelPlesniak May 14, 2026 11:01
@emmuhamm

Copy link
Copy Markdown
Member Author

@PawelPlesniak I think this is ready for your look and testing!!

@emmuhamm
emmuhamm marked this pull request as ready for review May 14, 2026 11:02
@PawelPlesniak

Copy link
Copy Markdown
Collaborator

This is great, thank you Emir!
I ran a single instance of the drunc-process-manager, and the suggested changes address all the issues in the PRs.
I skipped running the integ tests as this is still aiming for full review from this branch into develop

@PawelPlesniak
PawelPlesniak merged commit 66c8707 into PawelPlesniakEmuhammad/SplitShellFixing May 14, 2026
2 of 4 checks passed
@PawelPlesniak
PawelPlesniak deleted the emmuhamm/split-shell-improvements branch May 14, 2026 14:39
emmuhamm added a commit that referenced this pull request Jun 22, 2026
emmuhamm added a commit that referenced this pull request Jun 23, 2026
emmuhamm added a commit that referenced this pull request Jun 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment