jenkinsci / nomad-plugin

Nomad cloud plugin for Jenkins
https://plugins.jenkins.io/nomad/
MIT License
56 stars 41 forks source link

fix(pruneOrphan): convert nomad expirtyTime from ns to ms #170

Open arsiesys opened 1 year ago

arsiesys commented 1 year ago

We are using Nomad 1.4.3 and jenkins 2.361.4.

I noticed that the pruneOrphan feature wasn't working on our side. After digging, I noticed two anomalies:

I address the previous anomalies by converting the parsed values to milliseconds and update the expirtyTime variable with the added timeout.

I did a small fix that seems to work on our side based on my first tests:

Dec 19, 2022 9:17:45 PM FINE org.jenkinsci.plugins.nomad.NomadCloud
Found Orphaned Node: jenkins-xx-xxx-4c83d5ce4910a6 in namespace default in region global

I am far away of being an expert with Java. If this PR need some "touch up", please help! :)

Thanks !

arsiesys commented 1 year ago

Also, I think this fix should come with a warning.. As this feature wasn't working before. In the case of multiple separated jenkins instance using the same nomad configuration and mostly the same "job prefix" name, instance A could remove the job of the instance B if they use the same prefix. Each jenkins instance using the same nomad cluster should use differents prefix in their nomad templates.

j3t commented 1 year ago

Also, I think this fix should come with a warning.. As this feature wasn't working before.

Makes sense, we could add something like this to the release notes I guess.

In the case of multiple separated jenkins instance using the same nomad configuration and mostly the same "job prefix" name, instance A could remove the job of the instance B if they use the same prefix. Each jenkins instance using the same nomad cluster should use differents prefix in their nomad templates.

tbh, I'm not familiar with the prune feature but I guess you are right, that could be the case. We should add a warning which is displayed right to the prune option so that the user is at least aware of the risk. Maybe we could also migrate the cloud configurations so that the prune feature is deactivated for all existing configurations and must be activated actively.

aarnaud commented 1 year ago

Hi, When this PR can be merge, it's a bugfix we need because nomad jobs aren't clear correctly. If the there is breaking changes / behaviour changes we can change the version to increment the major version ?

aarnaud commented 1 year ago

I tested and running this patch in our environment its work well