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

ci-public: replace multijob with freestyle for custom-suites #1436

Closed
wants to merge 2 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented Aug 9, 2018

I've configure a new job https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/ (job XML attached).
This should replace https://ci.nodejs.org/job/node-test-commit-custom-suites/ in node-test-commit, as the current one is based on multijob which ignores the PLATFORM_LABEL parameter, and is unneccecerly combersome in this scenario.
Because of its limitations the current job is recursive (the flyweight job runs on a jenkins-workspace node and the actual build also runs on a jenkins-workspace node) so when the systems under load it can lead to unexpected behavior.

This is the diff from the current job https://www.diffchecker.com/MKqrFehH

I've configure a new job https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/ (job XML attached).
This should replace https://ci.nodejs.org/job/node-test-commit-custom-suites/ in node-test-commit, as the current one is based on `multijob` which ignores the `PLATFORM_LABEL` parameter, which is unneccecery in this scenario.
Because of its limitations the current job is recursive (the flyweight job runs on a `jenkins-workspace` node and the actual build also runs on a `jenkins-workspace` node) so when the systems under load it can lead to unexpected behavior.

This is the diff from the current job https://www.diffchecker.com/MKqrFehH
maclover7

This comment was marked as off-topic.

@richardlau
Copy link
Member

If this is for use in node-test-commit, does it need to be custom-suites with all the customisation, or should it be a node-test-worker (since currently the chained job is for running with --worker)?

@refack
Copy link
Contributor Author

refack commented Aug 10, 2018

If this is for use in node-test-commit, does it need to be custom-suites with all the customisation, or should it be a node-test-worker (since currently the chained job is for running with --worker)?

It was designed for running customizable tests. We are already using it in three configurations:

  1. to run the default suits with --worker - example
  2. as part of daily-master to run the internet suite - example
  3. as part of the test-v8 job to test the v8-update suite - example

So IMHO the conscumizations have proven very (re)useful.

@refack
Copy link
Contributor Author

refack commented Aug 10, 2018

Due to an apparent degradation of stability, I've implemented the switch, and am monitoring the situation.

@richardlau
Copy link
Member

richardlau commented Aug 10, 2018

@refack I agree that customisations are very useful, especially for one off runs (e.g. like the stress job). My query is whether we want to be reusing the same job in this way as part of larger jobs/flows/pipelines? For example, it's harder to see the history of --worker runs because the job history will be a mixture of the three scenarios you have outlined (plus any manual runs).

@refack
Copy link
Contributor Author

refack commented Aug 10, 2018

My query is whether we want to be reusing the same job in this way as part of larger jobs/flows/pipelines?

It's a balance of useability/reusability... This case is actually a good example, as if we had 3 seperate jobs it would have been harder to identify the issue and correct the configuration.
So IMHO for the time being using it for --worker is a good solution. But we should be exploring better code reuse solutions, such as project inheritance, managed scripts and shared groovy steps.

@maclover7
Copy link
Contributor

My query is whether we want to be reusing the same job in this way as part of larger jobs/flows/pipelines? For example, it's harder to see the history of --worker runs because the job history will be a mixture of the three scenarios you have outlined (plus any manual runs).

To view the history of --worker builds, you can check out the node-test-commit-custom-suites part from https://nodejs-ci-health.mmarchini.me/#/detailed-jobs

@richardlau
Copy link
Member

@maclover7 but are all of those are --worker builds (e.g. https://ci.nodejs.org/job/node-test-commit-custom-suites/410/ isn't a --worker build)?

@refack
Copy link
Contributor Author

refack commented Aug 10, 2018

FTR the issue that the jobs were running on jenkins-workspace nodes, was because I did not RTFM and missed that there's a specific way to trigger such jobs. (That is most probably why we have been seeing very frequent infra related failure these last couple of days.
image

@sam-github
Copy link
Contributor

@richardlau is this still relevant? in-progress? already done some other way? Maybe its stale and should be closed, but its not immediately obvious, and it looks like you've refed it in an open issue.

@richardlau
Copy link
Member

https://ci.nodejs.org/job/node-test-commit-custom-suites/ is disabled in Jenkins and has a description saying to instead use https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/ (incidentally I've just PR'ed a change to the COLLABORATOR_GUIDE (nodejs/node#31557) in core to update the job link).

I think we can just close this out. I don't know what policy (or if we have one written down) covers what is backed up to https://github.com/nodejs/build/tree/master/jenkins/xml but it's hard to tell from a glance if the XML in this PR is current (i.e. has anyone changed the job configuration since). In any case backing up Jenkins changes from test and release CI is being handled by the proposed scripts @rvagg is working on in #2133.

@richardlau richardlau closed this Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants