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

NXBT-2300: cleanup windb job #22

Merged
merged 3 commits into from
Jul 5, 2018
Merged

Conversation

alexistimic
Copy link
Contributor

@alexistimic alexistimic commented Jun 5, 2018

@jcarsique jcarsique force-pushed the feature-NXBT-2300-windb-cleanup branch from 2bc5632 to 25791ba Compare June 5, 2018 12:53
@jcarsique
Copy link
Contributor

See https://jenkins.io/doc/pipeline/examples/#jobs-in-parallel ?

I'd recommend to use the @NonCPS for the loops.

@alexistimic alexistimic force-pushed the feature-NXBT-2300-windb-cleanup branch from cc14ba0 to 52ec628 Compare June 6, 2018 10:16
@alexistimic alexistimic force-pushed the feature-NXBT-2300-windb-cleanup branch 2 times, most recently from d27a8b2 to 2884e33 Compare July 4, 2018 15:20
pipeline 'bat' step + PowerShell execution
@jcarsique jcarsique force-pushed the feature-NXBT-2300-windb-cleanup branch from 078d08e to 95de6d7 Compare July 4, 2018 18:03
Replace 'bat' + PowerShell with 'sh' + Bash

Reviewed-by: Julien Carsique <jcarsique@nuxeo.com>
@jcarsique jcarsique force-pushed the feature-NXBT-2300-windb-cleanup branch from 95de6d7 to ea75648 Compare July 4, 2018 18:59
Copy link
Contributor

@jcarsique jcarsique left a comment

Choose a reason for hiding this comment

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

LGTM after recommended changes.


Next improvement is likely to parallelize the tasks and handle busy nodes.

Because of the planed deletion, it makes sense that the node must be fully idle (even if or especially if there are multiple executors).

But the node() step reserves an executor and waits for it being assigned. So it would also work fine for the busy nodes while they have only one executor: the task will wait for being accepted.

Then it would be required to parallelize the cleanup tasks. Anyway, that is already whished if not required.
See https://support.cloudbees.com/hc/en-us/articles/230922168-Pipeline-Parallel-execution-of-tasks

Finally, in case of a node with multiple executors, you would have to find a strategy to forbid simultaneous execution of other tasks, or skip the cleanup.

* - Iterates through windb1 to windb8 in order to cleanup workspaces
*/

def PowerShell(psCmd) {
Copy link
Contributor

Choose a reason for hiding this comment

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

candidate for lib?


def PowerShell(psCmd) {
psCmd=psCmd.replaceAll("%", "%%")
bat "powershell.exe -NonInteractive -Command \"\$ErrorActionPreference='Continue';[Console]::OutputEncoding=[System.Text.Encoding]::UTF8;$psCmd;EXIT \$global:LastExitCode\""
Copy link
Contributor

Choose a reason for hiding this comment

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

multiline to ease maintenance?

if ((aNode.getNodeName().contains('windb')) && (aNode.toComputer().isOnline()) && (aNode.toComputer().countBusy() == 0)) {
winNodes += aNode.getDisplayName();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

provide output and clearer steps about the (ignored) nodes, for instance:

import jenkins.model.Jenkins

def winNodes = [];
for (slave in jenkins.model.Jenkins.instance.getNodes()) {
    def nodeName = slave.getNodeName()
    if (!nodeName.contains('windb')) {
        continue
    }
    def computer = slave.toComputer()
    if (!computer.isOnline()) {
        println "Node '$nodeName' is offline - skip cleanup"
    } else if (computer.isIdle()) {
        winNodes.add(nodeName)
    } else {
        println "Node '$nodeName' is busy - skip cleanup"
    }
}

Use computer.isIdle() instead of computer.countBusy() == 0 (although equivalent since countBusy() = countExecutors()-countIdle())

println("Cleaning $winNodes...");

for (winNode in winNodes) {
node("${winNode}") {
Copy link
Contributor

Choose a reason for hiding this comment

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

node(winNode) does not work?

@jcarsique
Copy link
Contributor

👍

1 similar comment
@tmartins
Copy link
Member

tmartins commented Jul 5, 2018

👍

@jcarsique jcarsique merged commit 6513968 into master Jul 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants