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/04/05 19:57:34 UTC

[GitHub] [camel-k] christophd opened a new pull request, #4233: fix: Limit running builds on operator

christophd opened a new pull request, #4233:
URL: https://github.com/apache/camel-k/pull/4233

   Usually the builds get strictly queued because of potential image layering in terms of reusing the integration kits. There are situations though where parallel builds on the operator are possible.
   
   - Avoid many running integration builds
   - Monitor all builds started by the operator instance and limit max number of running builds according to given setting
   - By default use max running builds limit = 3 for build strategy routine
   - By default use max running builds limit = 10 for build strategy pod
   - Add max running builds setting to IntegrationPlatform 
   
   **Release Note**
   ```release-note
   fix: Limit parallel builds on operator
   ```


-- 
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 a diff in pull request #4233: fix: Limit parallel builds on operator

Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on code in PR #4233:
URL: https://github.com/apache/camel-k/pull/4233#discussion_r1159882258


##########
pkg/platform/defaults.go:
##########
@@ -226,6 +226,15 @@ func setPlatformDefaults(p *v1.IntegrationPlatform, verbose bool) error {
 			Duration: 5 * time.Minute,
 		}
 	}
+
+	if p.Status.Build.MaxRunningBuilds <= 0 {
+		if p.Status.Build.BuildStrategy == v1.BuildStrategyRoutine {

Review Comment:
   yes I have added some documentation: [docs/modules/ROOT/pages/architecture/cr/build.adoc](https://github.com/apache/camel-k/pull/4233/files#diff-2e97257191709cea3c6a41e2800a84e53380b9a01924635e4a49fa3c5eed94eb)



-- 
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 #4233: fix: Limit parallel builds on operator

Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #4233:
URL: https://github.com/apache/camel-k/pull/4233#discussion_r1161580987


##########
pkg/platform/defaults.go:
##########
@@ -226,6 +226,15 @@ func setPlatformDefaults(p *v1.IntegrationPlatform, verbose bool) error {
 			Duration: 5 * time.Minute,
 		}
 	}
+
+	if p.Status.Build.MaxRunningBuilds <= 0 {
+		if p.Status.Build.BuildStrategy == v1.BuildStrategyRoutine {
+			p.Status.Build.MaxRunningBuilds = 3

Review Comment:
   I had a second look, I think this approach is fine, as we already have several defaults for the platform there. Sorry, for the noise :)



-- 
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 #4233: fix: Limit parallel builds on operator

Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #4233:
URL: https://github.com/apache/camel-k/pull/4233#discussion_r1161540637


##########
pkg/apis/camel/v1/build_types.go:
##########
@@ -41,6 +41,8 @@ type BuildSpec struct {
 	// and its phase set to BuildPhaseFailed.
 	// +kubebuilder:validation:Format=duration
 	Timeout metav1.Duration `json:"timeout,omitempty"`
+	// the maximum amount of parallel running builds started by this operator instance
+	MaxRunningBuilds int32 `json:"maxRunningBuilds,omitempty"`

Review Comment:
   I think the Build should not be aware of this information. It seems something to the domain of the controller instead, not of this resource specifically.



##########
pkg/platform/defaults.go:
##########
@@ -226,6 +226,15 @@ func setPlatformDefaults(p *v1.IntegrationPlatform, verbose bool) error {
 			Duration: 5 * time.Minute,
 		}
 	}
+
+	if p.Status.Build.MaxRunningBuilds <= 0 {
+		if p.Status.Build.BuildStrategy == v1.BuildStrategyRoutine {
+			p.Status.Build.MaxRunningBuilds = 3

Review Comment:
   I think we should provide these defaults in the pkg/util/defautls/defaults.go resource so it's easier to inspect as it happens for other operator constants.



-- 
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 a diff in pull request #4233: fix: Limit parallel builds on operator

Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on code in PR #4233:
URL: https://github.com/apache/camel-k/pull/4233#discussion_r1159886070


##########
pkg/controller/build/build_monitor.go:
##########
@@ -0,0 +1,107 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements.  See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0
+(the "License"); you may not use this file except in compliance with
+the License.  You may obtain a copy of the License at
+
+   http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package build
+
+import (
+	"context"
+	"github.com/apache/camel-k/v2/pkg/util/kubernetes"
+	"sync"
+
+	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/selection"
+	"k8s.io/apimachinery/pkg/types"
+	ctrl "sigs.k8s.io/controller-runtime/pkg/client"
+
+	v1 "github.com/apache/camel-k/v2/pkg/apis/camel/v1"
+)
+
+var runningBuilds sync.Map
+
+type Monitor struct {
+	maxRunningBuilds int32
+}
+
+func (bm *Monitor) canSchedule(ctx context.Context, c ctrl.Reader, build *v1.Build) (bool, error) {
+	var runningBuildsTotal int32
+	runningBuilds.Range(func(_, v interface{}) bool {
+		runningBuildsTotal = runningBuildsTotal + 1
+		return true
+	})
+
+	if runningBuildsTotal >= bm.maxRunningBuilds {
+		requestName := build.Name
+		requestNamespace := build.Namespace
+		buildCreator := kubernetes.GetCamelCreator(build)
+		if buildCreator != nil {
+			requestName = buildCreator.Name
+			requestNamespace = buildCreator.Namespace
+		}
+
+		Log.WithValues("request-namespace", requestNamespace, "request-name", requestName, "max-running-builds-limit", runningBuildsTotal).
+			ForBuild(build).Infof("Maximum number of running builds (%d) exceeded", runningBuildsTotal)

Review Comment:
   Done, thanks!



-- 
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 #4233: fix: Limit parallel builds on operator

Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #4233:
URL: https://github.com/apache/camel-k/pull/4233#issuecomment-1521882635

   > @squakez no idea what is happening with the jobs here. all green on my fork [christophd#92](https://github.com/christophd/camel-k/pull/92)
   
   let's give another spin


-- 
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] oscerd merged pull request #4233: fix: Limit parallel builds on operator

Posted by "oscerd (via GitHub)" <gi...@apache.org>.
oscerd merged PR #4233:
URL: https://github.com/apache/camel-k/pull/4233


-- 
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 #4233: fix: Limit parallel builds on operator

Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on PR #4233:
URL: https://github.com/apache/camel-k/pull/4233#issuecomment-1520686357

   @squakez I finally managed to add an E2E test, could you please trigger the workflow once more? thx


-- 
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] claudio4j commented on a diff in pull request #4233: fix: Limit parallel builds on operator

Posted by "claudio4j (via GitHub)" <gi...@apache.org>.
claudio4j commented on code in PR #4233:
URL: https://github.com/apache/camel-k/pull/4233#discussion_r1158971819


##########
pkg/controller/build/build_monitor.go:
##########
@@ -0,0 +1,107 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements.  See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0
+(the "License"); you may not use this file except in compliance with
+the License.  You may obtain a copy of the License at
+
+   http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package build
+
+import (
+	"context"
+	"github.com/apache/camel-k/v2/pkg/util/kubernetes"
+	"sync"
+
+	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/selection"
+	"k8s.io/apimachinery/pkg/types"
+	ctrl "sigs.k8s.io/controller-runtime/pkg/client"
+
+	v1 "github.com/apache/camel-k/v2/pkg/apis/camel/v1"
+)
+
+var runningBuilds sync.Map
+
+type Monitor struct {
+	maxRunningBuilds int32
+}
+
+func (bm *Monitor) canSchedule(ctx context.Context, c ctrl.Reader, build *v1.Build) (bool, error) {
+	var runningBuildsTotal int32
+	runningBuilds.Range(func(_, v interface{}) bool {
+		runningBuildsTotal = runningBuildsTotal + 1
+		return true
+	})
+
+	if runningBuildsTotal >= bm.maxRunningBuilds {
+		requestName := build.Name
+		requestNamespace := build.Namespace
+		buildCreator := kubernetes.GetCamelCreator(build)
+		if buildCreator != nil {
+			requestName = buildCreator.Name
+			requestNamespace = buildCreator.Namespace
+		}
+
+		Log.WithValues("request-namespace", requestNamespace, "request-name", requestName, "max-running-builds-limit", runningBuildsTotal).
+			ForBuild(build).Infof("Maximum number of running builds (%d) exceeded", runningBuildsTotal)

Review Comment:
   It would be interesting to add a note saying the other incoming builds are going to be enqueued. So the user can reason about it.



##########
pkg/platform/defaults.go:
##########
@@ -226,6 +226,15 @@ func setPlatformDefaults(p *v1.IntegrationPlatform, verbose bool) error {
 			Duration: 5 * time.Minute,
 		}
 	}
+
+	if p.Status.Build.MaxRunningBuilds <= 0 {
+		if p.Status.Build.BuildStrategy == v1.BuildStrategyRoutine {

Review Comment:
   It would be interesting to document these values.



-- 
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 #4233: fix: Limit parallel builds on operator

Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #4233:
URL: https://github.com/apache/camel-k/pull/4233#issuecomment-1504944855

   > @squakez YAY, checks are green. I will rebase and resolve the conflicts
   
   Please, mind my previous comments as well:
   
   >  I just am not sure to understand why the controller variable slips into the Build resource. Also, I think this requires an e2e in order to validate such feature, ie, a build really queues up when the max limit is reached.
   
   Thanks.


-- 
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 #4233: fix: Limit parallel builds on operator

Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on PR #4233:
URL: https://github.com/apache/camel-k/pull/4233#issuecomment-1503460750

   @claudio4j @squakez could you please rerun the failed workflows? I assume that the errors are flaky tests and are not related to the changes made in this PR


-- 
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 #4233: fix: Limit parallel builds on operator

Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on PR #4233:
URL: https://github.com/apache/camel-k/pull/4233#issuecomment-1504909849

   @squakez YAY, checks are green. I will rebase and resolve the conflicts


-- 
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 #4233: fix: Limit parallel builds on operator

Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on PR #4233:
URL: https://github.com/apache/camel-k/pull/4233#issuecomment-1521835029

   @squakez no idea what is happening with the jobs here. 
   all green on my fork https://github.com/christophd/camel-k/pull/92


-- 
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