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

CI: replace JDK 15 with 16 #16195

Closed
13 of 16 tasks
famod opened this issue Apr 1, 2021 · 32 comments · Fixed by #17053 or #17082
Closed
13 of 16 tasks

CI: replace JDK 15 with 16 #16195

famod opened this issue Apr 1, 2021 · 32 comments · Fixed by #17053 or #17082
Assignees
Labels
area/housekeeping Issue type for generalized tasks not related to bugs or enhancements
Milestone

Comments

@famod
Copy link
Member

famod commented Apr 1, 2021

Description

JDK 16 reached GA a few weeks ago and the natural cycle would be to replace the jobs in Quarkus CI that are currently running on JDK 15 with JDK 16.
Our EA test job/workflow [1] is still running on 16 (and not 17-ea) because there are still some problems, especially with Kotlin and Gradle.
The idea is to get the EA job to the point where we can make the switch (both for CI JDK 15 -> 16 and EA job 16 -> 17-ea).

[1] https://github.com/quarkusio/quarkus/actions/workflows/jdk-early-access-build.yml (see also #15867)

FTR:

/cc @gsmet @gastaldi

Currently known TODOs (potentially incomplete)

For now checked TODOs can be found in my fork: main...famod:jdk16-fixes

@famod famod added the area/housekeeping Issue type for generalized tasks not related to bugs or enhancements label Apr 1, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Apr 1, 2021

/cc @evanchooly

@famod famod removed the area/kotlin label Apr 1, 2021
@famod famod self-assigned this Apr 1, 2021
@famod
Copy link
Member Author

famod commented Apr 1, 2021

I'm currently testing some fixes/workarounds for the known problems:
https://github.com/famod/quarkus/actions/workflows/jdk-early-access-build.yml?query=branch%3Ajdk16-fixes

@famod
Copy link
Member Author

famod commented Apr 2, 2021

After skipping devtools/gradle and integration-tests/gradle and adding an --add-opens workaround for the kotlin-maven-plugin, the build failed in my fork with 100 test failures. I'll try to group the failures...

@famod
Copy link
Member Author

famod commented Apr 2, 2021

Affected test classes with number of failing test methods:

grep -P '<<< (ERROR|FAILURE)' quarkus-jdk16-failures.log | grep -Po '[^ ]+Test(?=\.)' | sort | uniq -c
      2 io.quarkus.cli.CliTest
     11 io.quarkus.elytron.security.ldap.CustomRoleDecoderTest
     11 io.quarkus.elytron.security.ldap.MinimalConfigurationTest
      1 io.quarkus.elytron.security.ldap.SearchRecursiveTest
      1 io.quarkus.it.kubernetes.BasicKubernetesTest
      1 io.quarkus.it.kubernetes.BasicOpenshiftTest
      1 io.quarkus.it.kubernetes.KnativeClusterLocalTest
      1 io.quarkus.it.kubernetes.KnativeContainerImageTest
      1 io.quarkus.it.kubernetes.KnativeScaleBoundsTest
      1 io.quarkus.it.kubernetes.KnativeScaleToZeroTest
      1 io.quarkus.it.kubernetes.KnativeTest
      1 io.quarkus.it.kubernetes.KnativeWithApplicationPropertiesTest
      1 io.quarkus.it.kubernetes.KnativeWithHealthTest
      1 io.quarkus.it.kubernetes.KnativeWithSecretConfigTest
      1 io.quarkus.it.kubernetes.KnativeWithVolumesTest
      1 io.quarkus.it.kubernetes.KubernetesAndMinikubeWithApplicationPropertiesTest
      1 io.quarkus.it.kubernetes.KubernetesConfigWithSecretsTest
      1 io.quarkus.it.kubernetes.KubernetesInitContainersTest
      1 io.quarkus.it.kubernetes.KubernetesServiceMappingTest
      1 io.quarkus.it.kubernetes.KubernetesWithApplicationPropertiesTest
      1 io.quarkus.it.kubernetes.KubernetesWithConfigMapCustomModeTest
      1 io.quarkus.it.kubernetes.KubernetesWithConfigMapTest
      1 io.quarkus.it.kubernetes.KubernetesWithDefaultsTest
      1 io.quarkus.it.kubernetes.KubernetesWithDeprecatedPropertiesTest
      1 io.quarkus.it.kubernetes.KubernetesWithEnvFromConfigMapTest
      1 io.quarkus.it.kubernetes.KubernetesWithEnvFromFieldTest
      1 io.quarkus.it.kubernetes.KubernetesWithEnvFromSecretTest
      1 io.quarkus.it.kubernetes.KubernetesWithGitlabLikeImageTest
      1 io.quarkus.it.kubernetes.KubernetesWithHealthAndJibTest
      1 io.quarkus.it.kubernetes.KubernetesWithHealthTest
      1 io.quarkus.it.kubernetes.KubernetesWithHostAliasesTest
      1 io.quarkus.it.kubernetes.KubernetesWithMetricsCustomAbsoluteTest
      1 io.quarkus.it.kubernetes.KubernetesWithMetricsCustomRelativeTest
      1 io.quarkus.it.kubernetes.KubernetesWithMetricsNoAnnotationsTest
      1 io.quarkus.it.kubernetes.KubernetesWithMetricsTest
      1 io.quarkus.it.kubernetes.KubernetesWithMicrometerTest
      1 io.quarkus.it.kubernetes.KubernetesWithMixedStyleEnvTest
      1 io.quarkus.it.kubernetes.KubernetesWithNewStyleEnvTest
      1 io.quarkus.it.kubernetes.KubernetesWithOldStyleEnvTest
      1 io.quarkus.it.kubernetes.KubernetesWithQuarkusAppNameTest
      1 io.quarkus.it.kubernetes.KubernetesWithRbacAndNamespaceTest
      1 io.quarkus.it.kubernetes.KubernetesWithResourcesRequirementsTest
      1 io.quarkus.it.kubernetes.KubernetesWithRootAndHealthTest
      1 io.quarkus.it.kubernetes.KubernetesWithSidecarAndJibTest
      1 io.quarkus.it.kubernetes.KubernetesWithSidecarAndProbesTest
      1 io.quarkus.it.kubernetes.KubernetesWithSysGroupTest
      1 io.quarkus.it.kubernetes.KubernetesWithWarningsEnvTest
      1 io.quarkus.it.kubernetes.MinikubeWithApplicationPropertiesTest
      1 io.quarkus.it.kubernetes.MinikubeWithDefaultsTest
      1 io.quarkus.it.kubernetes.MinikubeWithMixedStyleEnvTest
      1 io.quarkus.it.kubernetes.OpenshiftV3Test
      1 io.quarkus.it.kubernetes.OpenshiftV4Test
      1 io.quarkus.it.kubernetes.OpenshiftWithAppConfigMapTest
      1 io.quarkus.it.kubernetes.OpenshiftWithAppSecretTest
      1 io.quarkus.it.kubernetes.OpenshiftWithApplicationPropertiesTest
      1 io.quarkus.it.kubernetes.OpenshiftWithHealthTest
      1 io.quarkus.it.kubernetes.OpenshiftWithLegacySidecarAndS2iTest
      1 io.quarkus.it.kubernetes.OpenshiftWithRoutePropertiesTest
      1 io.quarkus.it.kubernetes.OpenshiftWithS2iTest
      1 io.quarkus.it.kubernetes.OpenshiftWithSidecarAndS2iTest
      1 io.quarkus.it.kubernetes.OpenshiftWithUberJarTest
      1 io.quarkus.it.kubernetes.WithKubernetesClientTest
      2 io.quarkus.it.main.MethodSourceTest
      2 io.quarkus.it.main.ParameterResolverTest
      1 org.eclipse.microprofile.fault.tolerance.tck.fallbackmethod.FallbackMethodDefaultMethodTest
     14 org.eclipse.microprofile.rest.client.tck.ClientHeaderParamTest
      1 org.eclipse.microprofile.rest.client.tck.InvalidInterfaceTest
      1 org.eclipse.microprofile.rest.client.tck.timeout.TimeoutBuilderIndependentOfMPConfigTest
      1 org.eclipse.microprofile.rest.client.tck.timeout.TimeoutTest
      2 org.eclipse.microprofile.rest.client.tck.timeout.TimeoutViaMPConfigTest
      2 org.eclipse.microprofile.rest.client.tck.timeout.TimeoutViaMPConfigWithConfigKeyTest

updated on 02.05.2021

@famod
Copy link
Member Author

famod commented May 2, 2021

All those io.quarkus.it.kubernetes.* failures are (AFAICT) caused by this change:
openjdk/jdk@6bb7e45#diff-64e57a650df91c988910dd64b53c82b0521869c5f4afd1024f03c6a97f3a8344
Surefire is still setting java.io.tmpdir for the forked JVM, but "too late" to be picked up by that JDK StaticProperty class.
This results in a mismatch of expected and actual working directories for those tests (that expect a kubernetes directory in a specific path), because System.getProperty("java.io.tmpdir") does yield the expected result (the target dir) but Files.createTempDirectory(...) picks up the default system temp dir. 🤦

