astefanutti commented on code in PR #3480: URL: https://github.com/apache/camel-k/pull/3480#discussion_r936453260
########## e2e/global/common/kamelet_binding_with_image_test.go: ########## @@ -0,0 +1,120 @@ +//go:build integration +// +build integration + +// To enable compilation of this file in Goland, go to "Settings -> Go -> Vendoring & Build Tags -> Custom Tags" and add "integration" + +/* +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 common + +import ( + "github.com/onsi/gomega/gstruct" + "testing" + + . "github.com/apache/camel-k/e2e/support" + . "github.com/onsi/gomega" + + corev1 "k8s.io/api/core/v1" + + "github.com/apache/camel-k/pkg/apis/camel/v1alpha1" +) + +func TestBindingWithImage(t *testing.T) { + WithNewTestNamespace(t, func(ns string) { + operatorID := "camel-k-binding-image" + bindingID := "with-image-binding" + + Expect(KamelInstallWithID(operatorID, ns).Execute()).To(Succeed()) + + from := corev1.ObjectReference{ + Kind: "Kamelet", + Name: "my-own-timer-source", + APIVersion: v1alpha1.SchemeGroupVersion.String(), + } + + to := corev1.ObjectReference{ + Kind: "Kamelet", + Name: "my-own-log-sink", + APIVersion: v1alpha1.SchemeGroupVersion.String(), + } + + emptyMap := map[string]string{} + + annotations1 := map[string]string{ + "trait.camel.apache.org/container.image": "docker.io/jmalloc/echo-server:0.3.2", + "trait.camel.apache.org/jvm.enabled": "false", + "trait.camel.apache.org/kamelets.enabled": "false", + "trait.camel.apache.org/dependencies.enabled": "false", + "test": "1", + } + annotations2 := map[string]string{ + "trait.camel.apache.org/container.image": "docker.io/jmalloc/echo-server:0.3.3", + "trait.camel.apache.org/jvm.enabled": "false", + "trait.camel.apache.org/kamelets.enabled": "false", + "trait.camel.apache.org/dependencies.enabled": "false", + "test": "2", + } + + t.Run("run with initial image", func(t *testing.T) { + expectedImage := annotations1["trait.camel.apache.org/container.image"] + + RegisterTestingT(t) + + Expect(BindKameletTo(ns, bindingID, annotations1, from, to, emptyMap, emptyMap)()). + To(Succeed()) + Eventually(IntegrationGeneration(ns, bindingID)). + Should(gstruct.PointTo(BeNumerically("==", 1))) + Eventually(IntegrationAnnotations(ns, bindingID)). + Should(HaveKeyWithValue("test", "1")) + Eventually(IntegrationAnnotations(ns, bindingID)). + Should(HaveKeyWithValue("trait.camel.apache.org/container.image", expectedImage)) + Eventually(IntegrationStatusImage(ns, bindingID)). + Should(Equal(expectedImage)) + + Eventually(IntegrationPodPhase(ns, bindingID), TestTimeoutLong). + Should(Equal(corev1.PodRunning)) + Eventually(IntegrationPodImage(ns, bindingID)). + Should(Equal(expectedImage)) + }) + + t.Run("run with new image", func(t *testing.T) { + expectedImage := annotations2["trait.camel.apache.org/container.image"] + + RegisterTestingT(t) + + Expect(BindKameletTo(ns, bindingID, annotations2, from, to, emptyMap, emptyMap)()). + To(Succeed()) + Eventually(IntegrationGeneration(ns, bindingID)). + Should(gstruct.PointTo(BeNumerically("==", 1))) + Eventually(IntegrationAnnotations(ns, bindingID)). Review Comment: I'd suggest to create a _transform_ function, e.g.: ```go func Annotations(object metav1.Object) map[string]string { return object.GetAnnotations() } ``` So it can be re-used for any object, and that can be used as: ```go Eventually(Integration(ns, bindingID)).Should(WithTransform(Annotations, HaveKeyWithValue("test", "2"))) ``` That would ultimately give: ```go Eventually(Integration(ns, bindingID)).Should(WithTransform(Annotations, And( HaveKeyWithValue("test", "2"))), HaveKeyWithValue("trait.camel.apache.org/container.image", expectedImage)), ) ``` ########## pkg/controller/integration/integration_controller.go: ########## @@ -75,6 +77,115 @@ func newReconciler(mgr manager.Manager, c client.Client) reconcile.Reconciler { ) } +func integrationUpdateFunc(old *v1.Integration, it *v1.Integration) bool { + // Observe the time to first readiness metric + previous := old.Status.GetCondition(v1.IntegrationConditionReady) + if next := it.Status.GetCondition(v1.IntegrationConditionReady); (previous == nil || previous.Status != corev1.ConditionTrue && (previous.FirstTruthyTime == nil || previous.FirstTruthyTime.IsZero())) && + next != nil && next.Status == corev1.ConditionTrue && next.FirstTruthyTime != nil && !next.FirstTruthyTime.IsZero() && + it.Status.InitializationTimestamp != nil { + duration := next.FirstTruthyTime.Time.Sub(it.Status.InitializationTimestamp.Time) + Log.WithValues("request-namespace", it.Namespace, "request-name", it.Name, "ready-after", duration.Seconds()). + ForIntegration(it).Infof("First readiness after %s", duration) + timeToFirstReadiness.Observe(duration.Seconds()) + } + + // If traits have changed, the reconciliation loop must kick in as + // traits may have impact + sameTraits, err := trait.IntegrationsHaveSameTraits(old, it) + if err != nil { + Log.ForIntegration(it).Error( + err, + "unable to determine if old and new resource have the same traits") + } + if !sameTraits { + return true + } + + // Ignore updates to the integration status in which case metadata.Generation does not change, + // or except when the integration phase changes as it's used to transition from one phase + // to another. + return old.Generation != it.Generation || + old.Status.Phase != it.Status.Phase +} + +func integrationKitEnqueueRequestsFromMapFunc(c client.Client, kit *v1.IntegrationKit) []reconcile.Request { + var requests []reconcile.Request + if kit.Status.Phase != v1.IntegrationKitPhaseReady && kit.Status.Phase != v1.IntegrationKitPhaseError { + return requests + } + + list := &v1.IntegrationList{} + // Do global search in case of global operator (it may be using a global platform) + var opts []ctrl.ListOption + if !platform.IsCurrentOperatorGlobal() { + opts = append(opts, ctrl.InNamespace(kit.Namespace)) + } + if err := c.List(context.Background(), list, opts...); err != nil { + log.Error(err, "Failed to retrieve integration list") + return requests + } + + for i := range list.Items { + integration := &list.Items[i] + log.Debug("Integration Controller: Assessing integration", "integration", integration.Name, "namespace", integration.Namespace) Review Comment: Probably not introduced by this PR, but seems better to re-used the scoped logger, e.g. `Log.Debug("Filtering integration from kit changes", ...)`. ########## pkg/controller/integration/integration_controller.go: ########## @@ -75,6 +77,115 @@ func newReconciler(mgr manager.Manager, c client.Client) reconcile.Reconciler { ) } +func integrationUpdateFunc(old *v1.Integration, it *v1.Integration) bool { + // Observe the time to first readiness metric + previous := old.Status.GetCondition(v1.IntegrationConditionReady) + if next := it.Status.GetCondition(v1.IntegrationConditionReady); (previous == nil || previous.Status != corev1.ConditionTrue && (previous.FirstTruthyTime == nil || previous.FirstTruthyTime.IsZero())) && + next != nil && next.Status == corev1.ConditionTrue && next.FirstTruthyTime != nil && !next.FirstTruthyTime.IsZero() && + it.Status.InitializationTimestamp != nil { + duration := next.FirstTruthyTime.Time.Sub(it.Status.InitializationTimestamp.Time) + Log.WithValues("request-namespace", it.Namespace, "request-name", it.Name, "ready-after", duration.Seconds()). + ForIntegration(it).Infof("First readiness after %s", duration) + timeToFirstReadiness.Observe(duration.Seconds()) + } + + // If traits have changed, the reconciliation loop must kick in as + // traits may have impact + sameTraits, err := trait.IntegrationsHaveSameTraits(old, it) + if err != nil { + Log.ForIntegration(it).Error( + err, + "unable to determine if old and new resource have the same traits") + } + if !sameTraits { + return true + } + + // Ignore updates to the integration status in which case metadata.Generation does not change, + // or except when the integration phase changes as it's used to transition from one phase + // to another. + return old.Generation != it.Generation || + old.Status.Phase != it.Status.Phase +} + +func integrationKitEnqueueRequestsFromMapFunc(c client.Client, kit *v1.IntegrationKit) []reconcile.Request { + var requests []reconcile.Request + if kit.Status.Phase != v1.IntegrationKitPhaseReady && kit.Status.Phase != v1.IntegrationKitPhaseError { + return requests + } + + list := &v1.IntegrationList{} + // Do global search in case of global operator (it may be using a global platform) + var opts []ctrl.ListOption + if !platform.IsCurrentOperatorGlobal() { + opts = append(opts, ctrl.InNamespace(kit.Namespace)) + } + if err := c.List(context.Background(), list, opts...); err != nil { + log.Error(err, "Failed to retrieve integration list") + return requests + } + + for i := range list.Items { + integration := &list.Items[i] + log.Debug("Integration Controller: Assessing integration", "integration", integration.Name, "namespace", integration.Namespace) + + match, err := sameOrMatch(kit, integration) + if err != nil { + log.Errorf(err, "Error matching integration %q with kit %q", integration.Name, kit.Name) Review Comment: Maybe use `Log.ForIntegration(it).Error(...` for consistency. ########## pkg/controller/kameletbinding/monitor.go: ########## @@ -76,14 +77,25 @@ func (action *monitorAction) Handle(ctx context.Context, kameletbinding *v1alpha operatorIDChanged := v1.GetOperatorIDAnnotation(kameletbinding) != "" && (v1.GetOperatorIDAnnotation(kameletbinding) != v1.GetOperatorIDAnnotation(&it)) + sameTraits, err := trait.IntegrationAndBindingSameTraits(&it, kameletbinding) + if err != nil { + return nil, err + } + // Check if the integration needs to be changed expected, err := CreateIntegrationFor(ctx, action.client, kameletbinding) if err != nil { return nil, err } - if !equality.Semantic.DeepDerivative(expected.Spec, it.Spec) || operatorIDChanged { - action.L.Info("Monitor: KameletBinding needs a rebuild") + semanticEquality := equality.Semantic.DeepDerivative(expected.Spec, it.Spec) + + if !semanticEquality || operatorIDChanged || !sameTraits { + action.L.Info( + "Monitor: KameletBinding needs a rebuild", Review Comment: `Monitor: KameletBinding needs a rebuild` -> `KameletBinding needs a rebuild` ########## pkg/controller/kameletbinding/monitor.go: ########## @@ -76,14 +77,25 @@ func (action *monitorAction) Handle(ctx context.Context, kameletbinding *v1alpha operatorIDChanged := v1.GetOperatorIDAnnotation(kameletbinding) != "" && (v1.GetOperatorIDAnnotation(kameletbinding) != v1.GetOperatorIDAnnotation(&it)) + sameTraits, err := trait.IntegrationAndBindingSameTraits(&it, kameletbinding) + if err != nil { + return nil, err + } + // Check if the integration needs to be changed expected, err := CreateIntegrationFor(ctx, action.client, kameletbinding) if err != nil { return nil, err } - if !equality.Semantic.DeepDerivative(expected.Spec, it.Spec) || operatorIDChanged { - action.L.Info("Monitor: KameletBinding needs a rebuild") + semanticEquality := equality.Semantic.DeepDerivative(expected.Spec, it.Spec) + + if !semanticEquality || operatorIDChanged || !sameTraits { + action.L.Info( + "Monitor: KameletBinding needs a rebuild", + "semantic-equality", !semanticEquality, + "operatorid-changed", operatorIDChanged, + "traites-changed", !sameTraits) Review Comment: `traites-changed` -> `traits-changed` ########## e2e/support/test_support.go: ########## @@ -545,6 +545,26 @@ func IntegrationSpecReplicas(ns string, name string) func() *int32 { } } +func IntegrationGeneration(ns string, name string) func() *int64 { + return func() *int64 { + it := Integration(ns, name)() + if it == nil { + return nil + } + return &it.Generation + } +} + +func IntegrationStatusObserverGeneration(ns string, name string) func() *int64 { Review Comment: nit: `IntegrationStatusObserverGeneration` -> `IntegrationStatusObservedGeneration` or even `IntegrationObservedGeneration` as status can be made implicit. ########## pkg/trait/util.go: ########## @@ -29,17 +29,36 @@ import ( user "github.com/mitchellh/go-homedir" "github.com/pkg/errors" "github.com/scylladb/go-set/strset" - + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime/pkg/client" v1 "github.com/apache/camel-k/pkg/apis/camel/v1" + "github.com/apache/camel-k/pkg/apis/camel/v1alpha1" "github.com/apache/camel-k/pkg/client" "github.com/apache/camel-k/pkg/metadata" "github.com/apache/camel-k/pkg/util" "github.com/apache/camel-k/pkg/util/camel" "github.com/apache/camel-k/pkg/util/property" ) +type Unstructured map[string]map[string]interface{} Review Comment: I'd suggest not to used `Unstructured` to avoid confusion with the eponymous type from k8s, maybe `Traits` or `TraitOptions`. -- 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-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
