Skip to content

Add more control commands #132

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

Merged
merged 3 commits into from
Apr 7, 2025
Merged

Conversation

AlanCoding
Copy link
Member

This adds the control commands:

  • producers
  • run
  • status

Only the "producers" actually does much novel, and what it adds isn't much, because we need #126 to track more meaningful information on the producers.

The "run" command seems silly, but needs #122, so this combined with that will allow tasks to run control tasks... and one control task can... run other tasks. Put it together, this would allow tasks to run other tasks.

This is extremely demo-able

$ dispatcherctl --help
usage: dispatcherctl [-h] [--log-level {DEBUG,INFO,WARNING,ERROR,CRITICAL}] [--config CONFIG] [--task TASK] [--uuid UUID]
                     [--expected-replies EXPECTED_REPLIES]
                     {running,cancel,alive,aio_tasks,workers,producers,status}

CLI entrypoint for dispatcher, mainly intended for testing.

and

$ dispatcherctl producers
DEBUG:dispatcher.cli:Configured standard out logging at DEBUG level
INFO:dispatcher.cli:Using config from file /home/alancoding/repos/dispatcher/dispatcher.yml
INFO:awx.main.dispatch.control:control-and-reply producers to None
INFO:dispatcher.brokers.pg_notify:Set up pg_notify listening on channel 'reply_to_0e4410ea_541c_47b7_bb25_aa472084f572'
DEBUG:dispatcher.brokers.pg_notify:Sent pg_notify message of 85 chars to test_channel
DEBUG:dispatcher.brokers.pg_notify:Starting listening for pg_notify notifications
INFO:awx.main.dispatch.control:control-and-reply message returned in 0.07825517654418945 seconds
- BrokeredProducer:
    produced_count: 2
  ControlProducer:
    produced_count: 0
  OnStartProducer:
    produced_count: 1
  ScheduledProducer:
    produced_count: 20
  node_id: demo-server-a

And the dispatcher status command is nice, but its output is a bit more than I want to paste here.

@AlanCoding AlanCoding force-pushed the more_control branch 2 times, most recently from 6003126 to 79a5bfa Compare March 24, 2025 13:56
@AlanCoding AlanCoding added debugging enhancement New feature or request labels Mar 24, 2025
@art-tapin art-tapin self-requested a review April 7, 2025 14:44
@AlanCoding AlanCoding changed the title Add more control commands for debugging Add more control commands Apr 7, 2025
Add tests for new commands

Fix linters

Add command for counts from main classes

Use more consistent status data method

Add pid of main process
Copy link
Collaborator

@art-tapin art-tapin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a fantastic addition to the dispatcher! Before your latest commit, I ran tested the PR covering:

  • Basic control commands (producers, status)
  • Task-to-task communication (parent tasks launching child tasks)
  • Multi-level task chaining (tasks launching tasks launching tasks...)
  • Edge cases (error handling, timeouts, parallel execution)

Everything works as expected and the implementation is solid! LGTM ✔️

However, I did notice one integration test failing with your latest "Add test for task-from-task" commit:

tests/integration/test_disruptions.py::test_sever_pg_connection
assert 2 == 1

The issue is that test_sever_pg_connection assumes there's only one producer (assert len(apg_dispatcher.producers) == 1), but you've added ControlProducer to the default test configuration in conftest.py, which increases the count to 2.

This should be easy to fix - either update the test to not make assumptions about the exact number of producers or modify it to specifically check for the presence of the pg_notify producer.

Otherwise, the PR looks great and the documentation updates are helpful.

Approved, just don't forget about that small test issue is addressed 🤓

🥇

@AlanCoding AlanCoding merged commit a9c6823 into ansible:main Apr 7, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debugging enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants