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

Update Windows Agent Installer to 1.7 and WinSW to 2.0.2 #2765

Conversation

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Feb 27, 2017

This change integrates major WinSW fixes into Jenkins. The change is WiP, jenkinsci/windows-slave-installer-module#6 to be integrated at least

WinSW changes

The update includes many fixes and improvements, the full list is provided in the WinSW changelog. There are several issues referenced in Jenkins bugtracker:

  • JENKINS-22692 - Connection reset issues when WinSW gets terminated due to the system shutdown
  • JENKINS-23487- Support of shared directories in WinSW
  • JENKINS-39231 - Enable Runaway Process Killer by default
  • JENKINS-39237 - Auto-upgrade of JNLP agent versions on the slaves

Windows Agent Installer changes

  • Adapt the default configurations to pick fixes above
  • Slave => Agent renaming where possible

Jenkins core changes

  • Modify the configuration template, reference advanced options
  • Enable Runaway Process Killer by default

@reviewbybees

@@ -23,15 +23,22 @@ THE SOFTWARE.
-->

<!--
Windows service definition for Jenkins slave
Windows service definition for Jenkins agent.
The actual version of this definition has been moved to https://github.com/jenkinsci/windows-slave-installer-module/blob/master/src/main/resources/org/jenkinsci/modules/windows_slave_installer/jenkins-slave.xml.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Feb 27, 2017

Author Member

I do not see any usages, but this file can be downloaded as a resource.
So I'd prefer to keep it up to date

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Feb 27, 2017

Member

Left behind from

String xml = IOUtils.toString(WindowsSlaveInstaller.class.getResourceAsStream("/windows-service/jenkins-slave.xml"), "UTF-8");
it seems -- probably time to remove it from core?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Feb 27, 2017

Author Member

IMHO then we need to add the resource alias somehow

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Feb 28, 2017

Author Member

OK, I have found no legitimate way to use this file excepting the access to the master's classloader from plugins (why would they do it). No occurrence in OSS repos as well.

Will remove the file

Now it is within windows-slave-installer
war/pom.xml Outdated
@@ -114,7 +114,7 @@ THE SOFTWARE.
<dependency>
<groupId>org.jenkins-ci.modules</groupId>
<artifactId>windows-slave-installer</artifactId>
<version>1.6</version>
<version>1.7-SNAPSHOT</version>

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Feb 28, 2017

Member

Could you upload a snapshot and reference it by timestamp?

@oleg-nenashev
Copy link
Member Author

oleg-nenashev commented Mar 3, 2017

Created PR with some upgrade docs: jenkinsci/windows-slave-installer-module#9

@oleg-nenashev
Copy link
Member Author

oleg-nenashev commented Mar 4, 2017

@jenkinsci/code-reviewers since I was unable to get feedback from @reviewbybees in time :(

@reviewbybees
Copy link

reviewbybees commented Mar 5, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@oleg-nenashev
Copy link
Member Author

oleg-nenashev commented Mar 6, 2017

@reviewbybees ping. 1-week review timeout is today

@jglick
jglick approved these changes Mar 6, 2017
Copy link
Member

jglick left a comment

I presume you have done manual testing on various platforms. Given the magnitude of the changes, meaningful code review seems hopeless.

@@ -755,7 +755,7 @@ THE SOFTWARE.
<artifactItem>
<groupId>com.sun.winsw</groupId>
<artifactId>winsw</artifactId>
<version>1.16</version>
<version>2.0.2</version>

This comment has been minimized.

Copy link
@jglick
@@ -114,7 +114,7 @@ THE SOFTWARE.
<dependency>
<groupId>org.jenkins-ci.modules</groupId>
<artifactId>windows-slave-installer</artifactId>
<version>1.6</version>
<version>1.7</version>

This comment has been minimized.

Copy link
@jglick
@jglick
Copy link
Member

jglick commented Mar 6, 2017

21:08:37 [Windows] [ERROR] npm ERR! code ECONNRESET

is likely CI flakiness.

@oleg-nenashev
Copy link
Member Author

oleg-nenashev commented Mar 6, 2017

@jglick

I presume you have done manual testing on various platforms. Given the magnitude of the changes, meaningful code review seems hopeless.

Yes, tested changes on 3 platforms (Win 10, Win Server 2012, Win 7).
I would also like to test the stuff on WinXP, but I have no such machine. In the case of WinSW there was one bug report to this platform, which has been fixed in 2.0.2

@reviewbybees done

@oleg-nenashev
Copy link
Member Author

oleg-nenashev commented Mar 8, 2017

Got +1 from @daniel-beck in IRC

@oleg-nenashev oleg-nenashev merged commit e698d1d into jenkinsci:master Mar 8, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/jenkins/pr-head This commit cannot be built
Details
Jenkins This pull request looks good
Details
oleg-nenashev added a commit to oleg-nenashev/jenkins.io that referenced this pull request Mar 9, 2017
oleg-nenashev added a commit to oleg-nenashev/jenkins that referenced this pull request Mar 13, 2017
Windows Slaves plugin performs a direct access to the resources bundled into the core.
Hence the file removal was a bad idea though I have not seen the issue in automatic tests and ATH.

This change also was a last-minute change in jenkinsci#2765 in order to address suggestions from @daniel-beck, hence I didn't test it properly
oleg-nenashev added a commit that referenced this pull request Mar 19, 2017
… restore compatibility with windows-slaves 1.2 (#2803)

* [JENKINS-42724] - Restore the jenkins-slave.xml file

Windows Slaves plugin performs a direct access to the resources bundled into the core.
Hence the file removal was a bad idea though I have not seen the issue in automatic tests and ATH.

This change also was a last-minute change in #2765 in order to address suggestions from @daniel-beck, hence I didn't test it properly

* [JENKINS-42724] - Update the Windows Agents plugin dependency to 1.3.1

* [JENKINS-42724]  -Revert the war/pom.xml upgrade
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.