I think I'll create an issue for Surefire. But we probably can't wait for a release with a fix (if any such fix is even possible in the first place).
Workaround ideas:

  • pass System.getProperty("java.io.tmpdir") to Files.createTempDirectory(...)
    major downside: we'd need to catch all usages!
  • set java.io.tmpdir via surefire config (yet to be tested, but should be set early enough...hopefully)

FYI @Tibor17. What's interesting is that Surefire is also setting user.dir (at least it's in this surefire_<many numbers>tmp file that is passed to the forked JVM) and user.dir has the same problem...but already since JDK 11!
openjdk/jdk@4098f25#diff-bce1cc542611fe2285622bbb95442ebc5ba4836fa8ef12438b2738a11cb375b7R66

@famod
Copy link
Member Author

famod commented May 2, 2021

Two tests in integration-tests/main (MethodSourceTest & ParameterResolverTest) fail due to:

com.thoughtworks.xstream.converters.ConversionException:
No converter available
---- Debugging information ----
message             : No converter available
type                : java.util.Arrays$ArrayList
converter           : com.thoughtworks.xstream.converters.reflection.ReflectionConverter
message[1]          : Unable to make field protected transient int java.util.AbstractList.modCount accessible: module java.base does not "opens java.util" to unnamed module @2a79781f
-------------------------------
        at com.thoughtworks.xstream.core.DefaultConverterLookup.lookupConverterForType(DefaultConverterLookup.java:88)

Will have to set --add-opens here as well.

@famod
Copy link
Member Author

famod commented May 2, 2021

Various tests fail in extensions/elytron-security-ldap/deployment with:

Caused by: java.lang.IllegalAccessException: class io.quarkus.elytron.security.ldap.QuarkusDirContextFactory$QuarkusInitialContextFactoryBuilder cannot access class com.sun.jndi.ldap.LdapCtxFactory (in module java.naming) because module java.naming does not export com.sun.jndi.ldap to unnamed module @2894f984
        at java.base/jdk.internal.reflect.Reflection.newIllegalAccessException(Reflection.java:385)
        at java.base/java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:687)
        at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:489)
        at java.base/java.lang.reflect.ReflectAccess.newInstance(ReflectAccess.java:128)
        at java.base/jdk.internal.reflect.ReflectionFactory.newInstance(ReflectionFactory.java:350)
        at java.base/java.lang.Class.newInstance(Class.java:642)
        at io.quarkus.elytron.security.ldap.QuarkusDirContextFactory$QuarkusInitialContextFactoryBuilder.createInitialContextFactory(QuarkusDirContextFactory.java:166)
        ... 23 more

Will have to set --add-opens here as well.

@gastaldi
Copy link
Contributor

gastaldi commented May 2, 2021

  • set java.io.tmpdir via surefire config (yet to be tested, but should be set early enough...hopefully)

We already do that in https://github.com/jboss/jboss-parent-pom/blob/main/pom.xml#L587.

Maybe that's not being used correctly?

@famod
Copy link
Member Author

famod commented May 2, 2021

@gastaldi

Maybe that's not being used correctly?

Thanks for checking, but turns out it has to be set via -D in <argLine>... (for JDK16+).

@famod
Copy link
Member Author

famod commented May 2, 2021

Ah! So it's not actually Surefire setting it on its own! That's good to know. So, sorry for bothering you @Tibor17!

@gastaldi
Copy link
Contributor

gastaldi commented May 2, 2021

Thanks for checking, but turns out it has to be set via -D in <argLine>... (for JDK16+).

Is that because of JDK 16+ only? I remember this setting working in other projects just fine.

@famod
Copy link
Member Author

famod commented May 2, 2021

Yes, JDK16+ only as far I could trace it back: openjdk/jdk@6bb7e45#diff-64e57a650df91c988910dd64b53c82b0521869c5f4afd1024f03c6a97f3a8344

@famod
Copy link
Member Author

famod commented May 2, 2021

I've updated the TODO list to reflect the current state of affairs. Two builds are running currently in my fork, will check them in the next few days.
This is what I have added so far: main...famod:jdk16-fixes (I'll remove [skip ci] once I think I'm done).

@Tibor17
Copy link
Contributor

Tibor17 commented May 2, 2021

@famod
We are not overriding java.io.tmpdir in the forked surefire JVM. I checked in the code. And we do not need to do it, no reason.
The CWD should be set to the path where the POM resides.

@Tibor17
Copy link
Contributor

Tibor17 commented May 2, 2021

@famod
We are overriding user.dir at the line setSystemProperties( new File( tmpDir, effectiveSystemPropertiesFileName ) );. It is an old code and I do not understand why we do it like this even if the CWD is set to the process cli.setWorkingDirectory( getWorkingDirectory( forkNumber ).getAbsolutePath() );. I think this can be checked out and the line result.setProperty( "user.dir", getWorkingDirectory().getAbsolutePath() ); can be removed in the AbstractSurefireMojo.

@famod
Copy link
Member Author

famod commented May 3, 2021

@Tibor17 WDYT about extending the systemPropertyVariables logic in surefire to detect "special" system properties like java.io.tmpdir and automatically set those via -D instead of setSystemProperties(...)?

Btw @gastaldi I'm now getting:

[WARNING] The system property java.io.tmpdir is configured twice! The property appears in <argLine/> and any of <systemPropertyVariables/>, <systemProperties/> or user property.

So I'll have to override the property from jboss-parent completely.

PS: Meh...I guess IntelliJ IDEA will then also use the default temp dir for testing.

@famod
Copy link
Member Author

famod commented May 3, 2021

@Ladicek @jmartisk @michalszynkiewicz
RestClientTestCase.testFaultTolerance (in integration-tests/main) fails on JDK 16 with:

Caused by: java.lang.reflect.InaccessibleObjectException: Unable to make java.lang.invoke.MethodHandles$Lookup(java.lang.Class) accessible: module java.base does not "opens java.lang.invoke" to unnamed module @486ecb69
        at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:357)
        at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
        at java.base/java.lang.reflect.Constructor.checkCanSetAccessible(Constructor.java:188)
        at java.base/java.lang.reflect.Constructor.setAccessible(Constructor.java:181)
        at io.smallrye.faulttolerance.DefaultMethodFallbackProvider.getFallback(DefaultMethodFallbackProvider.java:19)
        at io.smallrye.faulttolerance.FaultToleranceInterceptor.lambda$prepareFallbackFunction$7(FaultToleranceInterceptor.java:481)
        at io.smallrye.faulttolerance.core.fallback.Fallback.doApply(Fallback.java:59)
        at io.smallrye.faulttolerance.core.fallback.Fallback.apply(Fallback.java:32)
        at io.smallrye.faulttolerance.core.metrics.MetricsCollector.doApply(MetricsCollector.java:80)
        at io.smallrye.faulttolerance.core.metrics.MetricsCollector.apply(MetricsCollector.java:70)
        at io.smallrye.faulttolerance.FaultToleranceInterceptor.syncFlow(FaultToleranceInterceptor.java:174)
        at io.smallrye.faulttolerance.FaultToleranceInterceptor.interceptCommand(FaultToleranceInterceptor.java:151)
        at io.smallrye.faulttolerance.FaultToleranceInterceptor_Bean.intercept(FaultToleranceInterceptor_Bean.zig:517)
        at org.jboss.resteasy.microprofile.client.InvocationContextImpl$InterceptorInvocation.invoke(InvocationContextImpl.java:154)
        at org.jboss.resteasy.microprofile.client.InvocationContextImpl.invokeNext(InvocationContextImpl.java:53)
        at org.jboss.resteasy.microprofile.client.InvocationContextImpl.proceed(InvocationContextImpl.java:89)

Is this a known problem? Is there a fix?

@Tibor17
Copy link
Contributor

Tibor17 commented May 4, 2021

@famod
Regarding this warning

[WARNING] The system property java.io.tmpdir is configured twice! The property appears in and any of , or user property.

it is a parameterized message and java.io.tmpdir in the blacklist. Not sure if the property is really a must there. After making a fast lookup in the code, I would say it is not but of course I have to double check it, remove it and run all ITs.

Regarding systemPropertyVariables I have no answer yet. Pls let me think about it a bit.

@Ladicek
Copy link
Contributor

Ladicek commented May 4, 2021

@famod It is a known problem now that you pointed it to me :-) I'll take a look, thanks.

@famod
Copy link
Member Author

famod commented May 4, 2021

Current state (rest is ok):

[INFO] Quarkus - Integration Tests - Devtools ............. FAILURE [04:34 min]
[INFO] Quarkus - Integration Tests - Kotlin ............... FAILURE [20:14 min]
[INFO] Quarkus - TCK - MicroProfile Fault Tolerance ....... FAILURE [11:34 min]
[INFO] Quarkus - TCK - MicroProfile REST Client ........... FAILURE [11:05 min]

