jenkinsci / nomad-plugin

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

Add force_pull, volumes, switch user for Docker driver #23

Closed zstyblik closed 6 years ago

zstyblik commented 7 years ago

MR is more or less up for discussion. I'm ok with splitting it up into multiple MRs, if required, or re-working some features if necessary.

MR adds the following features:

Cheers :)

zstyblik commented 7 years ago

Thank you for comments @jovandeginste and I'm sorry for wasting a bit of time. I found out later on, when actually trying to build HPI, that unit tests are broken. I mean, all those comments are valid and I have addressed them in build branch where I needed these features and [[ https://github.com/jenkinsci/nomad-plugin/pull/17]], but I didn't have time to back-port them here. Now, I'm a bit ill, but I'll get to it and back-port those fixes.

Thank you and my apologies.

jovandeginste commented 7 years ago

@zstyblik I understand.

I also think there is a better way to implement the switchUser part (at the Nomad side). Nomad supports the user stanza at the task level, which I think does exactly what you implemented. I'm testing my changes to your PR on my environment now to verify them.

jovandeginste commented 7 years ago

@zstyblik Please find my suggestions + fixes in PR on your fork

jovandeginste commented 7 years ago

@zstyblik may I suggest you squash or rebase your commits, so the double changes are gone? This would make reviewing the changes by other contributors easier. While rebasing, I would suggest you still split the implementation of the different options into separate commits (as you did), but limit to three commits (one for every option).

jovandeginste commented 7 years ago

I'm now running this PR in my production Jenkins setup. Will report back next week (but the jobs I ran yesterday where all OK, so I don't expect any issues).

zstyblik commented 7 years ago

@jovandeginste, I've been thinking about what you've suggested, but I'm still not sure if I follow. To go back and fix unit tests in those commits without having an extra commit to do that, sure thing. Now, what do to about switch user, resp. your commit, I don't know. If you have any hints or something specific in mind, please, let me know. Otherwise I will give it some more thought and try to come up with something on my own in the evening. ;)

jovandeginste commented 7 years ago

You can use my changes, I give you permission for that. Maybe reference me in the commit message ;-)

zstyblik commented 7 years ago

Alright, I've cleaned up a bit. @jovandeginste, please, see the commit message and let me know if you're happy with it. ;)

jovandeginste commented 7 years ago

@zstyblik Had a quick look, will run the new code in production this afternoon (GMT+2)

jovandeginste commented 7 years ago

(Code is running for some time now, all seems well)

jovandeginste commented 6 years ago

No issues so far, LGTM

zstyblik commented 6 years ago

@jovandeginste, I'd say your removal of % su; has solved issues with java. Other than that, changes in this MR do work for me without any issues.

thatsk commented 6 years ago

@jovandeginste if I want use your strategy how can change it and make it work in Jenkins. Because I am also facing issue.can you tell me

jovandeginste commented 6 years ago

@thatsk I'm not sure I understand the question. Do you need the compiled plug-in?

thatsk commented 6 years ago

Yes.. how to use latest fix you have resolved you have requested to merge that. But not yet updated in Plugin

jovandeginste commented 6 years ago

I have a branch "work" in my fork, if you build that you have all the pr's here. If you don't know how to build a plug-in, it's very easy... I can provide you with a hpi-file tomorrow.

thatsk commented 6 years ago

Yes. @jovandeginste you are awesome

thatsk commented 6 years ago

Just let me know if I want also contribute how can I build hpi file for this project

jovandeginste commented 6 years ago

@thatsk if you have Maven installed, it should be as simple as mvn package if I remember correctly.

thatsk commented 6 years ago

Ok .. thanks alot. I will try that let you know. And waiting for your hpi

thatsk commented 6 years ago

@jovandeginste thanks for informing. But when I bind volume and checked nomad logs it is compulsory to provide constraint. Can you tell me an example if I want to provide constraint? demo

jovandeginste commented 6 years ago

@thatsk If you read https://www.nomadproject.io/docs/job-specification/constraint.html, you will find a few examples.

thatsk commented 6 years ago

@jovandeginste As per your comment, i have gone through comment configured as below. The container can bind host path started correctly but does not build the plan again it is going to create another container when I check the alloc-status it shows after one min it is killed don't know why and also build plan in the queue. and also when I check nomad client log it shows fetching stats error in gc alloc agentnomad

task. please let me know if you need any further information. Slave template image. slavetemplate

thatsk commented 6 years ago

Also is there any connection problem if I am using official nomad plugin 0.4 on Jenkins master and 0.5 on another Jenkins master.will it cause problem

jovandeginste commented 6 years ago

@thatsk As you blanked out the docker image name, I assume you built it yourself; does it contain Java? Is Java readily accessible (no special PATH needs to be set?)

thatsk commented 6 years ago

@jovandeginste Sorry my mistake i have passed wrong path url of jenkins url and slave agent thats why its not passing. Appreciate for your help. Its nice to talk with you.

thatsk commented 6 years ago

You must have to merge it, man. The new features are awesome.

zstyblik commented 6 years ago

@jippi ?

thatsk commented 6 years ago

@jovandeginste if i want to bind multiple volume is this correct way to add multiple volume. I want to try dind. 1

I am not able to detect logs or how can i find what cause to fix this.

jovandeginste commented 6 years ago

@thatsk I would expect this to be the case. I'm not using more than one volume myself...

zstyblik commented 6 years ago

@thatsk, that's the correct way, yes :)

thatsk commented 6 years ago

but its not working if i use more than one volume. The container won't start

zstyblik commented 6 years ago

And what's the error then?

thatsk commented 6 years ago

thanks man its somehow missing the folder in the container. that's why its not mounting . Great work guies much appreciated.