You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "christophd (via GitHub)" <gi...@apache.org> on 2023/06/29 08:25:14 UTC
[GitHub] [camel-k] christophd opened a new pull request, #4523: [WIP] fix(#592): Introduce build order strategy
christophd opened a new pull request, #4523:
URL: https://github.com/apache/camel-k/pull/4523
- 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)
<!-- Description -->
<!--
Enter your extended release note in the below block. If the PR requires
additional action from users switching to the new release, include the string
"action required". If no release note is required, write "NONE".
You can (optionally) mark this PR with labels "kind/bug" or "kind/feature" to make sure
the text is added to the right section of the release notes.
-->
**Release Note**
```release-note
NONE
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] christophd commented on pull request #4523: fix(#592): Introduce build order strategy
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on PR #4523:
URL: https://github.com/apache/camel-k/pull/4523#issuecomment-1618168063
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] christophd merged pull request #4523: fix(#592): Introduce build order strategy
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd merged PR #4523:
URL: https://github.com/apache/camel-k/pull/4523
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #4523: fix(#592): Introduce build order strategy
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #4523:
URL: https://github.com/apache/camel-k/pull/4523#issuecomment-1619699896
> @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #4523: fix(#592): Introduce build order strategy
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #4523:
URL: https://github.com/apache/camel-k/pull/4523#issuecomment-1617714001
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. But also we want to have a possible parallelism of Z which has no dependencies in later incremental image reuse.
I think this is what we actually miss.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] christophd commented on pull request #4523: fix(#592): Introduce build order strategy
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on PR #4523:
URL: https://github.com/apache/camel-k/pull/4523#issuecomment-1618834814
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #4523: fix(#592): Introduce build order strategy
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #4523:
URL: https://github.com/apache/camel-k/pull/4523#issuecomment-1618561618
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on a diff in pull request #4523: fix(#592): Introduce build order strategy
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #4523:
URL: https://github.com/apache/camel-k/pull/4523#discussion_r1252064352
##########
pkg/controller/build/build_monitor.go:
##########
@@ -84,18 +85,26 @@ func (bm *Monitor) canSchedule(ctx context.Context, c ctrl.Reader, build *v1.Bui
return false, err
}
- // Emulate a serialized working queue to only allow one build to run at a given time.
- // This is currently necessary for the incremental build to work as expected.
- // We may want to explicitly manage build priority as opposed to relying on
- // the reconciliation loop to handle the queuing.
- for _, b := range builds.Items {
- if b.Status.Phase == v1.BuildPhasePending || b.Status.Phase == v1.BuildPhaseRunning {
- // Let's requeue the build in case one is already running
- return false, nil
- }
+ var allowed bool
+ switch bm.buildOrderStrategy {
+ case v1.BuildOrderStrategyFIFO:
+ // Check on builds that have been created before the current build and grant precedence if any.
+ allowed = !builds.HasScheduledBuildsBefore(build)
+ case v1.BuildOrderStrategyDependencies:
+ // Check on the Integration dependencies and see if we should queue the build in order to leverage incremental builds
+ // because there is already another build in the making that matches the requirements
+ allowed = !builds.HasMatchingBuild(build)
+ case v1.BuildOrderStrategySequential:
+ // Emulate a serialized working queue to only allow one build to run at a given time.
+ // Let's requeue the build in case one is already running
+ allowed = !builds.HasRunningBuilds()
+ default:
+ Log.WithValues("request-namespace", requestNamespace, "request-name", requestName, "order-strategy", bm.buildOrderStrategy).
+ ForBuild(build).Infof("Unsupported build order strategy (%s) - the build gets enqueued", bm.buildOrderStrategy)
+ allowed = false
}
- return true, nil
+ return allowed, nil
Review Comment:
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] christophd commented on pull request #4523: fix(#592): Introduce build order strategy
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on PR #4523:
URL: https://github.com/apache/camel-k/pull/4523#issuecomment-1619672340
@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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] christophd commented on pull request #4523: [WIP] fix(#592): Introduce build order strategy
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on PR #4523:
URL: https://github.com/apache/camel-k/pull/4523#issuecomment-1617580820
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] christophd commented on pull request #4523: fix(#592): Introduce build order strategy
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on PR #4523:
URL: https://github.com/apache/camel-k/pull/4523#issuecomment-1617611676
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org