asfgit pushed a commit to apache/maven-surefire that referenced this issue May 4, 2021
@Tibor17
Copy link
Contributor

Tibor17 commented May 4, 2021

@famod
I have removed user.dir. Let's see the build result.
https://github.com/apache/maven-surefire/tree/userDir
https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven-surefire/job/userDir/
The subprocess CWD and the property should have the same value. So, removing the setter for the property should not change anything but it's safe for new versions of Java.

@Tibor17
Copy link
Contributor

Tibor17 commented May 4, 2021

Regarding java.io.tmpdir we do not override it. This warning means that your project set this twice. Once in the config <argLine> and some system property.

[WARNING] The system property java.io.tmpdir is configured twice! The property appears in and any of , or user property.

Quoting from Surefire code, the system properties are the Maven system properties and config parameters systemProperties, systemPropertyVariables and User Properties. Check also settings.xml, folder for .mvn or the environment JAVA_OPTS. Pls check everything!

@famod
Copy link
Member Author

famod commented May 4, 2021

@Tibor17

Regarding java.io.tmpdir we do not override it. This warning means that your project set this twice. Once in the config <argLine> and some system property.

It's clear where it's coming from: jboss-parent is setting it via systemPropertyVariables and since it is set too late for JDK 16, I had to add it via argLine here in Quarkus (not yet merged, still only in my fork).
Until it is changed in jboss-parent (in whichever way), I'm using this workaround to avoid inheritance:

<systemPropertyVariables combine.self="override">

Btw, I think I'll create a SUREFIRE issue in the next few days for the suggestion to handle certain systemPropertyVariables in a special way.

@famod
Copy link
Member Author

famod commented May 4, 2021

@Ladicek

@famod It is a known problem now that you pointed it to me :-) I'll take a look, thanks.

Thanks! Btw, the same happens in org.eclipse.microprofile.fault.tolerance.tck.fallbackmethod.FallbackMethodDefaultMethodTest.fallbackMethodDefaultMethod (tcks/microprofile-fault-tolerance).

@Tibor17
Copy link
Contributor

Tibor17 commented May 4, 2021

@famod
Copy link
Member Author

famod commented May 4, 2021

@Tibor17 Interesting! But that's kind of funny because those are deprecated and not long ago I changed it in jboss-parent to systemPropertyVariables (because of the deprecation).

@Tibor17
Copy link
Contributor

Tibor17 commented May 4, 2021

@famod
I checked it again. It is not set eagerly. We can make it eager or a new parameter similar to ...Values.

@Ladicek
Copy link
Contributor

Ladicek commented May 5, 2021

@famod It is a known problem now that you pointed it to me :-) I'll take a look, thanks.

Thanks! Btw, the same happens in org.eclipse.microprofile.fault.tolerance.tck.fallbackmethod.FallbackMethodDefaultMethodTest.fallbackMethodDefaultMethod (tcks/microprofile-fault-tolerance).

Funny thing is that once I updated Weld in SmallRye Fault Tolerance (to the latest release that supports JDK 16), all tests pass, including the entire TCK. Which suggests there may be something wrong in Quarkus. Hmm.

@Ladicek
Copy link
Contributor

Ladicek commented May 5, 2021

Ah, I know what's wrong. smallrye-fault-tolerance-5.0.0.jar is supposed to be a MR JAR, but the java9 variant of DefaultMethodFallbackProvider is lost somewhere during the build process!

EDIT: the META-INF/versions directory was lost in the 4.3.1 release, so this commit must be guilty: smallrye/smallrye-fault-tolerance@4b5081b

@Ladicek
Copy link
Contributor

Ladicek commented May 5, 2021

Should be fixed in next SmallRye Fault Tolerance release that will contain smallrye/smallrye-fault-tolerance#409

@famod
Copy link
Member Author

famod commented May 5, 2021

@Ladicek Great, thanks! Will exclude the affected tests until then.

asfgit pushed a commit to apache/maven-surefire that referenced this issue May 5, 2021
@Tibor17
Copy link
Contributor

Tibor17 commented May 6, 2021

@famod
The CI build passed successfully. It removed the code overriding user.dir in forked surefire JVM. Pls create a Jira ticket for this, thx.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/housekeeping Issue type for generalized tasks not related to bugs or enhancements
Projects
None yet
4 participants