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.

Sign up for GitHub

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

Jump to bottom

Add --noReconnectAfter 1d option when available in the agent image. #1553

Merged
merged 6 commits into from
May 22, 2024
Merged

Add --noReconnectAfter 1d option when available in the agent image. #1553

merged 6 commits into from
May 22, 2024

Conversation

Vlatombe
Copy link
Member

@Vlatombe Vlatombe commented May 16, 2024 •
edited
Loading

jenkinsci/docker-agent#809 added support for REMOTING_OPTS in order to pass custom remoting options to the agent.

This makes use of the new option to pass --noReconnectAfter 1d, allowing the agent to bail out after 1 day of not being able to contact the controller.

This can be tweaked using -Dorg.csanchez.jenkins.plugins.kubernetes.PodTemplateBuilder.noReconnectAfter=30d using the syntax allowed by the agent per jenkinsci/remoting#738

Tested using mvn hpi:run -Dorg.csanchez.jenkins.plugins.kubernetes.PodTemplateBuilder.noReconnectAfter=1m, launching a kubernetes agent, then turning off the controller. The agent bails out after 1 minute in that case.

Testing done

Submitter checklist
Preview Give feedback

@Vlatombe
Usage of jenkinsci/docker-agent#809
405fa8f 
Add system properties based switches to use the new mechanism

To enable usage of REMOTING_OPTS
-Dorg.csanchez.jenkins.plugins.kubernetes.PodTemplateBuilder.useRemotingOpts=true

Enabling it requires all pod templates to use a version of agent with jenkinsci/docker-agent#809 in.

Add extra REMOTING options
-Dorg.csanchez.jenkins.plugins.kubernetes.PodTemplateBuilder.extraRemotingOpts="-noReconnectAfter 10m"

Used this approach because there are migration challenges:
* The remoting version must be recent enough to support REMOTING_OPTS
* When overriding the JNLP container, the configuration form is generic, and part of REMOTING_OPTS is generated based on options defined in the cloud, so can't expose either an "extra remoting options" field, not use the `REMOTING_OPTS` environment variable.
@Vlatombe Vlatombe changed the title Usage of https://github.com/jenkinsci/docker-agent/pull/809 Introduce a way to pass REMOTING_OPTS to remoting agent. May 16, 2024
@jglick
Copy link
Member

jglick commented May 16, 2024

You could just continue to use the existing (now deprecated) variables for existing purposes, and allow the new variable to be set for new purposes such as -noReconnectAfter. That would be more compatible. Eventually you would want to do this to clean up.

@jglick
Copy link
Member

jglick commented May 17, 2024

Actually I expected -noReconnectAfter to be set by default for K8s agents, at least if pod GC is enabled. So it ought to be as simple as defining this env var with just that option and bumping the default image version to pick up the new release; if the user is overriding the jnlp container with some older image, they just will not get the new feature, but nothing else would be broken.

@Vlatombe
Add default value for -noReconnectAfter of 1 day
74502ab 
Can be tuned via org.csanchez.jenkins.plugins.kubernetes.PodTemplateBuilder.noReconnectAfter using the same syntax as what remoting.jar -noReconnectAfter takes.

If the remoting version is too old, this parameter is ignored
@jglick
Copy link
Member

jglick commented May 17, 2024

🤔

set by default for K8s agents, at least if pod GC is enabled

or just a new text field in KubernetesCloud for a list of extra options?

@Vlatombe
Simplify patch
1daed42 
* Bump built-in remoting to the version coming with REMOTING_OPTS
@Vlatombe Vlatombe added the enhancement Improvements label May 21, 2024
@Vlatombe Vlatombe changed the title Introduce a way to pass REMOTING_OPTS to remoting agent. Add --noReconnectAfter option when available in the agent image. May 21, 2024
@Vlatombe Vlatombe changed the title Add --noReconnectAfter option when available in the agent image. Add --noReconnectAfter 1d option when available in the agent image. May 21, 2024
@Vlatombe
Copy link
Member Author

Simplified the patch accordingly.

This was referenced May 21, 2024
Closed
Closed
@Vlatombe Vlatombe marked this pull request as ready for review May 21, 2024 08:10
@Vlatombe Vlatombe requested a review from a team as a code owner May 21, 2024 08:10
@Vlatombe Vlatombe requested a review from jglick May 21, 2024 13:46
jglick
jglick approved these changes May 21, 2024
View reviewed changes
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 nice and simple.

src/test/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateBuilderTest.java Outdated Show resolved Hide resolved
@Vlatombe Vlatombe enabled auto-merge (squash) May 22, 2024 06:30
@Vlatombe Vlatombe merged commit ba6b893 into jenkinsci:master May 22, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jglick jglick jglick approved these changes

Assignees
No one assigned
Labels
enhancement Improvements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Vlatombe @jglick