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.

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

Ref #3023: Add native compilation with sources #4021

Merged
merged 1 commit into from
Feb 17, 2023

Conversation

essobedo
Copy link
Contributor

@essobedo essobedo commented Feb 1, 2023

fixes #3023

Motivation

Some DSLs need to have the sources present at compile time in native mode to be able to generate classes and include them in the native image.

Modifications

  • Add the support of the loader's metadata to get the expected behavior of a language according to the Camel Catalog
  • Add the sources to include at build time in the kit and build for native mode only
  • Copy the sources to include at build time in the directory maven/src/main/resources/routes
  • Add an integration test for the native mode for each language
  • Prevent creating a ConfigMap when the source is already included in the native image
  • Get rid of the configuration of sources already included in the native image
  • Prevent configuring a volume for sources already included in the native image
  • Add the support of the JavaShell DSL
  • Adapt the bash scripts used by the build to make them compatible with MacOS to launch the E2E tests for native mode on MacOS runners
  • Split E2E tests for native mode in 2 parts (based on memory consumption) to limit the duration of the build
  • Adapt the different timeouts to MacOS runners as they are much slower compared to Unbuntu runners

Release Note

feat: Java, Groovy, and Kotlin native support 

pkg/builder/types.go Outdated Show resolved Hide resolved
@squakez
Copy link
Contributor

squakez commented Feb 1, 2023

BTW, if you want to run the native test suite as well, you need to call the native workflow directly on the branch as we have moved them into nightly releases (they used to eat up a lot of memory):

- name: Native smoke tests

@essobedo essobedo force-pushed the 3023/compile-with-sources branch 3 times, most recently from 1870e16 to ea59a8d Compare February 1, 2023 17:30
@squakez
Copy link
Contributor

squakez commented Feb 2, 2023

As I'm working on some ideas around Camel K 2.0 (gitops, being able to build any Camel application, ...), let me share with you something I've been thinking lately. I may probably create an issue with some visual support, but hopefully can help you in the work you're doing.

I think that instead of passing the whole content of a source along the different CRs, we should have the concept of a "generated Maven project" (it may even come from camel export - Camel JBang instead of our own logic). This is a standard Maven project that could be even stored in a Git repository. The project should be generated by the Operator when an Integration is submitted (here we can think to provide the Source or the Git repo URL). In the first case a project generation is triggered (ie, via Camel JBang) and we should store temporary the result in a workspace directory. The project generation may be controlled via a new Custom Resource and a reconciliation loop.

Once the Project is ready, the Build can kick off. The Build has to be fed with the workspace directory and the rest should be proceeding more or less the same way it is now (in order to be able to reuse the incremental image kit logic).

@squakez
Copy link
Contributor

squakez commented Feb 2, 2023

Details of the proposal defined in the previous comment exposed in #4024

@essobedo
Copy link
Contributor Author

essobedo commented Feb 2, 2023

Your idea is great however it is scoped to Camel K 2.0 while I would expect a solution for Camel K 1.17, don't you believe that it could be an acceptable approach for 1.17?

@essobedo essobedo force-pushed the 3023/compile-with-sources branch 2 times, most recently from d361b70 to e11efff Compare February 2, 2023 21:39
pkg/trait/quarkus.go Show resolved Hide resolved
@@ -216,6 +265,9 @@ func (t *quarkusTrait) newIntegrationKit(e *Environment, packageType traitv1.Qua
Traits: propagateKitTraits(e),
}

if packageType == traitv1.NativePackageType {
kit.Spec.Sources = propagateSourcesRequiredAtBuildTime(e)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 good to include only when native

pkg/trait/trait_test.go Show resolved Hide resolved
@squakez
Copy link
Contributor

squakez commented Feb 3, 2023

Your idea is great however it is scoped to Camel K 2.0 while I would expect a solution for Camel K 1.17, don't you believe that it could be an acceptable approach for 1.17?

I guess you mean 1.12.0 (1.17 is the runtime). It could work, but mind that we could apply an heavy refactoring when moving to 2.0.

@squakez squakez mentioned this pull request Feb 3, 2023
@essobedo essobedo force-pushed the 3023/compile-with-sources branch from e11efff to 10706ec Compare February 3, 2023 11:45
@essobedo
Copy link
Contributor Author

essobedo commented Feb 3, 2023

@squakez your remarks have been addressed.
What remains:

  • The test TestOLMAutomaticUpgrade fails and it is not clear to me why. I see this test fails in another build so I guess it is not related to these changes.
  • There are some tests failing due to what you fixed in this PR fix(test): use Kamelet catalog #4029
  • Some tests that I have added for native mode due to a lack of memory, maybe I should only keep the less greedy, WDYT?

@squakez
Copy link
Contributor

squakez commented Feb 3, 2023

Cool, I still need to have a deeper look, but in the while, about the native test I suggest you move them into a separate workflow (similar of what we used to have before) so that they won't timeout because the rest of the common already takes almost 3 hours and I bet, native only now may take a few more hours.

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.

Good for me. Let's see how the checks perform.

pkg/trait/trait_test.go Outdated Show resolved Hide resolved
Copy link
Member

@astefanutti astefanutti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

@essobedo essobedo force-pushed the 3023/compile-with-sources branch 4 times, most recently from 2e7c1aa to 8f5d4fa Compare February 3, 2023 15:22
@essobedo essobedo marked this pull request as ready for review February 3, 2023 15:31
@essobedo essobedo force-pushed the 3023/compile-with-sources branch from 8f5d4fa to 89579af Compare February 3, 2023 17:57
@essobedo
Copy link
Contributor Author

essobedo commented Feb 6, 2023

Infortunatelly the native E2E test cannot pass on the GitHub CI because it requires more than 6 Go of RAM, so I need to remove it.

@essobedo essobedo force-pushed the 3023/compile-with-sources branch from 89579af to 5716960 Compare February 6, 2023 08:18
@squakez
Copy link
Contributor

squakez commented Feb 6, 2023

Infortunatelly the native E2E test cannot pass on the GitHub CI because it requires more than 6 Go of RAM, so I need to remove it.

Have you rebased it? it seems there was some failure I've fixed in #4022 - we need to find a way to run the native check smoothly anyhow, maybe we can run it as an additional nightly task right after the release task (to avoid resource consumption conflict).

@essobedo essobedo force-pushed the 3023/compile-with-sources branch 3 times, most recently from 0ea14eb to eca430c Compare February 6, 2023 13:37
@essobedo essobedo force-pushed the 3023/compile-with-sources branch 13 times, most recently from c5e02f2 to aaf89ad Compare February 15, 2023 08:16
@gansheer gansheer mentioned this pull request Feb 15, 2023
@essobedo essobedo force-pushed the 3023/compile-with-sources branch 3 times, most recently from c4b6d90 to b462918 Compare February 16, 2023 05:58
@essobedo
Copy link
Contributor Author

Sounds good to be reviewed now, the build native / install-native only failed during the cleanup phase, the tests pass

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.

All good but a little change to avoid affecting the rest of testing. Great work!

@@ -23,11 +23,12 @@ runs:
steps:
- id: install-cluster
name: Install Cluster
uses: container-tools/kind-action@v1
uses: container-tools/kind-action@61f1afd4807b0dac84f3232ec99e45c63701d220
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can merge with this commit id, but, please, create a follow up issue to release the kind-action and update here accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it is #4063

env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

on:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we have verified for this release, let's change this to run only on a nightly schedule to avoid such a high resource consumption process for every PR or merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, as you like but I did my best to avoid increasing the overall duration of a full build as explained in this comment #4021 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and it's a great effort indeed. However, native workflow is too much resource consuming and we don't have enough capacity to run at every github action. We still have a daily view of what's going on, though.

cancel-in-progress: true

jobs:
install-native-high-memory:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 2 different jobs?

Copy link
Contributor Author

@essobedo essobedo Feb 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially, it was to be able to launch the "lightest" tests on a Linux runner but they still need more than what a Linux runner can offer. Now, it is still interesting as they are launched in parallel so it allows having a duration that is equivalent to the duration of common / common-it which also means that the overall duration of a full build is not really affected.


// TestTimeoutVeryLong should be used only for testing native builds.
var TestTimeoutVeryLong = 90 * time.Minute
var TestTimeoutVeryLong = 60 * time.Minute
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We better provide a specific timeout for the native test, no needs to use global variables. With this change we are slowing down also any other test that would expect those previous values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, actually it is the opposite, I reduced the duration of the timeout by 30 minutes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was more concerned about TestTimeoutLong which is increased a little bit. I wonder if we could instead introduce a different timeout variables for native (ie, NativeTestTimeoutLong) or either just use the quantity of minutes. But we refine this in a following iteration, not a blocker for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I had to change a bit the timeout because, on MacOS runners, everything is much slower probably because it needs a container runtime which is not the case with Linux. However in the end, If a test works as expected the timeout set should not have any effect on the duration of the test, it is only when something goes wrong the state is not the expected one.

@essobedo essobedo force-pushed the 3023/compile-with-sources branch from b462918 to 839aceb Compare February 16, 2023 19:08
@essobedo essobedo requested a review from squakez February 16, 2023 19:08
@squakez
Copy link
Contributor

squakez commented Feb 17, 2023

Good for me. As it's all green let's merge this :) - we will understand if moving the native workflow to a daily basis if we see it start becoming too much resource consuming.


// TestTimeoutVeryLong should be used only for testing native builds.
var TestTimeoutVeryLong = 90 * time.Minute
var TestTimeoutVeryLong = 60 * time.Minute
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was more concerned about TestTimeoutLong which is increased a little bit. I wonder if we could instead introduce a different timeout variables for native (ie, NativeTestTimeoutLong) or either just use the quantity of minutes. But we refine this in a following iteration, not a blocker for this PR.

@squakez squakez merged commit 3f7522d into main Feb 17, 2023
@squakez
Copy link
Contributor

squakez commented Feb 17, 2023

Excellent work. Great feature to promote for 1.12.0 release!

@essobedo essobedo deleted the 3023/compile-with-sources branch February 17, 2023 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Camel-K Java native support - native run of a java route fails
4 participants