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

removing build time dependencies from the runtime #22501

Merged
merged 1 commit into from
Aug 21, 2023
Merged

removing build time dependencies from the runtime #22501

merged 1 commit into from
Aug 21, 2023

Conversation

shawkins
Copy link
Contributor

@vmuzikar all of the other dependencies are transitive from these.

Closes #22496

@shawkins shawkins requested review from a team as code owners August 16, 2023 16:41
@vmuzikar
Copy link
Contributor

@shawkins Thank you, will review tomorrow.

@ASzc Can you please check if this works for prod process?

@ASzc
Copy link
Contributor

ASzc commented Aug 16, 2023

LGTM. I don't think these pom changes will break anything prod-wise, and I confirmed that they remove the excess jars from the produced zip

@ghost ghost added the flaky-test label Aug 16, 2023
@ghost ghost mentioned this pull request Aug 16, 2023
Closed
@vmuzikar vmuzikar self-assigned this Aug 17, 2023
vmuzikar
vmuzikar reviewed Aug 17, 2023
View reviewed changes
operator/pom.xml Outdated Show resolved Hide resolved
@shawkins shawkins requested a review from vmuzikar August 17, 2023 09:13
vmuzikar
vmuzikar previously approved these changes Aug 17, 2023
View reviewed changes
Copy link
Contributor

@vmuzikar vmuzikar left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@shawkins Can you please backport this as well?

@vmuzikar vmuzikar enabled auto-merge (squash) August 17, 2023 09:54
@vmuzikar
Copy link
Contributor

OLM tests seem to fail. Let's see if it's just some instability...

@shawkins
Copy link
Contributor Author

OLM tests seem to fail. Let's see if it's just some instability...

Unfortunately since nothing else was already marked as optional, this probably means that quarkus-operator-sdk-bundle-generator is currently required at runtime as well - @metacosm can you confirm this?

metacosm
metacosm reviewed Aug 17, 2023
View reviewed changes
operator/pom.xml
<dependency>
<groupId>io.fabric8</groupId>
<artifactId>crd-generator-api</artifactId>
</dependency>
<dependency>
<groupId>io.fabric8</groupId>
<artifactId>crd-generator-apt</artifactId>
Copy link

Choose a reason for hiding this comment

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

Why do you need this dependency, actually?

Copy link
Contributor Author

@shawkins shawkins Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

At the very least it's still transitively bringing in the sundrio dependencies. I'm not sure historically why crd-generator-apt was needed.

It looks like instead of the apt dependency we could separtely use:
generator-annotations (for the Required annotation)
crd-generator-api (for the SchemaSwap annotation - it's confusing why this is not in generator-annotations)
builder-annotations (but there's no managed version of this provided by the poms we're currently importing)

Since it looks like this can be at the provided scope, I'd be in favor of leaving it as is unless you think it could be introducing other problems.

Any thoughts on quarkus-operator-sdk-bundle-generator?

auto-merge was automatically disabled August 17, 2023 12:37

Head branch was pushed to by a user without write access

@shawkins
Copy link
Contributor Author

Separating off the quarkus-operator-sdk-bundle-generator change. It will likely need to be handled under https://issues.redhat.com/browse/QUARKUS-2279

@shawkins
Copy link
Contributor Author

@vmuzikar let's move ahead with this one without dealing with quarkus-operator-sdk-bundle-generator

@metacosm
Copy link

The bundle dependency shouldn't be needed at runtime. Do you have a pointer on what is failing if the dependency isn't there?

@shawkins
Copy link
Contributor Author

The bundle dependency shouldn't be needed at runtime. Do you have a pointer on what is failing if the dependency isn't there?

The prior test runs with the dependency marked as optional and filtered from the quarkus lib. The only the local tests passed, the tests running the image failed.

@metacosm
Copy link

The bundle dependency shouldn't be needed at runtime. Do you have a pointer on what is failing if the dependency isn't there?

The prior test runs with the dependency marked as optional and filtered from the quarkus lib. The only the local tests passed, the tests running the image failed.

Do you have a link, please? Not sure where to look in all the tests that you run…

@metacosm
Copy link

Hmm, looking at the code, the module might actually be needed if you use the CSV metadata annotations to control how the CSV files are generated.

@shawkins
Copy link
Contributor Author

Hmm, looking at the code, the module might actually be needed if you use the CSV metadata annotations to control how the CSV files are generated.

Yes that's our case.

Here's a failed run - https://github.com/keycloak/keycloak/actions/runs/5888994162/job/15971593804 unfortunately there's not much to see in that log because we have additional logging/logic that's looking for failed keycloaks, but not for a failing operator.

@metacosm
Copy link

Hmm, looking at the code, the module might actually be needed if you use the CSV metadata annotations to control how the CSV files are generated.

Yes that's our case.

Here's a failed run - https://github.com/keycloak/keycloak/actions/runs/5888994162/job/15971593804 unfortunately there's not much to see in that log because we have additional logging/logic that's looking for failed keycloaks, but not for a failing operator.

Hmm, that said, the annotations should only be needed at build time but there is a SharedCSVMetadata interface which might be used to provide shared metadata across reconcilers, so this is more likely to be the issue.

@shawkins
Copy link
Contributor Author

Hmm, that said, the annotations should only be needed at build time but there is a SharedCSVMetadata interface which might be used to provide shared metadata across reconcilers, so this is more likely to be the issue.

Running this locally gave me the following stacktrace from the operator pod:

Exception in thread "main" java.lang.reflect.InvocationTargetException
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:568)
        at io.quarkus.bootstrap.runner.QuarkusEntryPoint.doRun(QuarkusEntryPoint.java:61)
        at io.quarkus.bootstrap.runner.QuarkusEntryPoint.main(QuarkusEntryPoint.java:32)
Caused by: java.lang.NoClassDefFoundError: io/quarkiverse/operatorsdk/bundle/runtime/BundleGenerationConfiguration
        at io.quarkus.runtime.generated.Config.<clinit>(Unknown Source)
        at io.quarkus.runner.ApplicationImpl.<clinit>(Unknown Source)
        at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
        at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
        at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480)
        at io.quarkus.runtime.Quarkus.run(Quarkus.java:70)
        at io.quarkus.runtime.Quarkus.run(Quarkus.java:44)
        at io.quarkus.runtime.Quarkus.run(Quarkus.java:124)
        at io.quarkus.runner.GeneratedMain.main(Unknown Source)
        ... 6 more
Caused by: java.lang.ClassNotFoundException: io.quarkiverse.operatorsdk.bundle.runtime.BundleGenerationConfiguration
        at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)
        at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
        at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:525)
        at io.quarkus.bootstrap.runner.RunnerClassLoader.loadClass(RunnerClassLoader.java:115)
        at io.quarkus.bootstrap.runner.RunnerClassLoader.loadClass(RunnerClassLoader.java:65)

@metacosm
Copy link

I had been wondering about this, actually, since the config class is marked as build/run time fixed, though I'm not sure why it's also made available at runtime. I need to track this down. Created quarkiverse/quarkus-operator-sdk#689 for this.

@vmuzikar vmuzikar enabled auto-merge (squash) August 21, 2023 11:11
@vmuzikar
Copy link
Contributor

@shawkins Can you please create a backport?

@vmuzikar
Copy link
Contributor

Created a follow-up: #22573

@shawkins shawkins mentioned this pull request Aug 21, 2023
Merged
This was referenced Aug 21, 2023
Closed
Closed
@vmuzikar vmuzikar merged commit 080b53f into keycloak:main Aug 21, 2023
81 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@metacosm metacosm metacosm left review comments

@vmuzikar vmuzikar vmuzikar approved these changes

Assignees

@vmuzikar vmuzikar

Labels
flaky-test team/cloud-native
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove build time dependencies from the Operator dist
4 participants
@shawkins @vmuzikar @ASzc @metacosm