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

Updated to use current version of Horreum 0.15.x #18

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

whitingjr
Copy link

@whitingjr whitingjr commented Sep 19, 2024

Fixes #17 feature request.

This PR updates the plugin to use the most recent version of Horreum libraries and api.

Testing done

Used existing JUnit test cases to ensure the operations provided are functioning as expected after the version bump.

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue
  • Replace SNAPSHOT dependency with released library version

@whitingjr whitingjr force-pushed the horreum-plugin-version-bump-horreum-to-0.14-v2 branch from 0f9c64b to 0211213 Compare September 19, 2024 15:33
@whitingjr
Copy link
Author

I have observed the creating of a HorreumClient object is flakey due to Connection reset exceptions. Retries have been included to solve this. In the HorreumTestClientExtension. This might still be un-reliable when running on machines that are slower. I am thinking a better solution is to attempt creating a client for a minute after the Horreum container starts.

@johnaohara
Copy link
Contributor

I have observed the creating of a HorreumClient object is flakey due to Connection reset exceptions. Retries have been included to solve this. In the HorreumTestClientExtension. This might still be un-reliable when running on machines that are slower. I am thinking a better solution is to attempt creating a client for a minute after the Horreum container starts.

@whitingjr I pinged you offline, but looks like that crossed with this comment :). I will copy here;

There is a HorreumResources.waitForContainerReady() method that is used to wait until the containers are ready before progressing, you can see it used in HorreumResources.startContainers() . This scans the container log, looking for a particular string, and throws an exception after a timeout. TestContainers also has a number of different waitFor strategies for pausing execution until different conditions are met if scanning the log is not sufficient.

This method can be used to ensure the services are started before trying to instantiate the client, and remove the need for any retry logic

@whitingjr
Copy link
Author

Thanks for pointing these out. I'll replace the retry and update the branch.

@whitingjr whitingjr force-pushed the horreum-plugin-version-bump-horreum-to-0.14-v2 branch from 0211213 to 6e7358c Compare September 20, 2024 15:57
@johnaohara
Copy link
Contributor

@whitingjr I have opened a PR in Horreum that removes all of the Quarkus transitive dependencies in horreum-infra-common: Hyperfoil/Horreum#2017

They are not needed and will greatly simplify the transitive dependency management of the plugin.

The main difference with this change is "quarkus.http.port" & "quarkus.http.host" need to be added to the map passed to HorreumResources.startContainers() : https://github.com/Hyperfoil/Horreum/pull/2017/files#diff-b43105658cc21b6fa970980abf2234869cdc5fed60280eae9d557a6b89ed54daR112-R116

@whitingjr whitingjr force-pushed the horreum-plugin-version-bump-horreum-to-0.14-v2 branch from 6e7358c to 7351706 Compare September 23, 2024 16:19
@whitingjr
Copy link
Author

Both properties are now added and the Quarkus transitive dependencies are no longer part of the project.

@whitingjr whitingjr force-pushed the horreum-plugin-version-bump-horreum-to-0.14-v2 branch from 7351706 to a343d6c Compare September 23, 2024 16:24
Copy link
Contributor

@johnaohara johnaohara left a comment

Choose a reason for hiding this comment

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

I think the tests and container services look great! I think the dependencies should be cleaned up aligned to latest LTS for Jenkins, comments are inline in POM

I have also opened this PR in Horreum to simplify the container image management: Hyperfoil/Horreum#2023

README.adoc Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@@ -27,7 +27,7 @@ THE SOFTWARE.
<parent>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plugin</artifactId>
<version>4.40</version>
<version>4.52</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

The current latest version is 4.85

Jenkinsfile Outdated Show resolved Hide resolved
pom.xml Show resolved Hide resolved
@whitingjr whitingjr force-pushed the horreum-plugin-version-bump-horreum-to-0.14-v2 branch from a343d6c to 22d3a96 Compare September 24, 2024 10:21
@whitingjr
Copy link
Author

There is one weakness of the test cases being separate objects. The test-suite is not able to use AfterAll annotation. It is executed for every test class which causes containers to be recycled. Docker handles shutting down running containers when the tests all cease but Podman I don't recall. Was there a reason for HorreumUploadStepTest, HorreumUploadTest, HorreumExpectStepTest, HorreumExpectTest etc ?

@whitingjr
Copy link
Author

@johnaohara For me when running the test-suite a 2nd time establishing a HorreumClient object causes Connection reset.

$ mvn clean test;mvn test
.....
[ERROR] Errors: 
[ERROR]   HorreumExpectStepTest.testExpect:22->HorreumPluginTestBase.createTest:18 » Runtime java.net.SocketException: Connection reset
[ERROR]   HorreumExpectTest.testExpectRun:22->HorreumPluginTestBase.createTest:18 » Runtime java.net.SocketException: Connection reset
[ERROR]   HorreumUploadStepTest.testUpload:23->HorreumPluginTestBase.createTest:18 » Runtime java.net.SocketException: Connection reset
[ERROR]   HorreumUploadStepTest.testUploadMultiple:59->HorreumPluginTestBase.createTest:18 » Runtime java.net.SocketException: Connection reset
[ERROR]   HorreumUploadTest.testUpload:32->HorreumPluginTestBase.createTest:18 » Runtime java.net.SocketException: Connection reset
[ERROR]   HorreumUploadTest.testUploadMultiple:55->HorreumPluginTestBase.createTest:18 » Runtime java.net.SocketException: Connection reset
[INFO] 
[ERROR] Tests run: 6, Failures: 0, Errors: 6, Skipped: 0
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------

In the first execution of the test goal jenkins-for-test being unpacked for booting the Jetty server causes sufficient delay for HorreumClient to authenticate. The second test goal uses the existing unpacked directory which is quicker.
Can you recreate this issue?

@whitingjr whitingjr force-pushed the horreum-plugin-version-bump-horreum-to-0.14-v2 branch from 22d3a96 to eaca4ad Compare September 24, 2024 14:06
@whitingjr whitingjr marked this pull request as ready for review September 24, 2024 14:07
@johnaohara
Copy link
Contributor

There is one weakness of the test cases being separate objects. The test-suite is not able to use AfterAll annotation. It is executed for every test class which causes containers to be recycled. Docker handles shutting down running containers when the tests all cease but Podman I don't recall. Was there a reason for HorreumUploadStepTest, HorreumUploadTest, HorreumExpectStepTest, HorreumExpectTest etc ?

I don't know the answer to this question

@johnaohara
Copy link
Contributor

@johnaohara For me when running the test-suite a 2nd time establishing a HorreumClient object causes Connection reset.

$ mvn clean test;mvn test
.....
[ERROR] Errors: 
[ERROR]   HorreumExpectStepTest.testExpect:22->HorreumPluginTestBase.createTest:18 » Runtime java.net.SocketException: Connection reset
[ERROR]   HorreumExpectTest.testExpectRun:22->HorreumPluginTestBase.createTest:18 » Runtime java.net.SocketException: Connection reset
[ERROR]   HorreumUploadStepTest.testUpload:23->HorreumPluginTestBase.createTest:18 » Runtime java.net.SocketException: Connection reset
[ERROR]   HorreumUploadStepTest.testUploadMultiple:59->HorreumPluginTestBase.createTest:18 » Runtime java.net.SocketException: Connection reset
[ERROR]   HorreumUploadTest.testUpload:32->HorreumPluginTestBase.createTest:18 » Runtime java.net.SocketException: Connection reset
[ERROR]   HorreumUploadTest.testUploadMultiple:55->HorreumPluginTestBase.createTest:18 » Runtime java.net.SocketException: Connection reset
[INFO] 
[ERROR] Tests run: 6, Failures: 0, Errors: 6, Skipped: 0
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------

In the first execution of the test goal jenkins-for-test being unpacked for booting the Jetty server causes sufficient delay for HorreumClient to authenticate. The second test goal uses the existing unpacked directory which is quicker. Can you recreate this issue?

do we need to do this?

@whitingjr
Copy link
Author

whitingjr commented Sep 24, 2024

do we need to do this?

No, just declaring it now. Other environments may fail for the initial test goal. I was seeing this occassionally before waitForContainers call was added to ArtemisMQ and Horreum. In the situation this occurs again I am thinking that after waitForContainers has returned to add a retry loop to create a HorreumClient. Let's see if necessary.

@whitingjr
Copy link
Author

whitingjr commented Sep 24, 2024

The CI is failing the build for jdk8 and jdk11.
It seems we are too ambitious targeting 17. Unless the build pipeline is updated.
https://ci.jenkins.io/job/Plugins/job/horreum-plugin/job/PR-18/

@johnaohara
Copy link
Contributor

The CI is failing the build for jdk8 and jdk11. It seems we are too ambitious targeting 17. Unless the build pipeline is updated. https://ci.jenkins.io/job/Plugins/job/horreum-plugin/job/PR-18/

the project will need to align to the minimum version that the LTS jenkins version supports

@whitingjr
Copy link
Author

the project will need to align to the minimum version that the LTS jenkins version supports

That is 11 in plugin-pom/pom.xml for 4.85 tag.
Setting the target to 11 in horreum-plugin/pom.xml then causes this enforcer rule violation.

[INFO] --- enforcer:3.1.0:enforce (display-info) @ horreum ---
[INFO] Restricted to JDK 11 yet io.hyperfoil.tools:horreum-client:jar:0.15.1:compile contains io/hyperfoil/tools/RunServiceExtension.class targeted to JDK 17
[INFO] Restricted to JDK 11 yet io.hyperfoil.tools:horreum-api:jar:0.15.1:compile contains io/hyperfoil/tools/horreum/api/ApiIgnore.class targeted to JDK 17
[WARNING] Unknown bytecodeVersion for com.fasterxml.jackson.core:jackson-core:jar:2.17.2:compile : META-INF/versions/21/com/fasterxml/jackson/core/io/doubleparser/FastDoubleSwar.class: got null class-file-version
[WARNING] Unknown bytecodeVersion for com.fasterxml.jackson.core:jackson-core:jar:2.17.2:compile : META-INF/versions/21/com/fasterxml/jackson/core/io/doubleparser/FastIntegerMath.class: got null class-file-version
[ERROR] Rule 3: org.apache.maven.plugins.enforcer.EnforceBytecodeVersion failed with message:
Found Banned Dependency: io.hyperfoil.tools:horreum-client:jar:0.15.1
Found Banned Dependency: io.hyperfoil.tools:horreum-api:jar:0.15.1

Is there any prospect of getting a JDK 11 compile targeted version for the 2 modules ?

@johnaohara
Copy link
Contributor

the project will need to align to the minimum version that the LTS jenkins version supports

That is 11 in plugin-pom/pom.xml for 4.85 tag. Setting the target to 11 in horreum-plugin/pom.xml then causes this enforcer rule violation.

[INFO] --- enforcer:3.1.0:enforce (display-info) @ horreum ---
[INFO] Restricted to JDK 11 yet io.hyperfoil.tools:horreum-client:jar:0.15.1:compile contains io/hyperfoil/tools/RunServiceExtension.class targeted to JDK 17
[INFO] Restricted to JDK 11 yet io.hyperfoil.tools:horreum-api:jar:0.15.1:compile contains io/hyperfoil/tools/horreum/api/ApiIgnore.class targeted to JDK 17
[WARNING] Unknown bytecodeVersion for com.fasterxml.jackson.core:jackson-core:jar:2.17.2:compile : META-INF/versions/21/com/fasterxml/jackson/core/io/doubleparser/FastDoubleSwar.class: got null class-file-version
[WARNING] Unknown bytecodeVersion for com.fasterxml.jackson.core:jackson-core:jar:2.17.2:compile : META-INF/versions/21/com/fasterxml/jackson/core/io/doubleparser/FastIntegerMath.class: got null class-file-version
[ERROR] Rule 3: org.apache.maven.plugins.enforcer.EnforceBytecodeVersion failed with message:
Found Banned Dependency: io.hyperfoil.tools:horreum-client:jar:0.15.1
Found Banned Dependency: io.hyperfoil.tools:horreum-api:jar:0.15.1

Is there any prospect of getting a JDK 11 compile targeted version for the 2 modules ?

They should be targeting 1.8, but looks like the target definition is wrong in the poms:

https://github.com/Hyperfoil/Horreum/blob/0.15/horreum-api/pom.xml#L13-L14

https://github.com/Hyperfoil/Horreum/blob/0.15/horreum-client/pom.xml#L18-L19

@johnaohara
Copy link
Contributor

@whitingjr : Fix target version for horreum-api and horreum-client Hyperfoil/Horreum#2026

@whitingjr
Copy link
Author

They should be targeting 1.8, but looks like the target definition is wrong in the poms:

https://github.com/Hyperfoil/Horreum/blob/0.15/horreum-api/pom.xml#L13-L14

https://github.com/Hyperfoil/Horreum/blob/0.15/horreum-client/pom.xml#L18-L19

Yes it seems to be targeted to 17.

$ javap -v horreum-api/target/classes/io/hyperfoil/tools/horreum/api/Version.class |grep major
  major version: 61

The effective pom reveals this for horreum-api

<maven.compiler.release>17</maven.compiler.release>
    <maven.compiler.source>8</maven.compiler.source>
    <maven.compiler.target>8</maven.compiler.target>

by adding the release property to horreum-api/pom.xml for the compiler plugin it creates the targeted version correctly.

$ javap -v horreum-api/target/classes/io/hyperfoil/tools/horreum/api/Version.class |grep major
  major version: 52

Shall I create a PR ?

@johnaohara
Copy link
Contributor

A PR is already merged: Hyperfoil/Horreum#2026

@johnaohara
Copy link
Contributor

@whitingjr horreum 0.15.2 has been released that includes the fix for class versions

@whitingjr
Copy link
Author

whitingjr commented Sep 24, 2024

@johnaohara I am seeing horreum-infra-common now causing an issue.

ERROR]   bad class file: /home/whitingjr/.m2/repository/io/hyperfoil/tools/horreum-infra-common/0.15.2/horreum-infra-common-0.15.2.jar(/io/hyperfoil/tools/horreum/infra/common/SelfSignedCert.class)
[ERROR]     class file has wrong version 61.0, should be 55.0

A test locally with horreum-infra-common module compiled to 11 has solved all the issues.

@johnaohara
Copy link
Contributor

@johnaohara I am seeing horreum-infra-common now causing an issue.

ERROR]   bad class file: /home/whitingjr/.m2/repository/io/hyperfoil/tools/horreum-infra-common/0.15.2/horreum-infra-common-0.15.2.jar(/io/hyperfoil/tools/horreum/infra/common/SelfSignedCert.class)
[ERROR]     class file has wrong version 61.0, should be 55.0

A test locally with horreum-infra-common module compiled to 11 has solved all the issues.

Is that running the build with JDK 11? we don't need to run the build/tests with JDK 11, we can use JDK17+, but release should target jdk11

@whitingjr
Copy link
Author

Is that running the build with JDK 11? we don't need to run the build/tests with JDK 11, we can use JDK17+, but release should target jdk11

		<maven.compiler.target>17</maven.compiler.target>
		<maven.compiler.source>17</maven.compiler.source>
		<maven.compiler.release>11</maven.compiler.release>

compiling with the build/test to 17 and release at 11 the error still occurs.

@johnaohara
Copy link
Contributor

Is that running the build with JDK 11? we don't need to run the build/tests with JDK 11, we can use JDK17+, but release should target jdk11

		<maven.compiler.target>17</maven.compiler.target>
		<maven.compiler.source>17</maven.compiler.source>
		<maven.compiler.release>11</maven.compiler.release>

compiling with the build/test to 17 and release at 11 the error still occurs.

what version of the JDK are you using to build with?

@whitingjr
Copy link
Author

Good catch. I was using 11. With 17 it is ok.

@whitingjr whitingjr force-pushed the horreum-plugin-version-bump-horreum-to-0.14-v2 branch from eaca4ad to 7f5c4a0 Compare September 25, 2024 08:24
@whitingjr whitingjr force-pushed the horreum-plugin-version-bump-horreum-to-0.14-v2 branch from 7f5c4a0 to bafea5f Compare September 25, 2024 08:52
@whitingjr
Copy link
Author

There is one weakness of the test cases being separate objects. The test-suite is not able to use AfterAll annotation. It is executed for every test class which causes containers to be recycled. Docker handles shutting down running containers when the tests all cease but Podman I don't recall. Was there a reason for HorreumUploadStepTest, HorreumUploadTest, HorreumExpectStepTest, HorreumExpectTest etc ?

I don't know the answer to this question

To get the advantage of using AfterAll annotation is there any reason not to combine these separate objects into a single object ?

@whitingjr
Copy link
Author

@johnaohara I have updated Jenkinsfile with configuration to change the CI builds. This isn't incorporated automatically to avoid malicious changes. Is there an alternative process to reconfigure the CI ?

@johnaohara
Copy link
Contributor

There is one weakness of the test cases being separate objects. The test-suite is not able to use AfterAll annotation. It is executed for every test class which causes containers to be recycled. Docker handles shutting down running containers when the tests all cease but Podman I don't recall. Was there a reason for HorreumUploadStepTest, HorreumUploadTest, HorreumExpectStepTest, HorreumExpectTest etc ?

I don't know the answer to this question

To get the advantage of using AfterAll annotation is there any reason not to combine these separate objects into a single object ?

I don't see why not

@johnaohara
Copy link
Contributor

@johnaohara I have updated Jenkinsfile with configuration to change the CI builds. This isn't incorporated automatically to avoid malicious changes. Is there an alternative process to reconfigure the CI ?

I do not know how CI/CD works for this project, that is something that needs figuring out

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.

Version bump horreum-plugin to use current 0.15.x Horreum version.
2 participants