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.

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

feat/windows slaves #148

Closed
wants to merge 9 commits into from
Closed

feat/windows slaves #148

wants to merge 9 commits into from

Conversation

rbutcher
Copy link
Contributor

@rbutcher rbutcher commented Aug 2, 2018

There are two existing pull requests trying to fix the problem of support for Windows slaves. This issue is described in JENKINS-36776, JENKINS-48518, JENKINS-51443, and JENKINS-52120.

PR-98 is more that a year old and is mostly the same changes as are in this PR.
PR-117 is a slimmer change that does not completely address Windows slave support.

While this is a duplicate of some of the other work already done I am opening this PR primarily to bring attention to this deficiency and the fact that this change is highly desired by the Windows developer community.

import hudson.LauncherDecorator;
import hudson.Proc;
import hudson.Util;
import hudson.*;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the aggressive import changes. Alt+Enter on intellij 😄
I can change this back if necessary.

@rbutcher
Copy link
Contributor Author

It's been a little over 2 weeks. Is there something I can do to get this reviewed and moving along?

@lglijo
Copy link

lglijo commented Sep 2, 2018

I so much want this fix.Really waiting to get this to work with windows images on windows boxes.
I approved the PR sometime ago, what is the process to approve and get this merged?

@lglijo
Copy link

lglijo commented Sep 2, 2018

@rbutcher I marked the issue as in progress here JENKINS-36776

@rbutcher
Copy link
Contributor Author

It's been about a month and a half since this was opened. What needs done to get this reviewed and possibly merged?

@henryborchers
Copy link

Same question as @rbutcher . I want to use Windows containers.

@jglick
Copy link
Member

jglick commented Sep 25, 2018

I do not think this plugin has a maintainer, and given that every change causes a critical regression, IMO it is best left untouched. (Particularly in this area, since we have no meaningful CI on Windows.) If you want to use Windows server containers in build steps, it is advisable to run Docker commands directly. You do not need any special plugin to do so.

@Snapstromegon
Copy link

I do not think this plugin has a maintainer, and given that every change causes a critical regression, IMO it is best left untouched.

I think that (as you have a CI and testing process on Linux to ensure nothing gets broken on Linux) its not the right way to break the CI usabilaty or such a large Audience. We were waiting for this for month and missing out this feature is preventing us from using Jenkins, because Docker (or Containers in general) is the only accaptable way for us to have a bundled, recreateable Environment in which we can repeat our CI Process. And since we are required to use Windows only tools, switching to Linux is not an option.

(Particularly in this area, since we have no meaningful CI on Windows.)

IMHO I think, that it's correct to say, that we shouldn't break Linux features to introduce Windows ones. But if I'm correct, it should be possible to test the Linux environment to be unchanged and as Windows currently doesn't have this feature, it's impossible to break it. It might create some new Bugreports, but IMO it's worse to just "ignore" the existing ones to preserve the stability of a feature for a platform it's not available for.

If you want to use Windows server containers in build steps, it is advisable to run Docker commands directly. You do not need any special plugin to do so.

Is there a way to run a Jenkins Plugin inside the container of which I don't know? Because transforming each container to a Jenkins Node creates way to much overhead and running the commands as "docker exec" / "docker run" isn't really usable.

I know, that my answer might seem a little harsh, but I think that theres nothing to break for the Windows CI world and that the gain would be massive, while the risk for Linux would be miniscule.

I hope someone will find the time and gives Jenkins and Docker for Windows some love and merges this (or a compareable) pull and maybe even to become a maintainer for this plugin.

@jetersen
Copy link
Member

jetersen commented Oct 9, 2018

@rbutcher we tested your branch and found that the method for whoAmI did not work because it is calling id function on Windows

@rbutcher
Copy link
Contributor Author

rbutcher commented Oct 9, 2018

@rbutcher we tested your branch and found that the method for whoAmI did not work because it is calling id function on Windows

You are totally right. When I setup my build machines I put the git /usr/bin on path so that folks can use things like sed.

I am going to push an override to the whoAmI function that will do the same thing, but without relying on id. The result of that function is not super important because it is not used to set the user inside the container because that works funky on Windows. The bind mount still results in correctly ACL'ed files that the worker process can still read.

