[PR] fix(tests): fixed flaky Readiness condition with never ready route test in e2e/common/traits/health_test.go [camel-k]
valdar opened a new pull request, #5503: URL: https://github.com/apache/camel-k/pull/5503 Due to how camel works stopping a route using the message bus inside the route can not assure that the route is reported always as stopped, there might be a small amount of time in which it can be reported as running. Since the test is testing the behavior of camel k in the case that an integration is always reported not ready I took another approach by registering a custom healthcheck that force the reported condition to be never ready. **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-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix(tests): fixed flaky Readiness condition with never ready route test in e2e/common/traits/health_test.go [camel-k]
valdar commented on PR #5503: URL: https://github.com/apache/camel-k/pull/5503#issuecomment-2109714704 > LGTM. I wonder if the work in #5450 was not already good enough though. According to the Camel Jira issues, that was the way it had to be fixed. according to this test run https://github.com/apache/camel-k/actions/runs/9065576954/job/24906613292?pr=5119 in particular: ```❌ TestHealthTrait (11m2.94s) Copy catalog camel-catalog-3.8.1 from namespace default Copy integration kit kit-cp136jalfugs739ljsu0 from namespace default Copy integration kit kit-cp138l2lfugs739ljsug from namespace default Setting build timeout to 10m OLM is not available in the cluster. Fallback to regular installation. Camel K installed in namespace test-5bad4aa0-0ccf-48e5-8576-d86e24c00b9a health_test.go:374: Failed after 2.522s. Expected : True to equal : False ``` something was still missing. -- 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
Re: [PR] fix(tests): fixed flaky Readiness condition with never ready route test in e2e/common/traits/health_test.go [camel-k]
lburgazzoli commented on PR #5503: URL: https://github.com/apache/camel-k/pull/5503#issuecomment-2109738304 Just for completeness, the parameters are controlling the behavior of the route controller in the case the a route would fail to start. In this case however, the route starts without errors and ad some point in the future it gets stopped which is outside of the control of the route controller (as the route was successfully started) hence, the route may result up if the health check is triggered before the control bus had a chance to actually stop the route. -- 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
Re: [PR] fix(tests): fixed flaky Readiness condition with never ready route test in e2e/common/traits/health_test.go [camel-k]
valdar commented on PR #5503: URL: https://github.com/apache/camel-k/pull/5503#issuecomment-2112864379 I simplified the route and test a little bit more to be sure that intent is clearer and the test dose not mislead the next poor soul who read it. In so doing I have also found that there were some minor issues (as conditions never actually evaluated) in the startup probe tests and I have tried to fix those as well. -- 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
Re: [PR] fix(tests): fixed flaky Readiness condition with never ready route test in e2e/common/traits/health_test.go [camel-k]
squakez commented on code in PR #5503: URL: https://github.com/apache/camel-k/pull/5503#discussion_r1602756276 ## e2e/common/traits/health_test.go: ## @@ -482,13 +472,15 @@ func TestHealthTrait(t *testing.T) { WithTransform(IntegrationConditionReason, Equal(v1.IntegrationConditionDeploymentReadyReason)), WithTransform(IntegrationConditionMessage, Equal("1/1 ready replicas" - Satisfy(func(is *v1.IntegrationSpec) bool { - if *is.Traits.Health.Enabled == true && *is.Traits.Health.StartupProbeEnabled == true && is.Traits.Health.StartupTimeout == 60 { - return true - } - return false - }) - + g.Expect(IntegrationTraitSpec(t, ctx, ns, name)()).Should( Review Comment: This can be removed. It's more part of the unit test and it's already covered. ## e2e/common/traits/health_test.go: ## @@ -363,9 +362,7 @@ func TestHealthTrait(t *testing.T) { name := RandomizedSuffixName("never-ready") g.Expect(KamelRunWithID(t, ctx, operatorID, ns, "files/NeverReady.java", "--name", name, "-t", "health.enabled=true", - // TODO remove these workaround properties when https://issues.apache.org/jira/browse/CAMEL-20244 is fixed - "-p", "camel.route-controller.unhealthyOnRestarting=true", - "-p", "camel.route-controller.unhealthyOnExhausted=true", + "-p", "camel.health.routesEnabled=false", Review Comment: I think this is defeating the goal of the test. The idea is that the health is enabled on Camel side and we use those endpoints to query the route about it's health condition. I am not sure if I am understanding why this option is needed. ## e2e/common/traits/health_test.go: ## @@ -439,7 +437,7 @@ func TestHealthTrait(t *testing.T) { var r *v1.HealthCheckResponse for h := range c.Pods[0].Health { - if c.Pods[0].Health[h].Name == "camel-routes" && c.Pods[0].Health[h].Status == "DOWN" { + if c.Pods[0].Health[h].Name == "never-ready" && c.Pods[0].Health[h].Status == "DOWN" { Review Comment: This one look weird. If it is expected the Integration name, then, a randomized name variable `name` should be provided instead. Are we sure this condition is really existing? ## e2e/support/test_support.go: ## @@ -1079,6 +1079,16 @@ func IntegrationTraitProfile(t *testing.T, ctx context.Context, ns string, name } } +func IntegrationTraitSpec(t *testing.T, ctx context.Context, ns string, name string) func() *v1.Traits { Review Comment: I'd remove this as well as it makes sense in unit test. In E2E we want to test the behavior instead. ## e2e/common/traits/health_test.go: ## @@ -409,14 +406,15 @@ func TestHealthTrait(t *testing.T) { return false } - return data["check.kind"].(string) == "READINESS" && data["route.status"].(string) == "Stopped" && data["route.id"].(string) == "never-ready" + return r.Status == v1.HealthCheckStatusDown && data["check.kind"].(string) == "READINESS" })) }) t.Run("Startup condition with never ready route", func(t *testing.T) { name := RandomizedSuffixName("startup-probe-never-ready-route") - g.Expect(KamelRunWithID(t, ctx, operatorID, ns, "files/NeverReady.java", "--name", name, "-t", "health.startup-probe-enabled=true", "-t", "health.startup-timeout=60").Execute()).To(Succeed()) + g.Expect(KamelRunWithID(t, ctx, operatorID, ns, "files/NeverReady.java", "--name", name, + "-t", "health.enabled=true", "-t", "health.startup-probe-enabled=true", "-t", "health.startup-timeout=60").Execute()).To(Succeed()) Review Comment: I think `health.enabled` is superfluous if any probe configuration is provided. -- 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
Re: [PR] fix(tests): fixed flaky Readiness condition with never ready route test in e2e/common/traits/health_test.go [camel-k]
valdar commented on code in PR #5503: URL: https://github.com/apache/camel-k/pull/5503#discussion_r1603094323 ## e2e/common/traits/health_test.go: ## @@ -409,14 +406,15 @@ func TestHealthTrait(t *testing.T) { return false } - return data["check.kind"].(string) == "READINESS" && data["route.status"].(string) == "Stopped" && data["route.id"].(string) == "never-ready" + return r.Status == v1.HealthCheckStatusDown && data["check.kind"].(string) == "READINESS" })) }) t.Run("Startup condition with never ready route", func(t *testing.T) { name := RandomizedSuffixName("startup-probe-never-ready-route") - g.Expect(KamelRunWithID(t, ctx, operatorID, ns, "files/NeverReady.java", "--name", name, "-t", "health.startup-probe-enabled=true", "-t", "health.startup-timeout=60").Execute()).To(Succeed()) + g.Expect(KamelRunWithID(t, ctx, operatorID, ns, "files/NeverReady.java", "--name", name, + "-t", "health.enabled=true", "-t", "health.startup-probe-enabled=true", "-t", "health.startup-timeout=60").Execute()).To(Succeed()) Review Comment: It is, but it was explicitly checked (already before this pr) afterwards... but since I remove the check part I will remove this as well. -- 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
Re: [PR] fix(tests): fixed flaky Readiness condition with never ready route test in e2e/common/traits/health_test.go [camel-k]
valdar commented on code in PR #5503: URL: https://github.com/apache/camel-k/pull/5503#discussion_r1603096578 ## e2e/common/traits/health_test.go: ## @@ -439,7 +437,7 @@ func TestHealthTrait(t *testing.T) { var r *v1.HealthCheckResponse for h := range c.Pods[0].Health { - if c.Pods[0].Health[h].Name == "camel-routes" && c.Pods[0].Health[h].Status == "DOWN" { + if c.Pods[0].Health[h].Name == "never-ready" && c.Pods[0].Health[h].Status == "DOWN" { Review Comment: We are sure because is the condition that we explicitly define in files/NeverReady.java -- 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
Re: [PR] fix(tests): fixed flaky Readiness condition with never ready route test in e2e/common/traits/health_test.go [camel-k]
valdar commented on code in PR #5503: URL: https://github.com/apache/camel-k/pull/5503#discussion_r1603104066 ## e2e/common/traits/health_test.go: ## @@ -363,9 +362,7 @@ func TestHealthTrait(t *testing.T) { name := RandomizedSuffixName("never-ready") g.Expect(KamelRunWithID(t, ctx, operatorID, ns, "files/NeverReady.java", "--name", name, "-t", "health.enabled=true", - // TODO remove these workaround properties when https://issues.apache.org/jira/browse/CAMEL-20244 is fixed - "-p", "camel.route-controller.unhealthyOnRestarting=true", - "-p", "camel.route-controller.unhealthyOnExhausted=true", + "-p", "camel.health.routesEnabled=false", Review Comment: That is there to avoid any possible (present or future) interaction and side effect between the custom check we define in files/NeverReady.java and the default checks. At the moment the test makes sense and would work both with and without that property, there are no routes declared in files/NeverReady.java anyway... -- 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
Re: [PR] fix(tests): fixed flaky Readiness condition with never ready route test in e2e/common/traits/health_test.go [camel-k]
valdar merged PR #5503: URL: https://github.com/apache/camel-k/pull/5503 -- 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