-
Notifications
You must be signed in to change notification settings - Fork 4.8k
jenkins master/slave cleanup #11844
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
jenkins master/slave cleanup #11844
Conversation
|
Note - still have to rework the kubernetes extended test to no longer use the old master/slave templates. But I wanted to get review of the doc going. I should be able to simplify it and match it to the new master/slave example ... create a sample job similar to the pipeline plugin test that restricts the job to the nodejs slave. |
| plugin and define the sample job used by the tutorial. | ||
| The sample builds upon the [sample job](https://github.com/openshift/origin/blob/master/examples/jenkins/README.md) | ||
| described in this file's parent directory, and leverages the OpenShift Jenkins slave image for NodeJS to launch the sample | ||
| job in a Jenkins slave provisioned as Kubernetes Pod on OpenShift. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the job actually reliant on using the nodejs slave image as opposed to the base slave image? (it's not doing anything nodejs specific, is it?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I guess the base image isn't available as a slave in the default jenkins image configuration. still it would be worth noting why we're using the nodejs slave)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example/jenkins/application-template.json is nodejs based....the src git repo in the build config is https://github.com/openshift/nodejs-ex.git and and the source strategy from clause is an ImageStreamTag for nodejs-010-centos7:latest. Are you saying to just point out those two details?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh ok. for some reason i thought the example was ruby based. so nevermind. (though it's still true that the jenkins job itself doesn't need local nodejs tools on the slave pod, because it's just triggering a build in openshift, it's not running any nodejs commands itself)
So there's probably nothing additional to say... it's still a bit misleading that we use a nodejs slave image when we don't actually leverage the fact that it has nodejs tooling in it, but probably not worth discussing here. It's at least less confusing than what i thought we were doing, which was using the nodejs slave image with a ruby application.
| matrix-project:1.6 | ||
| scm-api:1.0 | ||
| script-security:1.13 | ||
| junit:1.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that this exists and would need to be maintained makes me confident we are making the right choice in dumping this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK ... merging into the top level README and completely removing master-slave has been pushed
| @@ -1,97 +1,37 @@ | |||
| Jenkins Master->Slave Example | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is going to be the only file left in this directory, right? maybe we should just append this content to the jenkins/README.md as a secondary tutorial/extended example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is the only file. I'm good with moving the content into the base README and nuking the master-slave subdir. I'll go with that approach (thought it may be an hour or so until I get to it, in case you want to ruminate on it some more and end up deciding against this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so far i still like it :)
4403dbd to
61ce991
Compare
|
@gabemontero doc changes look good. sounds like you've already looked into the extended tests but please do a general grep of the codebase for master-slave to see anywhere else we might have been referencing this. Also the official docs. |
|
Good call - turns out ./test/integration/build_admission_test.go leverages the jenkins master template. Will have to update that one as well. the openshift-docs grep came up clean. |
| for Centos and RHEL produced by the [OpenShift Jenkins repository](https://github.com/openshift/jenkins). The OpenShift | ||
| Jenkins repository also produces a [base Jenkins slave image](https://github.com/openshift/jenkins/tree/master/slave-base), | ||
| as well as sample Jenkins slave images for [Maven](https://github.com/openshift/jenkins/tree/master/slave-maven) and | ||
| [NodeJS](https://github.com/openshift/jenkins/tree/master/slave-nodejs) which extend that base Jenkins slave image. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should refer to these as "sample" images. We (Red Hat) ship these images, and as such they should be supported.
I also feel that telling customer that they should use these images as "samples" to build other images, portrays a very unpolished look and feel to the product. We should ship images for all of the supported SCL images, if we are going to promote this route.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sferich888 per the exchange between @bparees and I below, is "reference" adequate for you, or simply dropping the word "sample" from the referenced phrase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropping the word sample is better IMO, however it may lead to question about when other slave images will be provided and supported.
For now dropping "sample" is the best solution, and we can deal with the RFEs should we get them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK "sample" remove in ref to slave images pushed
|
On Thu, Nov 10, 2016 at 2:52 PM, Eric Rich notifications@github.com wrote:
|
|
reference or just dropping sample is fine with me. |
3deb30f to
44f7d61
Compare
|
Pushed the ext/integ test updates for nuking master-slave. Also fixed per our brief changed the plugin extended test job configs for the ramifications from the jenkins example app changing its ict to auto=false |
|
and removed the WIP/DO NOT MERGE moniker |
|
This PR will need the jenkin image update with v1.0.33 of openshift pipeline from openshift/jenkins#194 for the plugin extended tests to pass, since some of the test jobs start with a scale frontend 0 call. |
| // pipeline buildconfig's successfully, so we're not using the standard jenkins template. | ||
| // but we do need a template that creates a service named jenkins. | ||
| template, err := testutil.GetTemplateFixture("../../examples/jenkins/master-slave/jenkins-master-template.json") | ||
| template, err := testutil.GetTemplateFixture("../testdata/jenkins-template.json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd like to see this template stripped down to the bare minimum (just creates a service named jenkins).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
template pruned
44f7d61 to
8a3f0b6
Compare
|
[testextended][extended:core(jenkins)] |
|
travis failure was a govet issue which on the extended tests, Haoran Wang's new pipeline build test failed. The failure looks orthogonal to this PRs changes. The diagnositic dump included this: The Build appears to have been created: I don't see any pod events related to the build. If we need say build controller logs to see why the build that was created was never started, the current debug in the extended tests do not included that. Thoughts on how to diagnose the pipeline failure @bparees or @csrwng ? Otherwise, the extended test changes related to the PR did pass. One could argue this could be merged, and we diagnose the pipeline failure (possibly add additional debug for these type of failures) with a separate PR. |
|
[testextended][extended:core(jenkins)] |
|
Evaluated for origin testextended up to 8a3f0b6 |
|
lgtm for a post 3.4 merge, but i'd definitely feel better if the pipeline test passes (Though i agree it doesn't seem related) |
|
continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/794/) (Base Commit: 9c4f9ee) (Extended Tests: core(jenkins)) |
|
On Mon, Nov 14, 2016 at 10:45 AM, Ben Parees notifications@github.com
Cool deal - and the extended test passed this time. So the prior failure If we wanted to add debug to capture why a build that was created timedout
|
|
@gabemontero the build that never started is a pipeline strategy, so there's no pod associated. we'd have to dump the jenkins pod logs and hope the sync plugin contained some useful debug. This could be the timing window that i think @jim-minter noticed: openshift/jenkins-sync-plugin#106 |
|
On Mon, Nov 14, 2016 at 10:57 AM, Ben Parees notifications@github.com
I see ... yeah, we are already dumping the jenkins pod logs. My
|
|
So otherwise, I'll bump when 3.4 ships and the gates open for 3.5. Thanks. |
|
sgtm |
|
[merge] |
|
Evaluated for origin merge up to 8a3f0b6 |
|
[Test]ing while waiting on the merge queue |
|
Evaluated for origin test up to 8a3f0b6 |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11515/) (Base Commit: a4b7101) |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11518/) (Base Commit: 3970bca) (Image: devenv-rhel7_5372) |
Fixes #8980
@bparees ptal