Another thing to note, if your Jenkins workspace is on a drive other than C:\ you will run into container start issues too. For example, we have a C:\ and an M:\ drive. Jenkins workspace is at M:\j\workspace. The plugin will create the bind mount of M:\j\workspace\<build number>:M:\j\workspace\<build number>. This fails because the container does not know of any M:\ drive.

I am thinking to resolve this I would just replace M:\ in this case when constructing bind mounts. Any thoughts?

Finally, I am planning on maintaining the fork since this does not seem likely to progress. I will leave it open in case @jglick changes his mind.

@jetersen
Copy link
Member

jetersen commented Oct 9, 2018

Tbh on windows you can skip the user parameter so I would rather have it as a noop.
Then skip adding -u to docker run 😜

@Snapstromegon
Copy link

@Casz Thanks for being active on this PR.
IMHO Skipping the user parameter might be helpful for now, but maybe at some other point it's going to cause some trouble.
It's not recommended to run as su or admin inside the container. For most people this won't be a problem, because they are doing it anyways (and in a controlled environment like a CI system it's less of a problem), but someone will certenly run into unexpected errors when forcing a user.

For now I think skipping will be an okay solution to get Docker on Windows rolling, but someone should look into this in the future if this doesn't get solved in this PR.

@jetersen
Copy link
Member

If you had to use it you go: docker run -u ContainerUser or docker run -u ContainerAdministrator

-u, --user string                    Username or UID (format: <name|uid>[:<group|gid>])

I rarely know of any windows image that create users beside using the existing.

In this scenario I think the option should be given inside the agent { docker { options } } as user = 'ContainerUser'

@rbutcher
Copy link
Contributor Author

It's not recommended to run as su or admin inside the container. For most people this won't be a problem, because they are doing it anyways (and in a controlled environment like a CI system it's less of a problem), but someone will certainly run into unexpected errors when forcing a user.

I am aware of this. For the Windows containers we are getting ready to run in production we do create non-ContainerAdministrator users and what not. In this case it does not affect the users on the ACLs for the files written while executing in the container so I am less worried about problems. I did add an implementation that does provide a proper username on Windows. For example, on my work system it returns onbase\rbutcher. I would have to create the user in the container beforehand for that to be useful though, so it gets thrown away.

I am however looking for insight into how to handle the path mappings from any other drive to C:. I just don't know how common it is for non-C:\ drives on CI systems. Its common at my job because of antivirus scanning exceptions.

@jetersen
Copy link
Member

jetersen commented Oct 10, 2018

@rbutcher what I would do is just replace M: to C: and then when launcher is inside a docker container use ws() function to switch workspace.

We used this to shorten path because of compiler limitation and hardware limited us to one executor anyway.

So E:/jenkins/workspace/build/multibranch<branch-feature>-<hash> became J:/ with subst.

I don't have access to the source code any longer

But from memory

node ('bluetooth') {
  checkout()
  def workdir = pwd()
  bat "subst J: ${workdir}"
  ws('j:/') {
     bat 'make'
  }
}

Work flawlessly I guess the same would work for bind mount in docker.

@solvingj
Copy link

solvingj commented Nov 8, 2018

@Casz would you be willing to comment on whether there is any chance you will help get this PR through the pipeline and merged?

@jetersen
Copy link
Member

jetersen commented Nov 8, 2018

I have plenty of work to do and we are waiting for 1809 and new dotnet SDK docker image to do any serious work on windows 😅

@jglick
Copy link
Member

jglick commented Nov 14, 2018

running the commands as "docker exec" / "docker run" isn't really usable

Subjective I guess, but: #105

The ideal outcome from my PoV:

  • The withDockerRegistry and withDockerServer steps are moved to another plugin, maybe just docker-commons. (They could even be SimpleBuildWrappers were it not for the compatibility code in [JENKINS-51395] Syntax problems in withDockerRegistry #138.)
  • Someone other than myself or regular Pipeline developers maintains (the rest of) this plugin.
  • It is removed from the deps of workflow-aggregator and pipeline-model-definition, removed from Evergreen, and all mentions in jenkins.io documentation removed other than to say “do not use this”.
  • Pending something nicer, [JENKINS-48050] Declarative Pipeline support for dockerNode docker-plugin#681 is listed as the only supported integration of (non-Kubernetes) Docker containers in (Declarative) Pipeline.

But for that I need some commitment from @abayer, @ndeloof, and probably @bitwiseman etc.

@basdl
Copy link

basdl commented Jan 2, 2019

Is there any chance this PR will actually be merged?

@dpn982
Copy link

dpn982 commented Apr 18, 2019

What is the current status of this pull request?

@jetersen
Copy link
Member

jetersen commented Apr 19, 2019

I have a working branch, and we are using it with great success.
Have yet to finalize the branch there is some work to be done for them to be equal on Windows and Linux I'll see if I can get up next week.

Though for the work to be truly done it would require Windows agent with Docker on Jenkins CI.
I guess I could try and get some appveyor tests going.

@bitwiseman
Copy link

@dpn982
Are you interested in contributing to this PR?

@bitwiseman
Copy link

@Casz
We have windows agents on Jenkins CI now right? I assume they lack Docker though.
In any case, would it make sense to push what you have to this PR for folks to look at?

@jetersen
Copy link
Member

🎯 focused on JCasC atm 😓

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.

#148 (comment): I think it is unwise to make any but the most trivial changes to this plugin until the scary and misconceived parts are officially marked experimental and removed from standard guidelines.

@@ -75,6 +75,12 @@ class Docker implements Serializable {
new Image(this, id)
}

String shell() {
node {
Copy link
Member

Choose a reason for hiding this comment

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

Can be deleted.

@@ -93,7 +99,7 @@ class Docker implements Serializable {
def commandLine = "docker build -t ${image} ${args}"
def buildArgs = DockerUtils.parseBuildArgs(commandLine)

script.sh commandLine
script."${shell()}" commandLine
Copy link
Member

Choose a reason for hiding this comment

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

More simply:

Suggested change
script."${shell()}" commandLine
script[shell()] commandLine

@bitwiseman
Copy link

@jglick
You're talking about JENKINS-48050, right? Is there plan to land that?

If the general plan is to not make further changes to this plugin that needs to be communicated clearly at the top level of this project and there needs to be a person or group driving the effort to migrate to the new plugin.

In the absence of that, not changing this plugin doesn't seem like an option. People will continue to submit PRs for this plugin and there needs to be a path forward for them. What if anything can be done to make changes less risky?

@jglick
Copy link
Member

jglick commented Apr 22, 2019

You're talking about JENKINS-48050, right?

Well, not so much, since that seems to have been abandoned. See jenkinsci/docker-plugin#681 (comment) for discussion.

If the general plan is to not make further changes to this plugin that needs to be communicated clearly at the top level of this project and there needs to be a person or group driving the effort to migrate to the new plugin.

Yes and yes!

@sboehmann
Copy link

Sorry, but this is so frustrating! We worked two weeks to migrate from our old CI to Jenkins and now we've stumbled across this issue. First, we started running Linux builds (with Docker) and Windows builds (without Docker). Then we updated Windows (to Windows Server 2019) to use Docker. And now this! Why is there no warning anywhere that this plugin is dead?

Is there any easy-to-use and maintenable workaround or alternative?

@Snapstromegon
Copy link

Snapstromegon commented May 3, 2019

@sboehmann even though I did not test this yet:
As far as I know there is no easy clean way to run pure docker on windows with jenkins.
but
you could try using a kubernetes cluster and add a windows node. This might solve your problem as kubernetes doesn't use that type of volume definition, which is problematic in docker for windows.

Adding a kubernetes Windows Node

Using kubernetes with jenkins works fine and even better when you deploy your jenkins in the cluster itself. But be aware that setting up a kubernetes cluster is harder than a docker swarm.

@sboehmann
Copy link

you could try using a kubernetes cluster and add a windows node. This might solve your problem as kubernetes doesn't use that type of volume definition, which is problematic in docker for windows.

@Snapstromegon That might work. But it's way too much for us. We build C/C++ software and just wanted to use Jenkins/Docker to have a clean build environment and a simpler setup of the build machines. For Linux this works very well, but Windows is just as important. Therefore Jenkins doesn't seem to be a good choice for us. It was an experiment and unfortunately it failed. We will investigate another CI.

@jjathman
Copy link

jjathman commented May 3, 2019

@sboehmann what my company did was just wrote some powershell scripts to emulate what Jenkins does for Linux. This plugin makes working with containers very easy, but for windows you can still execute the same Docker commands. You just need to write it yourself. As long as you use some shared Jenkins scripts, it isn’t too bad. Just a handful of commands.

@sboehmann
Copy link

@jjathman Yes, we already have a shared library. We already build 50+ repositories - practically always in the same way. So our entire pipeline script is in a shared library.

I'll try your suggestion. But would you be so kind to share your shell script? Or could you give some more hints on how to write such a script? Sorry, but I'm really new to the Windows/Docker universe (I'm more the Unix guy). And it would probably also help others who run into this issue.

@ghost
Copy link

ghost commented May 3, 2019

@sboehmann :

I understand your frustration, we had to solve it as well. That said, we're sticking with Jenkins for now.

Here's how to run Docker builds on Windows 2016/2019.

Edit: I replaced the code with a link to Stackoverflow where I posted the answer.

@rgl
Copy link

rgl commented May 3, 2019

@gyorgynadaban looks very interesting! thanks for sharing!

Can you please explain what is the SCRIPTS environment variable, the initializeWorkspace, publishAllArtifacts and updateJira functions?

Is this kind of Jenkinsfile considered a Declarative Pipeline or Scripted Pipeline?

@sboehmann
Copy link

@gyorgynadaban Thank you very much. I was really frustrated, sorry for that.
But with your example I almost made it work now. Thanks!

However, I still have one (last?) problem. How can I pull the image from my private registry. Or especially, how can I get the already configured registry (configured for docker-workflow-plugin)?

@ghost
Copy link

ghost commented May 3, 2019

@rgl : I think this is considered a scripted pipeline.

SCRIPTS is just an environment variable, you'd have similar ones based on your actual implementation, that you can pass on to the Docker container. You'd have to implement it as it's not in my example code, but it's definitely not hard.

Function explanations:
initializeWorkspace: You would do pre-build initialization here, e.g. create artifact directories, copy helper scripts to the workspace, etc.
publishAllArtifacts: The way I opted to implement support for various output types is handled here, basically this function looks at a predefined path to see if there's anything to publish.

Eg. if you create directories like this in the workspace:
__artifacts\chocolatey
__artifacts\nuget
__artifacts\npm
__artifacts\tests\nunit
__artifacts\tests\junit
__artifacts\tests\mstest

Then in your publishAllArtifacts function you'd simply scoop up output types and publish to your choice of artifact destination. It helps manage multi-output projects eg. which produce more than one type of output, e.g. mix of Chocolatey packages, NUnit and JUnit test results.

updateJira: If you use Jira issue tracking, you might want to add a comment to the corresponding case with the build results. This is completely optional as well, but the idea is that your branch name can be e.g. your jira case ID.

@ghost
Copy link

ghost commented May 3, 2019

@sboehmann : No worries, I was as frustrated as well before I had the solution worked out, I'm glad I could help. :)

As for image registry, you'd need to do 2 things:

  1. Add your registry to the Docker settings file
  2. If necessary, log into your private registry with the user that's running the build. Alternatively, you can do a Docker login command as part of your init phase, which is what I did.

Your registry would be configured in C:\programdata\Docker\config\daemon.json on your Jenkins node:

{
    "registry-mirrors": [ "https://docker-dev.myserver.net" ], 
    "hosts": [ "tcp://0.0.0.0:2376", "npipe://" ]
}

The authentication step depends on whether your registry requires authentication to get or publish images, and it depends on your individual setup.

@rgl
Copy link

rgl commented May 4, 2019

@gyorgynadaban I see... so those are your custom functions! are you putting them in a shared library like described at https://jenkins.io/doc/book/pipeline/shared-libraries/? or can they also go somewhere in the workspace?

@ghost
Copy link

ghost commented May 4, 2019

@rgl : Yes, those are shared libraries. I found it most practical to have a repository with the shared libraries that are loaded implicitly in the Jenkins job.

@solvingj
Copy link

solvingj commented Jun 6, 2019

@Casz can you share your working branch, or even a built version, so that some of us who are somewhat desperate could deploy it locally to our dev environments and provide feedback sooner rather than later?

@jetersen
Copy link
Member

jetersen commented Jun 6, 2019

I'll try and get around to it tomorrow.

@recampbell
Copy link
Member

Hi @Casz -- I just wanted to check in here. This is a really exciting feature!

@clement-cln
Copy link

Hi guys, is there any update about the potential merge of this feature ?

@jetersen
Copy link
Member

here is my updated branch: #184

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.