Skip to content

start containers in parallel#2732

Merged
rochdev merged 25 commits intomainfrom
rochdev/parallel-container-start
Jul 23, 2024
Merged

start containers in parallel#2732
rochdev merged 25 commits intomainfrom
rochdev/parallel-container-start

Conversation

@rochdev
Copy link
Copy Markdown
Member

@rochdev rochdev commented Jul 11, 2024

Motivation

Containers that don't depend on each other shouldn't have to wait for each other before starting.

Changes

Start containers in parallel. This PR is a replacement for #2670 with less changes and more flexible code to manage interdependencies.

Workflow

  1. ⚠️ Create your PR as draft ⚠️
  2. Work on you PR until the CI passes (if something not related to your task is failing, you can ignore it)
  3. Mark it as ready for review
    • Test logic is modified? -> Get a review from RFC owner. We're working on refining the codeowners file quickly.
    • Framework is modified, or non obvious usage of it -> get a review from R&P team

🚀 Once your PR is reviewed, you can merge it!

🛟 #apm-shared-testing 🛟

Reviewer checklist

  • If PR title starts with [<language>], double-check that only <language> is impacted by the change
  • No system-tests internal is modified. Otherwise, I have the approval from R&P team
  • CI is green, or failing jobs are not related to this change (and you are 100% sure about this statement)
  • A docker base image is modified?
    • the relevant build-XXX-image label is present
  • A scenario is added (or removed)?

@rochdev rochdev marked this pull request as ready for review July 12, 2024 10:57
@rochdev rochdev requested a review from a team as a code owner July 12, 2024 10:57
Copy link
Copy Markdown
Collaborator

@cbeauchesne cbeauchesne left a comment

Choose a reason for hiding this comment

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

If I'm not wrong, buddies also require the proxy (and possibly the agent?)

Comment thread utils/_context/_scenarios/core.py Outdated
Comment thread utils/_context/_scenarios/core.py Outdated
Comment thread utils/_context/containers.py Outdated
Comment thread utils/_context/containers.py Outdated
Comment thread utils/_context/containers.py Outdated
@cbeauchesne
Copy link
Copy Markdown
Collaborator

This PR is a replacement for #2670 with less changes and more flexible code to manage interdependencies.

Can I close this PR ?

Comment thread utils/_context/containers.py Outdated
rochdev and others added 4 commits July 12, 2024 13:19
@rochdev
Copy link
Copy Markdown
Member Author

rochdev commented Jul 12, 2024

If I'm not wrong, buddies also require the proxy (and possibly the agent?)

Just saw this and added a dependency on the agent just in case.

Can I close this PR ?

Yes.

@cbeauchesne cbeauchesne mentioned this pull request Jul 12, 2024
8 tasks
Comment thread utils/_context/containers.py Outdated
@cbeauchesne
Copy link
Copy Markdown
Collaborator

There are still lot of work on this PR, I'm putting it in draft mode to prevent spam in our notifs.

@cbeauchesne cbeauchesne marked this pull request as draft July 15, 2024 09:02
@rochdev
Copy link
Copy Markdown
Member Author

rochdev commented Jul 15, 2024

There are still lot of work on this PR, I'm putting it in draft mode to prevent spam in our notifs.

Can you clarify? Assuming the tests start passing it's actually ready for review.

Comment thread utils/_context/_scenarios/core.py Outdated
Comment thread utils/_context/containers.py Outdated
Comment thread utils/_context/_scenarios/core.py Outdated
@datadog-datadog-prod-us1
Copy link
Copy Markdown

datadog-datadog-prod-us1 Bot commented Jul 18, 2024

Library Vulnerabilities

Critical badge  High badge  Medium badge  Low badge

Copy link
Copy Markdown
Collaborator

@cbeauchesne cbeauchesne left a comment

Choose a reason for hiding this comment

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

LGTM. Just need to fix the CI crashes, and some minor changes, and we're good to merge !

Comment thread utils/_context/containers.py Outdated
Comment thread utils/_context/containers.py Outdated
Comment thread utils/_context/containers.py Outdated
Comment thread utils/_context/containers.py Outdated
Comment thread utils/_context/containers.py Outdated
Copy link
Copy Markdown
Collaborator

@cbeauchesne cbeauchesne left a comment

Choose a reason for hiding this comment

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

To fix Ci issue on Lib injection :

  • WeblogInjectionScenario must join threads
  • OpenTelemetryCollectorContainer.start has a wrong return type

@rochdev rochdev marked this pull request as ready for review July 19, 2024 17:27
@cbeauchesne
Copy link
Copy Markdown
Collaborator

@rochdev i've added two commits :

  • A first one that put the circular check in the main thread : having a exception raising inside a thread created an output hard to understand (or would have required to propagate the exception in the main thread). With that, it's the same logic, but easier to understand without extra code.
  • And some test cases

I'm waiting for the CI, and if it's all green (or not related, PHP failures expected ... 😭 ) -> merge !

@rochdev
Copy link
Copy Markdown
Member Author

rochdev commented Jul 22, 2024

@cbeauchesne Is everything good? I'm not sure what is going on with all the failures but it doesn't seem like it would be related?

@cbeauchesne
Copy link
Copy Markdown
Collaborator

No it's should not be. Just for my peace of mind, I'd like to have an almost-all-green CI before merging it. Don't worry, I'm taking care of it.

@rochdev rochdev merged commit fb2823d into main Jul 23, 2024
@rochdev rochdev deleted the rochdev/parallel-container-start branch July 23, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants