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

Closed
wants to merge 2 commits into from
Closed

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
Loading

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 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 added 2 commits May 13, 2024 17:22
@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
@valdar valdar mentioned this pull request Jul 23, 2024
Closed
@valdar
Copy link
Member Author

valdar commented Jul 23, 2024

This work has been moved to a branch under camel-k project: https://github.com/apache/camel-k/tree/issue/3397 and another PR has been opened #5701

@valdar valdar closed this Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gansheer gansheer gansheer left review comments

@lburgazzoli lburgazzoli lburgazzoli approved these changes

@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.
4 participants
@valdar @squakez @lburgazzoli @gansheer