replace worker template by a more flexible approach #82

j3t · 2021-07-27T12:10:56Z

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

I have created different plugin configuration based on v0.7.4 and added tests for them (see NomadCloudConfigurationTest). Afterwards, I have replaced the worker template by a more flexible approach and added a migration path for older configurations.

j3t

added tests for previous migrations

src/main/java/org/jenkinsci/plugins/nomad/MigrationHelper.java

src/main/java/org/jenkinsci/plugins/nomad/NomadCloud.java

src/main/java/org/jenkinsci/plugins/nomad/NomadWorker.java

src/main/java/org/jenkinsci/plugins/nomad/NomadWorkerTemplate.java

src/main/java/org/jenkinsci/plugins/nomad/NomadCloud.java

j3t · 2021-07-28T11:11:05Z

src/main/java/org/jenkinsci/plugins/nomad/NomadApi.java

        Request.Builder builder = new Request.Builder()
-                .url(this.nomadApi + "/v1/job/" + workerName + "?region=" + template.getRegion());
+                .url(this.nomadApi + "/v1/jobs");


Since region and workerName are part of the job file already, we don't have to define them here explicitly.

src/main/java/org/jenkinsci/plugins/nomad/NomadWorker.java

src/main/java/org/jenkinsci/plugins/nomad/NomadCloud.java

src/main/java/org/jenkinsci/plugins/nomad/MigrationHelper.java

mikedld · 2021-10-18T14:32:33Z

Anything more to work on here @j3t? I'd love to try and add another worker driver support but would hate to interfere with whatever is happening here.

multani · 2021-10-18T15:53:30Z

@mikedld It's only waiting for my review but I didn't have time to get into this PR and test it yet :'(

j3t · 2021-10-19T10:47:02Z

Anything more to work on here @j3t? I'd love to try and add another worker driver support but would hate to interfere with whatever is happening here.

Hi @mikedld, it would be really nice if you could test the current state and give use feedback.You can find the build artifacts here https://ci.jenkins.io/blue/organizations/jenkins/Plugins%2Fnomad-plugin/detail/PR-82/10/artifacts. You just have to download the hpi file and then you can install it manually via Manage Jenkins -> Manage Plugins -> Advanced -> Upload Plugin.

multani

@j3t I (finally!) took a bit of time to have a look at your pull request.

Generally speaking: it's quite cool 🎉

I had a look if we could have native Nomad jobs (HCL) instead of JSON as it would be easier to write in the text area, and it would provide native support for interpolating the secret/token variables if it was supporting HCL2 but:

The only HCL capable Java library I found was https://github.com/bertramdev/hcl4j and it doesn't support HCL2 for the time being.
Migrating from the original configuration to HCL may be more challenging (in the past, I found it difficult to serialize arbitrary structure to HCL, I don't it has changed since)

Anyway:

I think it would be useful to add loggers from the migrations are executed, I would say:

At the very least when a worker template is migrated
Possibly when the whole migration is executed

Otherwise, from what I have tested, this is working great and I don't see any problem merging this soon 👍

src/main/java/org/jenkinsci/plugins/nomad/MigrationHelper.java

src/main/java/org/jenkinsci/plugins/nomad/NomadCloud.java

src/main/java/org/jenkinsci/plugins/nomad/NomadWorker.java

src/main/java/org/jenkinsci/plugins/nomad/NomadWorkerTemplate.java

src/main/resources/org/jenkinsci/plugins/nomad/NomadWorkerTemplate/help-jobTemplate.html

j3t · 2021-10-20T08:53:26Z

Thanks! I have incorporated the feedback.

What is about the SpotBugs findings (UUF_UNUSED_FIELD)? I don't know how this could be solved? Is there a way to ignore them?

j3t · 2021-10-20T10:16:04Z

I had a look if we could have native Nomad jobs (HCL) instead of JSON as it would be easier to write in the text area, and it would provide native support for interpolating the secret/token variables if it was supporting HCL2 but:

The only HCL capable Java library I found was https://github.com/bertramdev/hcl4j and it doesn't support HCL2 for the time being.

Migrating from the original configuration to HCL may be more challenging (in the past, I found it difficult to serialize arbitrary structure to HCL, I don't it has changed since)

In general I would like to have that but as far as I know, the Nomad REST API only supports JSON. This means we have to transform HCL2 to JSON, right? There is a discussion and also some libraries available (see hashicorp/hcl#294).

Co-authored-by: Jonathan Ballet <jon@multani.info>

j3t · 2021-10-20T14:59:27Z

The only HCL capable Java library I found was https://github.com/bertramdev/hcl4j and it doesn't support HCL2 for the time being.

Maybe we can use the Nomad API to transform HCL to JSON (see https://www.nomadproject.io/api-docs/jobs#parse-job).

multani · 2021-10-21T07:24:27Z

I had a look if we could have native Nomad jobs (HCL) instead of JSON as it would be easier to write in the text area, and it would provide native support for interpolating the secret/token variables if it was supporting HCL2 but:

The only HCL capable Java library I found was https://github.com/bertramdev/hcl4j and it doesn't support HCL2 for the time being.

Migrating from the original configuration to HCL may be more challenging (in the past, I found it difficult to serialize arbitrary structure to HCL, I don't it has changed since)

In general I would like to have that but as far as I know, the Nomad REST API only supports JSON. This means we have to transform HCL2 to JSON, right? There is a discussion and also some libraries available (see hashicorp/hcl#294).

Maybe we can use the Nomad API to transform HCL to JSON (see https://www.nomadproject.io/api-docs/jobs#parse-job).

The API only takes JSON yes, but if it could be transformed once before submitting the job, that would be really nice!
I forgot about the /parse API, maybe that could be useful? It's available since Nomad 0.8.2 which is more than 3 years old, so I think it would be fine to support.

Also (I forgot yesterday), in any case, it would be nice if the UI could provide some basic validation of the input: at the moment, writing in a non-valid JSON "succeeds" silently when the form is submitted :)

j3t · 2021-10-21T08:08:06Z

I forgot about the /parse API, maybe that could be useful? It's available since Nomad 0.8.2 which is more than 3 years old, so I think it would be fine to support.

Do we really need the HCL support in the first place? I mean, we could add it later as an alternative as well.

j3t · 2021-10-21T08:12:29Z

Also (I forgot yesterday), in any case, it would be nice if the UI could provide some basic validation of the input: at the moment, writing in a non-valid JSON "succeeds" silently when the form is submitted :)

We could use the Nomad API for that as well (see https://www.nomadproject.io/api-docs/validate#validate-job), but I prefer a validation button for that instead of a silent validation when the form is submitted.

multani · 2021-10-22T10:12:39Z

I forgot about the /parse API, maybe that could be useful? It's available since Nomad 0.8.2 which is more than 3 years old, so I think it would be fine to support.

Do we really need the HCL support in the first place? I mean, we could add it later as an alternative as well.

If there's a good UX / migration path for supporting HCL later, it's fine by me 👍 (I don't really see how to nicely support both right now though).

Also (I forgot yesterday), in any case, it would be nice if the UI could provide some basic validation of the input: at the moment, writing in a non-valid JSON "succeeds" silently when the form is submitted :)

We could use the Nomad API for that as well (see https://www.nomadproject.io/api-docs/validate#validate-job), but I prefer a validation button for that instead of a silent validation when the form is submitted.

The validation button would be fine, yes 👍

Actually, I haven't tried to save the buggy JSON and see what the plugin would do in that case; but the earlier and clearer the error message we can get, the better it would be IMO.

From experience, when cloud workers provisioning fails for any reason (the Nomad plugin or any other plugin providing dynamic workers), it's always a PITA to debug / figure out in Jenkins.

j3t · 2021-10-22T11:55:17Z

I have added a validation button. Below you can find an example where the datacenter is missing:

j3t · 2021-10-22T12:16:49Z

With the /job/plan api it looks even better

:

j3t · 2021-10-22T12:29:48Z

If there's a good UX / migration path for supporting HCL later, it's fine by me 👍 (I don't really see how to nicely support both right now though).

We could just allow both formats. No migration necessary at all.

multani · 2021-10-25T15:53:41Z

With the /job/plan api it looks even better :

Yes, that's great!

If there's a good UX / migration path for supporting HCL later, it's fine by me +1 (I don't really see how to nicely support both right now though).

We could just allow both formats. No migration necessary at all.

OK, well let's start with this initial approach and think how to add (or not) HCL after that.

multani

Tested with the latest changes and looks good to me!

multani · 2021-10-26T18:32:34Z

@j3t Thanks a lot for this refactoring: this is a huge PR and I think it will help to support more Nomad features/changes in the future, this is really cool 🚀
I'll make a new pre-release with this change: hopefully it all works correctly and we can make a proper release :)

multani · 2021-10-27T11:22:47Z

I directly released a new version in https://github.com/jenkinsci/nomad-plugin/releases/tag/v0.9.0

j3t mentioned this pull request Jul 27, 2021

Rework of worker template so that it is more flexible #81

Closed

j3t force-pushed the feature-81-more-flexible-worker-template branch from 03fd9fa to e8c2f8b Compare July 28, 2021 10:08

j3t commented Jul 28, 2021

View reviewed changes

src/main/java/org/jenkinsci/plugins/nomad/NomadWorker.java Show resolved Hide resolved

j3t commented Jul 28, 2021

View reviewed changes

src/main/java/org/jenkinsci/plugins/nomad/NomadWorker.java Show resolved Hide resolved

j3t commented Jul 28, 2021

View reviewed changes

src/main/java/org/jenkinsci/plugins/nomad/NomadCloud.java Show resolved Hide resolved

j3t commented Jul 28, 2021

View reviewed changes

src/main/java/org/jenkinsci/plugins/nomad/MigrationHelper.java Show resolved Hide resolved

j3t force-pushed the feature-81-more-flexible-worker-template branch 2 times, most recently from 8e94acd to e0d8cad Compare September 6, 2021 11:45

multani requested changes Oct 20, 2021

View reviewed changes

Jens Thielscher added 4 commits October 20, 2021 09:52

replace all job related form fields by a job template

827b95e

fix malformed HTML

698e123

refactoring

cf06c76

fix NPE

2a8fa1b

j3t force-pushed the feature-81-more-flexible-worker-template branch from e0d8cad to 2a8fa1b Compare October 20, 2021 07:55

Jens Thielscher added 3 commits October 20, 2021 10:39

fix spotbugs finding

1f184fd

remove unnecessary declaration

d004220

refactoring

1a610a0

j3t force-pushed the feature-81-more-flexible-worker-template branch from 7952cb1 to 1a610a0 Compare October 20, 2021 08:42

improve documentation

45466ad

Co-authored-by: Jonathan Ballet <jon@multani.info>

mark deprecated fields as such

a4a1c1b

Jens Thielscher added 2 commits October 21, 2021 09:55

mark old template classes as deprecated

25a1f0d

log migration process

fa82658

j3t force-pushed the feature-81-more-flexible-worker-template branch from 4415e57 to a983949 Compare October 22, 2021 12:15

add validation possibility for job file

2540039

j3t force-pushed the feature-81-more-flexible-worker-template branch from a983949 to 2540039 Compare October 22, 2021 12:24

j3t marked this pull request as ready for review October 25, 2021 12:48

multani approved these changes Oct 26, 2021

View reviewed changes

multani merged commit 42015f0 into jenkinsci:master Oct 26, 2021

This was referenced Oct 30, 2021

make agent.jar download optional #77

Closed

Add support for agent.jar #76

Closed

Allow to specify Java args for Jenkins client #26

Closed

Exception raised when no datacenter is given #22

Closed

j3t mentioned this pull request Nov 11, 2021

cleanup and polish codebase, improve test coverage and documentation #109

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace worker template by a more flexible approach #82

replace worker template by a more flexible approach #82

j3t commented Jul 27, 2021 •

edited

Loading

j3t left a comment

j3t Jul 28, 2021

mikedld commented Oct 18, 2021

multani commented Oct 18, 2021

j3t commented Oct 19, 2021

multani left a comment

j3t commented Oct 20, 2021 •

edited

Loading

j3t commented Oct 20, 2021

j3t commented Oct 20, 2021

multani commented Oct 21, 2021

j3t commented Oct 21, 2021

j3t commented Oct 21, 2021

multani commented Oct 22, 2021

j3t commented Oct 22, 2021

j3t commented Oct 22, 2021

j3t commented Oct 22, 2021

multani commented Oct 25, 2021

multani left a comment

multani commented Oct 26, 2021

multani commented Oct 27, 2021

replace worker template by a more flexible approach #82

replace worker template by a more flexible approach #82

Conversation

j3t commented Jul 27, 2021 • edited Loading

j3t left a comment

Choose a reason for hiding this comment

j3t Jul 28, 2021

Choose a reason for hiding this comment

mikedld commented Oct 18, 2021

multani commented Oct 18, 2021

j3t commented Oct 19, 2021

multani left a comment

Choose a reason for hiding this comment

j3t commented Oct 20, 2021 • edited Loading

j3t commented Oct 20, 2021

j3t commented Oct 20, 2021

multani commented Oct 21, 2021

j3t commented Oct 21, 2021

j3t commented Oct 21, 2021

multani commented Oct 22, 2021

j3t commented Oct 22, 2021

j3t commented Oct 22, 2021

j3t commented Oct 22, 2021

multani commented Oct 25, 2021

multani left a comment

Choose a reason for hiding this comment

multani commented Oct 26, 2021

multani commented Oct 27, 2021

j3t commented Jul 27, 2021 •

edited

Loading

j3t commented Oct 20, 2021 •

edited

Loading