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

[SET-494] eap-7.4.x-jdk17-testsuite job has wrong parameters defined #34

Merged
merged 1 commit into from May 13, 2022

Conversation

gaol
Copy link

@gaol gaol commented Apr 28, 2022

Copy link
Member

@xstefank xstefank left a comment

Choose a reason for hiding this comment

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

@gaol looks good, but we need to also compile with -Delytron right?

Copy link

@rpelisse rpelisse left a comment

Choose a reason for hiding this comment

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

@spyrkob LGTM!

@gaol
Copy link
Author

gaol commented May 5, 2022

@gaol looks good, but we need to also compile with -Delytron right?

Yes, it has the -Delytron -DnoCompile specified

@gaol
Copy link
Author

gaol commented May 6, 2022

Please on hod.

  • TESTSUITE_OPTS=-Delytron -DnoCompile would cause hera/job.sh fail because there is a space in between
  • CGROUP_MOUNT_ENABLED as booleanParam makes it always has value even unset, like: CGROUP_MOUNT_ENABLED="false", which fails the test of [ -n "${CGROUP_MOUNT_ENABLED}" ];

I will work on it and push later.

@spyrkob
Copy link
Collaborator

spyrkob commented May 6, 2022

Is CGROUP_MOUNT_ENABLED something we need because of Java 17? Do we need it in every job running on 17?

@gaol
Copy link
Author

gaol commented May 6, 2022

Is CGROUP_MOUNT_ENABLED something we need because of Java 17?

yes, it is.

Do we need it in every job running on 17?

I think currently only eap-7.4.x-jdk17-testsuite job using JDK 17, but yes, I think so before https://bugs.openjdk.java.net/browse/JDK-8286212 gets fixed

@spyrkob
Copy link
Collaborator

spyrkob commented May 6, 2022

I'm wondering if we can add it automatically in either the builder or one of Util classes whenever job uses JDK17. Just to avoid gotchas and repetition when we have more jobs on 17

@gaol
Copy link
Author

gaol commented May 6, 2022

IMO, to add it automatically, we need to consider the runtime JDK version, so it might be in the pipelines and/or the hera job.sh to decide if we need mount cgroup, not the builder which is used to build the job configuration.

I am not sure if it is worth to do, @rpelisse WDYT?

@rpelisse
Copy link

rpelisse commented May 6, 2022

So, generally speaking, I don't think jobs should be tweaking what is added as volume to the container. The fact that we have to add cgroup is a nasty workaround. So I would rather avoid building changes around this specific requirements. Hopefully, it will be fixed soon and we'll be able to remove this entirely.

@gaol
Copy link
Author

gaol commented May 7, 2022

Updated to add additional job: eap-7.4.x-jdk17-build, which will build on JDK8 with options: -Delytron -DallTests -DskipTests which builds all test modules as well, then the job eap-7.4.x-jdk17-testsuite will copy bits from eap-7.4.x-jdk17-build and run with options: -Delytron -DnoCompile -DallTests on JDK 17.

@gaol
Copy link
Author

gaol commented May 12, 2022

This PR is ready to get merged if nobody object :)

@spyrkob spyrkob merged commit 4be0412 into jboss-set:master May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants