-
Notifications
You must be signed in to change notification settings - Fork 340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix #4948 part of #3397 #5119
base: main
Are you sure you want to change the base?
fix #4948 part of #3397 #5119
Conversation
5be3431
to
15b484a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main concern I have is about code duplication (maintainability) and performance loss. About the first, I think it would be a matter of abstracting the code from the operator and be able to call it from different places.
About performances I guess it's a bit more complicated. IIUC, so far we're having two separate Pods with 2 separate caches. I honestly don't think that the platform controller has to watch at any resource at all, but only owns the IntegrationPlatform resource. If it requires to watch all those resources for any reason, then, we should understand if we can share the cache somehow.
Additionally, I wonder if instead of having a new deployment, we can simply have a sidecar container to take care of controlling this part.
What about these tests: ❌ TestMetrics (3m0.27s) they seems flaky to me, first run they were passing, they are passing locally, but now failing in CI :/ They are also marked as "problematic" but CI runs without the skip problematic config? :/ |
No, I don't think they are flaky. It fails at line:
I suspect that the operator perform and provide metrics which we may have lost if running the reconciliation loop outside the main process. |
7cfce32
to
82e6c57
Compare
|
1 similar comment
|
@valdar @rinaldodev mind rebasing ? |
I've commented this aspect already some time ago in the comment: #5119 (comment) apologies if it was not clear enough. I'm taking the task to notify this to any PR that may affect the Quarkus native checks. Unfortunately, while they are not fixed, the best way to handle this is to run them manually and on local contributors environments. By the way, just to make this clear, I'm doing the same for my own PRs and I'll have to do this process at the time of cutting the release. |
I think this is more for 3.x than 2.x, wonder if we should branch 2.x now and move main toward 3.x |
No problem for me. Mind that there may be some work to perform on the actions as well (nightly upgrades, releases, etc...) and likely this has to be communicated and accepted by the community before. |
@valdar please rebase against |
|
|
|
9460bec
to
046ed3e
Compare
|
|
|
|
|
fb907a5
to
4457b27
Compare
|
283699e
to
f77f281
Compare
…te operator. Add a separate platformcontroller subcommand to kamel, amend install command and other installations (OLM, kustomize, helm) as needed. The platformcontroller works as the operator command but runs an operator that handles just the IntegrationPlatform crd; the operator dose not manage IntegrationPlatform crd any more.
Fix for #4948 that is part of #3397
A
platformcontroller
command is added as subcommand ofkamel
; it runs an operator like theoperator
subcomand does, but platformcontroller would just handleIntegrationPlatform
CRs. All the other CRDs are handled by operator as before.These installation methods have been amended accordingly:
install
commande2e tests has been ran locally as well.
Release Note