Skip to content
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

Reduce executor code for docker #4438

Merged
merged 7 commits into from
Jul 18, 2023
Merged

Conversation

mdegat01
Copy link
Contributor

Proposed change

Currently nearly everything in the core DockerInterface class everything shares is done in the executor. This is problematic for a few reasons:

  1. Not all of it needs to be. I/O interactions with docker should be done there but interactions with coresys objects don't need to be
  2. We occasionally modify coresys objects (like when creating issues). These objects are shared across threads so this isn't safe
  3. RFC: Job based API #1946 is quite difficult to implement. The goal is to allow one job to call another and track that interaction. But if entire jobs are occurring in the executor any place we did that tracking would have to be mutable across threads. If jobs are asyncio tasks that call into the executor as needed for I/O this becomes much more straightforward.

The docker interface is the biggest offender here, this PR changes that.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality to the supervisor)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:
  • Link to cli pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast supervisor tests)
  • Tests have been added to verify that the new code works.

If API endpoints of add-on configuration are added/changed:

@mdegat01 mdegat01 added the refactor A code change that neither fixes a bug nor adds a feature label Jul 13, 2023
@mdegat01 mdegat01 requested review from agners and pvizeli July 13, 2023 18:55
@mdegat01 mdegat01 force-pushed the reduce-executor-code-for-docker branch from 922d9d9 to e513826 Compare July 13, 2023 19:33
Copy link
Member

@agners agners left a comment

Choose a reason for hiding this comment

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

Otherwise nice cleanup, LGTM!

tests/dbus/network/test_network_manager.py Show resolved Hide resolved
@mdegat01 mdegat01 force-pushed the reduce-executor-code-for-docker branch from 98f2e30 to b358b60 Compare July 17, 2023 21:28
@mdegat01 mdegat01 merged commit 1f92ab4 into main Jul 18, 2023
@mdegat01 mdegat01 deleted the reduce-executor-code-for-docker branch July 18, 2023 15:39
@github-actions github-actions bot locked and limited conversation to collaborators Jul 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed refactor A code change that neither fixes a bug nor adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants