Re: [PR] fix(tests): fixed flaky Readiness condition with never ready route test in e2e/common/traits/health_test.go [camel-k]

2024-05-17 Thread via GitHub 


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

Re: [PR] fix(tests): fixed flaky Readiness condition with never ready route test in e2e/common/traits/health_test.go [camel-k]

2024-05-16 Thread via GitHub 


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]

2024-05-16 Thread via GitHub 


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]

2024-05-16 Thread via GitHub 


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]

2024-05-16 Thread via GitHub 


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]

2024-05-15 Thread via GitHub 


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]

2024-05-14 Thread via GitHub 


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]

2024-05-14 Thread via GitHub 


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

[PR] fix(tests): fixed flaky Readiness condition with never ready route test in e2e/common/traits/health_test.go [camel-k]

2024-05-14 Thread via GitHub 


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