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 2024/04/24 17:36:03 UTC
[PR] fix(#5402): Make sure to enable Knative profile [camel-k]
christophd opened a new pull request, #5419:
URL: https://github.com/apache/camel-k/pull/5419
- Use Knative profile when Serving or Eventing is installed on cluster
- Make sure to enable Knative trait when Serving or Eventing is installed
- Enable knative-service trait only when Knative Serving is installed
**Release Note**
```release-note
fix(#5402): Make sure to enable Knative profile when installed
```
--
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] fix(#5402): Make sure to enable Knative profile [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on PR #5419:
URL: https://github.com/apache/camel-k/pull/5419#issuecomment-2075487101
also fixes #4962
--
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] fix(#5402): Make sure to enable Knative profile [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on code in PR #5419:
URL: https://github.com/apache/camel-k/pull/5419#discussion_r1579135921
##########
pkg/trait/knative_service.go:
##########
@@ -149,6 +150,11 @@ func (t *knativeServiceTrait) SelectControllerStrategy(e *Environment) (*Control
return nil, nil
}
+ // Knative serving is required
+ if ok, _ := knative.IsServingInstalled(e.Client); !ok {
Review Comment:
but the controller strategy selector is a totally different interface. The `Configure()` already evaluates the controller strategy to skip the trait when the strategy is not Knative. Apart from that we need to make sure that the controller strategy is only Knative when Serving is installed as the interface can be used also when the trait is disabled. This is why I put this into the controller strategy selector interface implementation part of the trait
--
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] fix(#5402): Evaluate Knative profile based on Serving/Eventing installed [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd merged PR #5419:
URL: https://github.com/apache/camel-k/pull/5419
--
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] fix(#5402): Make sure to enable Knative profile [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on PR #5419:
URL: https://github.com/apache/camel-k/pull/5419#issuecomment-2076681555
> I think the check on the serving API was done on purpose, however I don't have enough historical knowledge about it. Please, @lburgazzoli have a look at this.
I think it was checking explicitly for Serving because the knative-service trait only works when Serving is available. I have added this check on Serving explicitly to the knative-service trait when it domes to detect the service controller strategy. It should make sure that the arbitrary deployment controller is used when Serving is not available.
--
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] fix(#5402): Evaluate Knative profile based on Serving/Eventing installed [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on PR #5419:
URL: https://github.com/apache/camel-k/pull/5419#issuecomment-2078890768
@squakez issues are fixed now. also added some unit tests. ready for review, 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
Re: [PR] fix(#5402): Make sure to enable Knative profile [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on PR #5419:
URL: https://github.com/apache/camel-k/pull/5419#issuecomment-2078012468
lets put this on hold for a while. I found out that gc trait needs adjustments, too. This is because the trait tries to remove Knative Serving resources based on the active Knative profile, but it may only be Eventing installed and Serving is not available.
Also I found out that the Knative trigger created by Camel K always uses Serving service as a subscriber ref. We should fallback to arbitrary service when Serving is not available.
I will work on this and try to incorporate the fixes 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
Re: [PR] fix(#5402): Evaluate Knative profile based on Serving/Eventing installed [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #5419:
URL: https://github.com/apache/camel-k/pull/5419#discussion_r1580833131
##########
pkg/util/knative/knative.go:
##########
@@ -75,7 +75,27 @@ func CreateSubscription(channelReference corev1.ObjectReference, serviceName str
}
}
-func CreateTrigger(brokerReference corev1.ObjectReference, serviceName string, eventType string, path string) *eventing.Trigger {
+// CreateServiceTrigger create Knative trigger with arbitrary Kubernetes Service as a subscriber - usually used when no Knative Serving is available on the cluster.
+func CreateServiceTrigger(brokerReference corev1.ObjectReference, serviceName string, eventType string, path string) (*eventing.Trigger, error) {
+ subscriberRef := duckv1.KReference{
+ APIVersion: "v1",
+ Kind: "Service",
+ Name: serviceName,
+ }
+ return CreateTrigger(brokerReference, subscriberRef, eventType, path)
+}
+
+// CreateKnativeServiceTrigger create Knative trigger with Knative Serving Service as a subscriber - default option when Knative Serving is available on the cluster.
+func CreateKnativeServiceTrigger(brokerReference corev1.ObjectReference, serviceName string, eventType string, path string) (*eventing.Trigger, error) {
+ subscriberRef := duckv1.KReference{
+ APIVersion: serving.SchemeGroupVersion.String(),
+ Kind: "Service",
+ Name: serviceName,
+ }
+ return CreateTrigger(brokerReference, subscriberRef, eventType, path)
+}
+
+func CreateTrigger(brokerReference corev1.ObjectReference, subscriberRef duckv1.KReference, eventType string, path string) (*eventing.Trigger, error) {
Review Comment:
I think it makes sense to have this with a private scope as I think any client has to call either Service or KnativeService on purpose.
--
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] fix(#5402): Make sure to enable Knative profile [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #5419:
URL: https://github.com/apache/camel-k/pull/5419#discussion_r1579119631
##########
pkg/trait/knative_service.go:
##########
@@ -149,6 +150,11 @@ func (t *knativeServiceTrait) SelectControllerStrategy(e *Environment) (*Control
return nil, nil
}
+ // Knative serving is required
+ if ok, _ := knative.IsServingInstalled(e.Client); !ok {
Review Comment:
I missed this. We must move this into the `Configure()` func instead and add the related condition and report accordingly in order to warn the user that the trait is skipped because there is no serving API installed.
--
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] WIP: fix(#5402): Evaluate Knative profile based on Serving/Eventing installed [camel-k]
Posted by "matzew (via GitHub)" <gi...@apache.org>.
matzew commented on PR #5419:
URL: https://github.com/apache/camel-k/pull/5419#issuecomment-2078646373
> Also I found out that the Knative trigger created by Camel K always uses Serving service as a subscriber ref. We should fallback to arbitrary service when Serving is not available.
+1
while serving is a valid (opinionated) choice when present, it generally works w/ just Kube `Service/v1` API. Which is the real meat and hook point of Eventing. It is not just "serverless", as in Knative Serving :smile:
--
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] fix(#5402): Evaluate Knative profile based on Serving/Eventing installed [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on code in PR #5419:
URL: https://github.com/apache/camel-k/pull/5419#discussion_r1580743874
##########
pkg/trait/knative.go:
##########
@@ -560,3 +571,19 @@ func (t *knativeTrait) extractServices(names []string, serviceType knativeapi.Ca
sort.Strings(answer)
return answer
}
+
+func (t *knativeTrait) determineServiceType(e *Environment) (string, error) {
+ controllerStrategy, err := e.DetermineControllerStrategy()
+ if err != nil {
+ return "", err
+ }
+
+ switch controllerStrategy {
+ case ControllerStrategyKnativeService:
+ return serving.SchemeGroupVersion.String(), nil
+ case ControllerStrategyDeployment:
Review Comment:
I' d rather raise the error in this case because a CronJob deployment strategy does not fit the Trigger.
--
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] fix(#5402): Make sure to enable Knative profile [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #5419:
URL: https://github.com/apache/camel-k/pull/5419#issuecomment-2076566530
Btw, we probably need a unit test in order to avoid introducing any regression in the future and make sure the expected behavior is captured and checked consistently.
--
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] WIP: fix(#5402): Evaluate Knative profile based on Serving/Eventing installed [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #5419:
URL: https://github.com/apache/camel-k/pull/5419#issuecomment-2078806751
Converting as a draft to avoid unintentional merge
--
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] fix(#5402): Evaluate Knative profile based on Serving/Eventing installed [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #5419:
URL: https://github.com/apache/camel-k/pull/5419#discussion_r1580698963
##########
pkg/trait/knative.go:
##########
@@ -486,9 +488,18 @@ func (t *knativeTrait) createTrigger(e *Environment, ref *corev1.ObjectReference
if ref.Namespace == "" {
ref.Namespace = e.Integration.Namespace
}
- trigger := knativeutil.CreateTrigger(*ref, e.Integration.Name, eventType, path)
+ serviceAPIVersion, err := t.determineServiceType(e)
+ if err != nil {
+ return err
+ }
+ trigger, err := knativeutil.CreateTrigger(*ref, e.Integration.Name, serviceAPIVersion, eventType, path)
Review Comment:
In order to simplify the maintenance, see the some comment below related to the CreateTrigger func. I think here, we should just call either CreateTriggerService or CreateTriggerKnativeService to make it more explicit. Also mind that we do have CronJob controller strategy.
##########
pkg/trait/knative.go:
##########
@@ -560,3 +571,19 @@ func (t *knativeTrait) extractServices(names []string, serviceType knativeapi.Ca
sort.Strings(answer)
return answer
}
+
+func (t *knativeTrait) determineServiceType(e *Environment) (string, error) {
+ controllerStrategy, err := e.DetermineControllerStrategy()
+ if err != nil {
+ return "", err
+ }
+
+ switch controllerStrategy {
+ case ControllerStrategyKnativeService:
+ return serving.SchemeGroupVersion.String(), nil
+ case ControllerStrategyDeployment:
Review Comment:
Probably also when CronJob strategy. In fact, why not defaulting to CreateTriggerService if not Knative enabled?
##########
pkg/util/knative/knative.go:
##########
@@ -75,7 +75,7 @@ func CreateSubscription(channelReference corev1.ObjectReference, serviceName str
}
}
-func CreateTrigger(brokerReference corev1.ObjectReference, serviceName string, eventType string, path string) *eventing.Trigger {
+func CreateTrigger(brokerReference corev1.ObjectReference, serviceName string, serviceAPIVersion string, eventType string, path string) (*eventing.Trigger, error) {
Review Comment:
Altough the logic is correct, I think we're making implicit an implementation detail that should be made explicit making the long term maintenance a possible problem (above all for anybody not knowing this part of the project). I think it's okey to refactor the code and have a private `createTrigger` func like this one, and then two public `CreateTriggerService()` and `CreateTriggerKnativeService` which clarify the intention in the signature. These two func would be calling `createTrigger` with the exepected apiVersion.
--
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