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

Ensure RealJenkinsRule tests always start on a free port #348

Merged
merged 23 commits into from Jun 15, 2022

Conversation

Vlatombe
Copy link
Member

@Vlatombe Vlatombe commented Nov 18, 2021

RealJenkinsRule tests running in parallel or involving multiple Jenkins instances have been flaky because the "random" port allocation can lead to attempts to use ports which are in fact already used.

This work relies on jenkinsci/winstone#174 to ensure the Jenkins instance started by RealJenkinsRule starts on a random free port (allocated by winstone/jetty). Subsequent restarts (if there are) will reuse the same port to ensure Jenkins URL stability.

  • 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
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

pom.xml Outdated Show resolved Hide resolved
src/main/java/org/jvnet/hudson/test/RealJenkinsRule.java Outdated Show resolved Hide resolved
src/main/java/org/jvnet/hudson/test/RealJenkinsRule.java Outdated Show resolved Hide resolved
src/main/java/org/jvnet/hudson/test/RealJenkinsRule.java Outdated Show resolved Hide resolved
src/main/java/org/jvnet/hudson/test/RealJenkinsRule.java Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@jglick
Copy link
Member

jglick commented Mar 19, 2022

Do we even need jenkinsci/winstone#174? Just pass --httpPort=0 and have RealJenkinsRule.Endpoint.register write the port number to a well-known file in $JENKINS_HOME. (Pass the selected port number on subsequent starts since some tests expect the Jenkins URL to be stable across sessions. Reintroduces the race condition for tests which do use multiple sessions, but these are in the minority and I think we have no alternative since you cannot just reserve a port without binding it.)

Mentioned previously in #347 (comment). That discussion diverged into whether RealJenkinsRule.getUrl needed to be supported prior to first startup for some tests. But this PR already breaks that semantics, and has no advantage that I can see over register, but the clear disadvantage of only working with new Jenkins versions.

There was also a discussion of acceptance-test-harness, and perhaps ATH could benefit from the new Winstone feature (I have not studied that case), but this does not mean RealJenkinsRule needs to rely on it. Am I missing something?

@Vlatombe
Copy link
Member Author

Vlatombe commented Mar 21, 2022

Correct, for RJR we could probably take the route you propose to avoid waiting for the next LTS.

@jglick
Copy link
Member

jglick commented Mar 22, 2022

I tried to prototype that but hit a wall—there is no apparent way from inside Jenkins code to tell which port Jetty started on, without having a HttpServletRequest. The JUnit JVM could parse the stdio of the process looking for messages like

o.e.j.server.AbstractConnector#doStart: Started ServerConnector@4201c465{HTTP/1.1,[http/1.1]}{127.0.0.1:32955}

but that does not seem reliable.

pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@Vlatombe Vlatombe marked this pull request as ready for review June 14, 2022 07:45
@Vlatombe Vlatombe changed the title Ensure free port winstone patch Ensure RealJenkinsRule tests always start on a free port Jun 14, 2022
@Vlatombe
Copy link
Member Author

I will lift draft state as soon as Jenkins 2.346 LTS is out.

@jglick
Copy link
Member

jglick commented Jun 14, 2022

#348 (comment) again. Is it feasible to make this work when on newer Jenkins versions but fall back to the previous behavior? Otherwise rollout will be a headache since Dependabot POM updates will break nearly every plugin.

@Vlatombe
Copy link
Member Author

I can probably adapt some of what I've done for the ath...

@Vlatombe Vlatombe marked this pull request as ready for review June 15, 2022 08:31
@Vlatombe Vlatombe requested review from jglick and jtnord June 15, 2022 08:31
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Looks right.

@Vlatombe Vlatombe requested a review from jglick June 15, 2022 15:05
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Ready for release?

src/main/java/org/jvnet/hudson/test/RealJenkinsRule.java Outdated Show resolved Hide resolved
@jglick jglick added bug and removed enhancement labels Jun 15, 2022
@jglick jglick enabled auto-merge June 15, 2022 18:31
@jglick jglick merged commit 83ab3ce into jenkinsci:master Jun 15, 2022
@Vlatombe Vlatombe deleted the ensure-free-port-winstone-patch branch June 15, 2022 19:19
Vlatombe added a commit to Vlatombe/jenkins-test-harness that referenced this pull request Aug 16, 2022
Amends jenkinsci#348, as it broke semantic of #getUrl which now must be called
after starting Jenkins.
@Vlatombe Vlatombe mentioned this pull request Aug 16, 2022
6 tasks
Vlatombe added a commit to Vlatombe/saml-plugin that referenced this pull request Sep 2, 2022
Since jenkinsci/jenkins-test-harness#475 & jenkinsci/jenkins-test-harness#348, you need to call RealJenkinsRule#getUrl after starting Jenkins.
@Vlatombe Vlatombe mentioned this pull request Sep 2, 2022
2 tasks
kuisathaverat pushed a commit to jenkinsci/saml-plugin that referenced this pull request Sep 2, 2022
Since jenkinsci/jenkins-test-harness#475 & jenkinsci/jenkins-test-harness#348, you need to call RealJenkinsRule#getUrl after starting Jenkins.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants