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

fix(#592): Introduce build order strategy #4523

Merged
merged 2 commits into from
Jul 6, 2023

Conversation

christophd
Copy link
Contributor

@christophd christophd commented Jun 29, 2023

The new build order strategy simply decides which of the already submitted builds gets picked first for execution (e.g. FIFO). This already allows to run the submitted builds in parallel to each other as long as max running builds limit has not been reached.

  • Run builds on same operator namespace with user defined strategy
  • Default strategy is "sequential" running only one single build at a time
  • Also support "fifo" strategy where builds are run/queued based on their creation timestamp. This strategy allows parallel builds as long as individual build dependency lists are not colliding
  • Users may adjust/overwrite the build order strategy via install command option, in the (local) integration platform settings or via the builder trait option
  • Max number of running builds limitation is not affected by the build order strategy (ensure to always obey the limitation)

Release Note

feat: Introduce build order strategy

@christophd christophd force-pushed the issue/592/build-order-strategy branch 3 times, most recently from b06bea2 to 2e05e48 Compare June 30, 2023 09:29
Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

I think the most interesting feature would be to have a strategy that run in parallel unless there is any possible dependency between builds (which I'd propose as the default choice). The structure looks fine, so, looking forward to see such implementation in place.

@christophd
Copy link
Contributor Author

@squakez yes, this has been my priority, too. Allowing parallel builds where possible. And then I realized that such a strategy is almost already in place.

When Integrations are independent to each other in terms of dependencies and the used Camel DSL the operator will create separate integrationkits and submit the builds in parallel to each other already. The last missing piece was to remove the single running build per namespace limitation that strictly queued all builds for a namespace (e.g. the operator namespace). Now that we have the max running builds limitation we can safely run builds in parallel once they are submitted.

The new build order strategy simply decides which of the already submitted builds gets picked first for execution (e.g. FIFO). And this already allows to run the submitted builds in parallel to each other as long as max running builds limit has not been reached.

@christophd christophd changed the title [WIP] fix(#592): Introduce build order strategy fix(#592): Introduce build order strategy Jul 3, 2023
@christophd christophd marked this pull request as ready for review July 3, 2023 08:12
@christophd
Copy link
Contributor Author

The default build order strategy is set to sequential at the moment. Sequential strategy will queue the builds for a namespace which is exactly like it has been implemented before in Camel K v1. I wonder if we should use fifo as a new default strategy in Camel K v2 instead in order to run builds in parallel to each other as long as max running build limit has not been reached.

WDYT?

@squakez
Copy link
Contributor

squakez commented Jul 3, 2023

What I'd like to see as a default is the following behavior:

Given an Integration X with dependencies: a,b
Given an Integration Y with dependencies: a,b,c
Given an Integration Z with dependencies: d,e,f

When the Integrations are required to be built at the same time, the operator should be able to start a parallel build for X and Z only so that the Y will be able to reuse the Kit from X when this is finished (incremental build). But also we want to have a possible parallelism of Z which has no dependencies from other running builds..

I think this is what we actually miss.

@christophd
Copy link
Contributor Author

Sorry, but I do not see how Y will benefit from build X. I had some local testing and I think that build Y is independent of build X. The integrationkit and the image built for Y is not building on top of X from what I can see. Maybe I am missing a piece.

@squakez
Copy link
Contributor

squakez commented Jul 3, 2023

Sorry, but I do not see how Y will benefit from build X. I had some local testing and I think that build Y is independent of build X. The integrationkit and the image built for Y is not building on top of X from what I can see. Maybe I am missing a piece.

As discussed offline, the incremental image build is a feature, so we should understand how to leverage that.

@christophd
Copy link
Contributor Author

Yeah, I was missing a piece and image layering is working as expected. I need to improve the order strategy to actually work with the layering

@christophd
Copy link
Contributor Author

@squakez I have added another build order strategy dependencies that uses the list of dependencies to queue builds that may reuse image layers of already scheduled or running builds

@squakez
Copy link
Contributor

squakez commented Jul 4, 2023

@squakez I have added another build order strategy dependencies that uses the list of dependencies to queue builds that may reuse image layers of already scheduled or running builds

Nice. It would be good to have an e2e test to validate the strategy, ie, an integration that depends on another really queues up until the depending kit is ready.

@christophd christophd force-pushed the issue/592/build-order-strategy branch 3 times, most recently from db00884 to 5ab1065 Compare July 4, 2023 13:30
Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

LGTM.

}

return true, nil
return allowed, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if we could return the reason why a build is allowed or not, so that we include it in the condition. In particular, we may show the user that a build is queued waiting for a depending build to complete. However I'm not sure how much complicated is to bubble up this information, you please evaluate if it makes sense to include it or defer to a later development. I think it also would simplify the e2e test (ie, we just need to check that a build is queued up with the proper condition without waiting it to complete)

@squakez squakez added the kind/feature New feature or request label Jul 4, 2023
@christophd christophd force-pushed the issue/592/build-order-strategy branch from 5ab1065 to a9f25b1 Compare July 4, 2023 14:57
@squakez squakez linked an issue Jul 4, 2023 that may be closed by this pull request
@christophd christophd force-pushed the issue/592/build-order-strategy branch 9 times, most recently from fb15b3b to 67706ef Compare July 6, 2023 06:49
@christophd christophd force-pushed the issue/592/build-order-strategy branch 2 times, most recently from 15a7e4f to 6d54955 Compare July 6, 2023 09:59
- Run builds on same operator namespace with user defined strategy
- Default strategy is "sequential" running only one single build at a time
- Also support "fifo" strategy where builds are run/queued based on their creation timestamp. This strategy allows parallel builds as long as individual build dependency lists are not colliding
- Users may adjust/overwrite the build order strategy via install command option, in the (local) integration platform settings or via the builder trait option
- Max number of running builds limitation is not affected by the build order strategy (ensure to always obey the limitation)
- Introduce new build order strategy "dependencies"
- Strategy looks at the list of dependencies required by an Integration and queues builds that may reuse base images produced by other scheduled builds in order to leverage the incremental build option
- The strategy allows non-matching builds to run in parallel to each other
@christophd christophd force-pushed the issue/592/build-order-strategy branch from 6d54955 to 1973825 Compare July 6, 2023 14:07
@christophd christophd merged commit 21b9689 into apache:main Jul 6, 2023
@squakez squakez mentioned this pull request Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build order strategy
2 participants