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

Let SimpleCommandLauncher kill the agent JVM more promptly when disconnected #921

Closed
wants to merge 2 commits into from

Conversation

jglick
Copy link
Member

@jglick jglick commented Feb 18, 2025

@jglick jglick added the bug label Feb 18, 2025
Copy link
Member

@dwnusbaum dwnusbaum left a comment

Choose a reason for hiding this comment

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

Seems fine. Do you think it is worth running through the PCT to check if it causes any problems for agent disconnection/restart related tests?

@jglick
Copy link
Member Author

jglick commented Feb 18, 2025

Do you think it is worth running through the PCT

That does sound prudent. I will have to look up how to test a JTH bump in PCT.

@jglick
Copy link
Member Author

jglick commented Feb 18, 2025

Turns out it is pretty complex due to a history of core changes: jenkinsci/plugin-compat-tester#746

@timja
Copy link
Member

timja commented Feb 18, 2025

Turns out it is pretty complex due to a history of core changes: jenkinsci/plugin-compat-tester#746

yeah you need to backport this change to the pre jetty 12 line of the test harness and then make a PR to plugin-compat-tester updating the minimum versions of JTH on both lines, then PR to bom with the updated PCT version

jglick added a commit to jglick/support-core-plugin that referenced this pull request Feb 18, 2025
@jglick
Copy link
Member Author

jglick commented Feb 18, 2025

I think I am not going to bother with all that. Will try this in a couple of plugins, and if later it turns out to cause any issues, we can just revert/amend.

@jglick
Copy link
Member Author

jglick commented Feb 18, 2025

Breaks ExecutorStepTest.nodeDisconnectMissingContextVariableException though I am not yet sure why. Also does not seem to suffice in every single case in support-core or in workflow-cps, though again I am unsure why. I was able to confirm that this does kill the agent process before JenkinsRule test cleanup begins, so what else is holding the file handle? Unfortunately Windows file lock error messages do not give details, and I have been unable to reproduce any of these failures locally.

jglick added a commit to jglick/support-core-plugin that referenced this pull request Feb 18, 2025
This reverts commit 06190e8.
It seemed to mostly work, but not in one case.
@jglick jglick closed this Feb 18, 2025
@jglick jglick deleted the SimpleCommandLauncher branch February 18, 2025 23:12
@jglick
Copy link
Member Author

jglick commented Feb 19, 2025

jenkinsci/support-core-plugin#624 still not stable. I think I actually meant to use afterDisconnect.

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.

3 participants