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

fix: remove job using namespace #139

Merged
merged 5 commits into from
May 12, 2022
Merged

fix: remove job using namespace #139

merged 5 commits into from
May 12, 2022

Conversation

jfroche
Copy link

@jfroche jfroche commented Apr 28, 2022 •
edited
Loading

Jobs created in other namespace than default are not removed correctly
when worker is stopped.

Provide the namespace to the nomad api when removing a Job/stopping a
worker.

  • 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

jfroche added 2 commits April 28, 2022 18:14
@jfroche
fix: remove job using namespace
9b570a1 
Jobs created in other namespace than `default` are not removed correctly
when worker is stopped.

Provide the namespace to the nomad api when removing a Job/stopping a
worker.
@jfroche
chore: add editorconfig config
0c2cc15 
https://editorconfig.org
@jfroche jfroche force-pushed the fix/stop-job-with-ns branch from 62fc7e6 to 0c2cc15 Compare April 28, 2022 16:15
@jfroche jfroche changed the title WIP fix: remove job using namespace fix: remove job using namespace Apr 28, 2022
@jdelee
Copy link

jdelee commented Apr 29, 2022

We've encountered this as well, and a related problem with nomad regions, where the job template can cause an agent to be run in a non-default region, but the request to destroy the agent does not specify the region and fails. Do you think this PR could be extended to handle region as well?

@multani multani self-requested a review April 29, 2022 16:49
@jfroche jfroche force-pushed the fix/stop-job-with-ns branch from 4a3c0d3 to d67391d Compare May 1, 2022 00:18
@jfroche
Copy link
Author

jfroche commented May 1, 2022 •
edited
Loading

We've encountered this as well, and a related problem with nomad regions, where the job template can cause an agent to be run in a non-default region, but the request to destroy the agent does not specify the region and fails. Do you think this PR could be extended to handle region as well?

I have added the region to the DELETE when it is not global/default region. The change doesn't handle multi region, not sure it is an actual use case for anyone.
Can you check if it works for you ? Builded plugin is here: https://ci.jenkins.io/job/Plugins/job/nomad-plugin/job/master/110/artifact/org/jenkins-ci/plugins/nomad/0.9.3-rc181.db_953b_d0c0de/nomad-0.9.3-rc181.db_953b_d0c0de.hpi

@jfroche
fix: remove job using region
32f810c 
Jobs created in other region than `global` are not removed when worker
is stopped.

This fix doesn't handle multi region yet.
@jfroche jfroche force-pushed the fix/stop-job-with-ns branch from d67391d to 32f810c Compare May 1, 2022 00:33
jfroche added 2 commits May 6, 2022 22:33
@jfroche
Fix pruning orphaned worker
f3055bd 
Make sure we leave enough time for a new nomad worker to join the jenkins
controller before removing the nomad worker.
@jfroche
Improve request logging on failure
3bc4aa9
multani
multani approved these changes May 12, 2022
View reviewed changes
Copy link

@multani multani left a comment

Choose a reason for hiding this comment

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

I didn't tested the fix related to the region (I don't have the setup to test that under my hands), but looking at the code this looks good. Thanks for the fix! 🙇

src/main/java/org/jenkinsci/plugins/nomad/NomadCloud.java
Comment on lines +365 to +371
String workerJob = nomad.startWorker(workerName, jnlpSecret, template);
JSONObject workerJobJSON = new JSONObject(workerJob).getJSONObject("Job");
String namespace = workerJobJSON.optString("Namespace");
if (!namespace.equals("")) {
worker.setNamespace(namespace);
}
worker.setRegion(workerJobJSON.optString("Region"));
Copy link

Choose a reason for hiding this comment

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

Maybe we should have a way to represent jobs as an objects, and serialize them on demand before sending them to the API, instead of building strings and parsing them out again from JSON to get/change some values.

@multani multani merged commit 5dab43b into jenkinsci:master May 12, 2022
@multani
Copy link

multani commented May 12, 2022 •
edited
Loading

I'll release it as part of 0.9.3, but https://repo.jenkins-ci.org is down at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@multani multani multani approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@jfroche @jdelee @multani