Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix #4948 part of #3397 #5119

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix #4948 part of #3397 #5119

wants to merge 2 commits into from

Conversation

valdar
Copy link
Member

@valdar valdar commented Jan 29, 2024

Fix for #4948 that is part of #3397

A platformcontroller command is added as subcommand of kamel; it runs an operator like the operator subcomand does, but platformcontroller would just handle IntegrationPlatform CRs. All the other CRDs are handled by operator as before.

These installation methods have been amended accordingly:

  • install command
  • olm bundle
  • helm charts

e2e tests has been ran locally as well.

Release Note

NONE

@valdar valdar added the kind/feature New feature or request label Jan 29, 2024
@valdar valdar mentioned this pull request Jan 30, 2024
Merged
@valdar valdar force-pushed the issue/3397 branch 5 times, most recently from 5be3431 to 15b484a Compare January 31, 2024 11:11
squakez
squakez reviewed Jan 31, 2024
View reviewed changes
Copy link
Contributor

@squakez squakez left a 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.

pkg/cmd/platformcontroller/platformcontroller.go Show resolved Hide resolved
@valdar valdar mentioned this pull request Feb 1, 2024
Merged
@valdar
Copy link
Member Author

valdar commented Feb 3, 2024 •
edited

What about these tests:

❌ TestMetrics (3m0.27s)
❌ TestMetrics/reconciliation_duration_metric (10ms)

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? :/

@squakez
Copy link
Contributor

squakez commented Feb 5, 2024

What about these tests:

❌ TestMetrics (3m0.27s) ❌ TestMetrics/reconciliation_duration_metric (10ms)

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:

Expect(platformReconciled).NotTo(BeNil())

I suspect that the operator perform and provide metrics which we may have lost if running the reconciliation loop outside the main process.

@valdar valdar force-pushed the issue/3397 branch 4 times, most recently from 7cfce32 to 82e6c57 Compare February 15, 2024 21:21
@github-actions GitHub Actions
Copy link
Contributor

⚠️ Unit test coverage report - coverage decreased from 35.8% to 35.7% (-0.1%)

1 similar comment
@github-actions GitHub Actions
Copy link
Contributor

⚠️ Unit test coverage report - coverage decreased from 35.8% to 35.7% (-0.1%)

@lburgazzoli
Copy link
Contributor

@valdar @rinaldodev mind rebasing ?

@lburgazzoli lburgazzoli mentioned this pull request Feb 27, 2024
Merged
lburgazzoli
lburgazzoli reviewed Feb 27, 2024
View reviewed changes
build/test.log Outdated Show resolved Hide resolved
@squakez
Copy link
Contributor

squakez commented Mar 19, 2024 •
edited

I'm not complaining about having to run them locally, I'm just noticing the OSX build is almost always failing so I wonder what is the value of such test. Now I can run the test locally on my linux box and reporting them as ok but, what I've testes is not what has failed so people may spend time waiting and trying to find out why things are not working on OSX whereas it is an environment issue.

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.

@lburgazzoli
Copy link
Contributor

I agree that we need some docs but since this PR is part of a larger issue and the rationale was briefly explained in #3397 (comment), then I wonder if the doc could be something we do as a separate PR.

In theory we want to release 2.3 by the end of the month. If we ends up merging this we'd include in 2.3 which, IIUC would not be either complete as still missing pieces (beside documentation). My suggestion would be to wait the release of 2.3 then, and to merge right after so you can focus on the rest of development to be ready for 2.4. Or, if you want to push this in 2.3 complete with the required documentation.

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

@squakez
Copy link
Contributor

squakez commented Mar 19, 2024

I agree that we need some docs but since this PR is part of a larger issue and the rationale was briefly explained in #3397 (comment), then I wonder if the doc could be something we do as a separate PR.

In theory we want to release 2.3 by the end of the month. If we ends up merging this we'd include in 2.3 which, IIUC would not be either complete as still missing pieces (beside documentation). My suggestion would be to wait the release of 2.3 then, and to merge right after so you can focus on the rest of development to be ready for 2.4. Or, if you want to push this in 2.3 complete with the required documentation.

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.

@squakez
Copy link
Contributor

squakez commented Mar 20, 2024

@valdar please rebase against main again. The Quarkus native checks have been fixed, so you won't need to run them manually any longer. Thanks.

@github-actions GitHub Actions
Copy link
Contributor

⚠️ Unit test coverage report - coverage decreased from 37.3% to 37.2% (-0.1%)

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 9, 2024

⚠️ Unit test coverage report - coverage decreased from 37.8% to 37.7% (-0.1%)

@github-actions GitHub Actions
Copy link
Contributor

⚠️ Unit test coverage report - coverage decreased from 37.8% to 37.7% (-0.1%)

@valdar valdar force-pushed the issue/3397 branch 2 times, most recently from 9460bec to 046ed3e Compare April 16, 2024 10:34
@github-actions GitHub Actions
Copy link
Contributor

⚠️ Unit test coverage report - coverage decreased from 38% to 37.9% (-0.1%)

@github-actions GitHub Actions
Copy link
Contributor

⚠️ Unit test coverage report - coverage decreased from 38% to 37.9% (-0.1%)

@github-actions GitHub Actions
Copy link
Contributor

⚠️ Unit test coverage report - coverage decreased from 38% to 37.9% (-0.1%)

@github-actions GitHub Actions
Copy link
Contributor

⚠️ Unit test coverage report - coverage decreased from 38% to 37.9% (-0.1%)

@github-actions GitHub Actions
Copy link
Contributor

⚠️ Unit test coverage report - coverage decreased from 38% to 37.9% (-0.1%)

@valdar valdar force-pushed the issue/3397 branch 2 times, most recently from fb907a5 to 4457b27 Compare April 23, 2024 08:48
@github-actions GitHub Actions
Copy link
Contributor

⚠️ Unit test coverage report - coverage decreased from 38% to 37.9% (-0.1%)

@lburgazzoli lburgazzoli mentioned this pull request Apr 24, 2024
Merged
@valdar valdar mentioned this pull request Apr 24, 2024
Merged
@valdar valdar force-pushed the issue/3397 branch 2 times, most recently from 283699e to f77f281 Compare May 13, 2024 14:59
@valdar
fix(apache#4948): Move handling of IntegrationPlatform CR to a separa…
e5f4ae5 
…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.
@valdar
fix(apache#4948): Refactor common logic between operator and platform…
0901791 
…controller
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lburgazzoli lburgazzoli lburgazzoli approved these changes

@gansheer gansheer gansheer left review comments

@squakez squakez Awaiting requested review from squakez

Assignees
No one assigned
Labels
kind/feature New feature or request trigger native test Use this label in PR when you want to trigger Quarkus Native tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@valdar @squakez @lburgazzoli @gansheer