You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "gansheer (via GitHub)" <gi...@apache.org> on 2024/02/15 10:07:08 UTC
[PR] feat(build): Add Build waiting condition [camel-k]
gansheer opened a new pull request, #5157:
URL: https://github.com/apache/camel-k/pull/5157
* Add on Build.Status a "Scheduled" condition containing the reason for it's scheduling status
Ref #4542
<!-- 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
feat(build): Add Build waiting condition
```
--
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
Re: [PR] feat(build): Add Build waiting condition [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd merged PR #5157:
URL: https://github.com/apache/camel-k/pull/5157
--
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
Re: [PR] feat(build): Add Build waiting condition [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #5157:
URL: https://github.com/apache/camel-k/pull/5157#discussion_r1491154763
##########
pkg/controller/build/build_monitor.go:
##########
@@ -129,3 +138,28 @@ func monitorRunningBuild(build *v1.Build) {
func monitorFinishedBuild(build *v1.Build) {
runningBuilds.Delete(types.NamespacedName{Namespace: build.Namespace, Name: build.Name}.String())
}
+
+func scheduledReadyBuildcondition(build *v1.Build) *v1.BuildCondition {
+ return &v1.BuildCondition{
+ Type: v1.BuildConditionScheduled,
+ Status: corev1.ConditionTrue,
+ Reason: v1.BuildConditionReadyReason,
+ Message: fmt.Sprintf(
+ "the build (%s) is scheduled",
+ build.Name,
+ ),
+ }
+}
+
+func scheduledWaitingBuildcondition(build *v1.Build, reason string) *v1.BuildCondition {
Review Comment:
Duplicated code. Either you have a generic func to return a condition, or simply favor the inline creation of the condition from the callers.
##########
pkg/controller/build/schedule.go:
##########
@@ -59,28 +59,33 @@ func (action *scheduleAction) Handle(ctx context.Context, build *v1.Build) (*v1.
action.lock.Lock()
defer action.lock.Unlock()
- if allowed, err := action.buildMonitor.canSchedule(ctx, action.reader, build); err != nil {
+ allowed, schedulingCondition, err := action.buildMonitor.canSchedule(ctx, action.reader, build)
+
+ if err != nil {
return nil, err
} else if !allowed {
+ action.L.Infof("GFO %s Build not allowed", build.Name)
Review Comment:
Not really needed.
--
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
Re: [PR] feat(build): Add Build waiting condition [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #5157:
URL: https://github.com/apache/camel-k/pull/5157#issuecomment-1961047697
@gansheer please, rebase against `main` as we have fixed the errored check.
--
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
Re: [PR] feat(build): Add Build waiting condition [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #5157:
URL: https://github.com/apache/camel-k/pull/5157#discussion_r1490760964
##########
e2e/builder/build_test.go:
##########
@@ -130,11 +131,34 @@ func TestKitMaxBuildLimit(t *testing.T) {
// verify that all builds are successful
Eventually(BuildPhase(ns, buildA), TestTimeoutLong).Should(Equal(v1.BuildPhaseSucceeded))
+ Eventually(BuildConditions(ns, buildA), TestTimeoutLong).ShouldNot(BeNil())
Review Comment:
I don't think it's really required a check on this condition, as it will eventually get scheduled. What may be more interesting is to check that a build which is supposed to wait, have this condition turned on, and later move to scheduled.
##########
pkg/controller/build/build_monitor.go:
##########
@@ -56,6 +57,16 @@ func (bm *Monitor) canSchedule(ctx context.Context, c ctrl.Reader, build *v1.Bui
if runningBuildsTotal >= bm.maxRunningBuilds {
Log.WithValues("request-namespace", requestNamespace, "request-name", requestName, "max-running-builds-limit", runningBuildsTotal).
ForBuild(build).Infof("Maximum number of running builds (%d) exceeded - the build (%s) gets enqueued", runningBuildsTotal, build.Name)
+ build.Status.SetCondition(
+ v1.BuildConditionScheduled,
+ corev1.ConditionFalse,
+ v1.BuildConditionWaitingReason,
+ fmt.Sprintf(
+ "Maximum number of running builds (%d) exceeded - the build (%s) gets enqueued",
Review Comment:
Let's put this into a variable and reuse in the 2 places instead.
##########
pkg/controller/build/schedule.go:
##########
@@ -63,7 +65,7 @@ func (action *scheduleAction) Handle(ctx context.Context, build *v1.Build) (*v1.
return nil, err
} else if !allowed {
// Build not allowed at this state (probably max running builds limit exceeded) - let's requeue the build
- return nil, nil
+ return build, nil
Review Comment:
Reading the code comment below, it seems we must store the status of the build into this critical section, instead of letting it to be done by the controller. We may extend the `toPendingPhase` to store any generic condition and state before returning to the controller in general.
##########
pkg/controller/build/build_monitor.go:
##########
@@ -117,6 +128,16 @@ func (bm *Monitor) canSchedule(ctx context.Context, c ctrl.Reader, build *v1.Bui
if !allowed {
Log.WithValues("request-namespace", requestNamespace, "request-name", requestName, "order-strategy", bm.buildOrderStrategy).
ForBuild(build).Infof("%s - the build (%s) gets enqueued", reason, build.Name)
+ build.Status.SetCondition(
Review Comment:
ditto
--
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
Re: [PR] feat(build): Add Build waiting condition [camel-k]
Posted by "gansheer (via GitHub)" <gi...@apache.org>.
gansheer commented on code in PR #5157:
URL: https://github.com/apache/camel-k/pull/5157#discussion_r1491195320
##########
pkg/controller/build/schedule.go:
##########
@@ -59,28 +59,33 @@ func (action *scheduleAction) Handle(ctx context.Context, build *v1.Build) (*v1.
action.lock.Lock()
defer action.lock.Unlock()
- if allowed, err := action.buildMonitor.canSchedule(ctx, action.reader, build); err != nil {
+ allowed, schedulingCondition, err := action.buildMonitor.canSchedule(ctx, action.reader, build)
+
+ if err != nil {
return nil, err
} else if !allowed {
+ action.L.Infof("GFO %s Build not allowed", build.Name)
Review Comment:
:sweat_smile: that's what happens when you forget `git add`...
--
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
Re: [PR] feat(build): Add Build waiting condition [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #5157:
URL: https://github.com/apache/camel-k/pull/5157#discussion_r1494209364
##########
e2e/common/traits/builder_test.go:
##########
@@ -233,6 +234,7 @@ func TestBuilderTrait(t *testing.T) {
TestTimeoutShort).Should(ContainSubstring("No such file or directory"))
Expect(Kamel("delete", "--all", "-n", ns).Execute()).To(Succeed())
+ Expect(Kamel("reset", "-n", ns).Execute()).To(Succeed())
Review Comment:
If we require to reset in the "common" e2e flow, then we should better move this test in the "common-with-custom-install".
--
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