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 2023/10/04 11:26:02 UTC
[PR] fix(ctrl): Change reconciliation of int in error [camel-k]
gansheer opened a new pull request, #4793:
URL: https://github.com/apache/camel-k/pull/4793
## Description
When the integration is in Error with the Kit condition in error, then if the integration kit referenced is still in error then finish the reconciliation process without any change to the integration.
Fix the health trait to avoid panic errors.
<!-- 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
Change reconciliation of integration in error
```
--
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(ctrl): Change reconciliation of int in error [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #4793:
URL: https://github.com/apache/camel-k/pull/4793#discussion_r1347397830
##########
pkg/controller/integration/monitor.go:
##########
@@ -81,6 +81,12 @@ func (action *monitorAction) Handle(ctx context.Context, integration *v1.Integra
integration.Status.IntegrationKit.Namespace, integration.Status.IntegrationKit.Name, err)
}
+ // If integration is in error and its kit is also in error then integration does not change
+ if isInIntegrationKitFailed(integration.Status) &&
Review Comment:
Yes. If there is no changes in the build, then, it must reuse the Integration kit. Imagine you change the definition of the Java DSL because you forgot a `;`. The IntegrationKit was good, but the runtime fails because the syntax is wrong. You would amend it with such a `;` and it would not need to rebuild a kit.
--
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(ctrl): Change reconciliation of int in error [camel-k]
Posted by "gansheer (via GitHub)" <gi...@apache.org>.
gansheer commented on PR #4793:
URL: https://github.com/apache/camel-k/pull/4793#issuecomment-1765890757
>
> do we expose in some ways (conditions, phase) at what stage the reconciliation stops ?
None that I could tell for the Integration CR. As for the IntegrationKit CR its reconciliation after 5 failed builds and I suppose you could consider the `status.failure` field as an indication that the reconciliation is stopped.
--
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(ctrl): Change reconciliation of int in error [camel-k]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4793:
URL: https://github.com/apache/camel-k/pull/4793#issuecomment-1746688697
:camel: **Thank you for contributing!**
Code Coverage Report :heavy_check_mark: - Coverage unchanged.
--
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(ctrl): Change reconciliation of int in error [camel-k]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on code in PR #4793:
URL: https://github.com/apache/camel-k/pull/4793#discussion_r1347360491
##########
pkg/controller/integration/monitor.go:
##########
@@ -81,6 +81,12 @@ func (action *monitorAction) Handle(ctx context.Context, integration *v1.Integra
integration.Status.IntegrationKit.Namespace, integration.Status.IntegrationKit.Name, err)
}
+ // If integration is in error and its kit is also in error then integration does not change
+ if isInIntegrationKitFailed(integration.Status) &&
Review Comment:
> Also, is there a case where the Integration get changed but it does not change the IntegrationKit ?
This is pretty much what happen every time you change a route without introducing any new component, in that case camel-k should re-use the integration kit previously built
--
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(ctrl): Change reconciliation of int in error [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #4793:
URL: https://github.com/apache/camel-k/pull/4793#discussion_r1347243569
##########
pkg/controller/integration/monitor.go:
##########
@@ -81,6 +81,12 @@ func (action *monitorAction) Handle(ctx context.Context, integration *v1.Integra
integration.Status.IntegrationKit.Namespace, integration.Status.IntegrationKit.Name, err)
}
+ // If integration is in error and its kit is also in error then integration does not change
+ if isInIntegrationKitFailed(integration.Status) &&
Review Comment:
I am not entirely sure, but I think that if we add this, here, then, the user won't be able to edit its Integration and have a rebuild with the amended changes.
--
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(ctrl): Change reconciliation of int in error [camel-k]
Posted by "gansheer (via GitHub)" <gi...@apache.org>.
gansheer commented on PR #4793:
URL: https://github.com/apache/camel-k/pull/4793#issuecomment-1748881078
I am moving it back to draft to make a first PR with E2E test on existing behavior.
--
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(ctrl): Change reconciliation of int in error [camel-k]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on code in PR #4793:
URL: https://github.com/apache/camel-k/pull/4793#discussion_r1347360491
##########
pkg/controller/integration/monitor.go:
##########
@@ -81,6 +81,12 @@ func (action *monitorAction) Handle(ctx context.Context, integration *v1.Integra
integration.Status.IntegrationKit.Namespace, integration.Status.IntegrationKit.Name, err)
}
+ // If integration is in error and its kit is also in error then integration does not change
+ if isInIntegrationKitFailed(integration.Status) &&
Review Comment:
> Also, is there a case where the Integration get changed but it does not change the IntegrationKit ?
This is pretty much what happen every time you change a route without introducing any new component
--
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(ctrl): Change reconciliation of int in error [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez merged PR #4793:
URL: https://github.com/apache/camel-k/pull/4793
--
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(ctrl): Change reconciliation of int in error [camel-k]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4793:
URL: https://github.com/apache/camel-k/pull/4793#issuecomment-1759260662
:camel: **Thank you for contributing!**
Code Coverage Report :heavy_check_mark: - Coverage unchanged.
--
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(ctrl): Change reconciliation of int in error [camel-k]
Posted by "gansheer (via GitHub)" <gi...@apache.org>.
gansheer commented on code in PR #4793:
URL: https://github.com/apache/camel-k/pull/4793#discussion_r1347339554
##########
pkg/controller/integration/monitor.go:
##########
@@ -81,6 +81,12 @@ func (action *monitorAction) Handle(ctx context.Context, integration *v1.Integra
integration.Status.IntegrationKit.Namespace, integration.Status.IntegrationKit.Name, err)
}
+ // If integration is in error and its kit is also in error then integration does not change
+ if isInIntegrationKitFailed(integration.Status) &&
Review Comment:
It actually works, because if we change the Integration CR then a new integration kit gets created and built. I will add this in the test.
Is there a case where the IntegrationKit could be fixed without any change the Integration CR ?
Also, is there a case where the Integration get changed but it does not change the IntegrationKit ?
--
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(ctrl): Change reconciliation of int in error [camel-k]
Posted by "gansheer (via GitHub)" <gi...@apache.org>.
gansheer commented on code in PR #4793:
URL: https://github.com/apache/camel-k/pull/4793#discussion_r1347339554
##########
pkg/controller/integration/monitor.go:
##########
@@ -81,6 +81,12 @@ func (action *monitorAction) Handle(ctx context.Context, integration *v1.Integra
integration.Status.IntegrationKit.Namespace, integration.Status.IntegrationKit.Name, err)
}
+ // If integration is in error and its kit is also in error then integration does not change
+ if isInIntegrationKitFailed(integration.Status) &&
Review Comment:
It actually works, because if we change the Integration CR then a new integration kit gets created and built. I will add this in the test.
Is there a case where the IntegrationKit could be fixed without any change the Integration CR ?
--
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(ctrl): Change reconciliation of int in error [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #4793:
URL: https://github.com/apache/camel-k/pull/4793#discussion_r1347399285
##########
pkg/controller/integration/monitor.go:
##########
@@ -81,6 +81,12 @@ func (action *monitorAction) Handle(ctx context.Context, integration *v1.Integra
integration.Status.IntegrationKit.Namespace, integration.Status.IntegrationKit.Name, err)
}
+ // If integration is in error and its kit is also in error then integration does not change
+ if isInIntegrationKitFailed(integration.Status) &&
Review Comment:
BTW, the above scenario is a great candidate for an E2E test...
--
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(ctrl): Change reconciliation of int in error [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #4793:
URL: https://github.com/apache/camel-k/pull/4793#issuecomment-1765714939
@lburgazzoli given the new E2E test provided by @gansheer, I think we are safe to merge this PR. Basically we stop the latest reconciliation loop to apply traits when both the Integration and the Kit are in an error state. Hence, there is no reason to continue applying trait logic. Do you have any further concern?
--
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(ctrl): Change reconciliation of int in error [camel-k]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4793:
URL: https://github.com/apache/camel-k/pull/4793#issuecomment-1748338461
:camel: **Thank you for contributing!**
Code Coverage Report :heavy_check_mark: - Coverage unchanged.
--
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(ctrl): Change reconciliation of int in error [camel-k]
Posted by "gansheer (via GitHub)" <gi...@apache.org>.
gansheer commented on code in PR #4793:
URL: https://github.com/apache/camel-k/pull/4793#discussion_r1347361749
##########
pkg/controller/integration/monitor.go:
##########
@@ -81,6 +81,12 @@ func (action *monitorAction) Handle(ctx context.Context, integration *v1.Integra
integration.Status.IntegrationKit.Namespace, integration.Status.IntegrationKit.Name, err)
}
+ // If integration is in error and its kit is also in error then integration does not change
+ if isInIntegrationKitFailed(integration.Status) &&
Review Comment:
Can the route be the source of an IntegrationKit not building ?
--
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(ctrl): Change reconciliation of int in error [camel-k]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on PR #4793:
URL: https://github.com/apache/camel-k/pull/4793#issuecomment-1763960821
> So let's recap a little:
>
> The solution proposed is to stop early on the process (before it get to the apply part) when the following conditions are ALL met:
>
> * Integration status contains a condition `IntegrationConditionKitAvailable` with status `Error`
>
> * Integration status phase is `Error`
>
> * IntegrationKit status phase is `Error` (the one declared by the Integration)
>
do we expose in some ways (conditions, phase) at what stage the reconciliation stops ?
--
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(ctrl): Change reconciliation of int in error [camel-k]
Posted by "gansheer (via GitHub)" <gi...@apache.org>.
gansheer commented on PR #4793:
URL: https://github.com/apache/camel-k/pull/4793#issuecomment-1763946473
So let's recap a little:
The solution proposed is to stop early on the process (before it get to the apply part) when the following conditions are ALL met:
* Integration status contains a condition `IntegrationConditionKitAvailable` with status `Error`
* Integration status phase is `Error`
* IntegrationKit status phase is `Error` (the one declared by the Integration)
From the [e2e test](https://github.com/apache/camel-k/pull/4815) I gathered the following:
* an Integration with invalid or unknown dependencies : any fix will generate a new Integration Kit => solution OK
* an Integration with unknown component in route : the IntegrationKit is not generated until all component are known => solution OK
* an Integration with route compilation error : the IntegrationKit was already generated successfully and any change in the route won't have any impact on the IntegrationKit => solution OK
I really don't see any use case where an IntegrationKit in Error will be able to recover without the creation of a new one. Is there such a process or something that could act directly on an IntegrationKit CR without acting from the Integration CR ?
@squakez @lburgazzoli
--
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