disable jenkins auto-deployment #10260

bparees · 2016-08-08T04:00:26Z

also fixes #10257

bparees · 2016-08-08T04:01:50Z

[test]

bparees · 2016-08-08T04:41:20Z

deads2k · 2016-08-08T13:38:13Z

pkg/cmd/server/origin/master_config.go

@@ -293,7 +293,7 @@ var (
 		"ProjectRequestLimit",
 		"OriginNamespaceLifecycle",
 		"PodNodeConstraints",
-		"JenkinsBootstrapper",
+		//"JenkinsBootstrapper",


Removing it this way makes it essentially impossible to turn back on. If you want it off by default, use the code here: https://github.com/openshift/origin/blob/master/pkg/cmd/server/start/admission.go#L43

when we're ready to turn it on, it will be part of a new release that enables the feature. we're not going to ship doc to customers about how to turn on a feature we deemed not ready for consumption.

smarterclayton · 2016-08-08T15:49:42Z

I don't want to disable the admission controller. We should be using the enabled flag, and having ansible set it to "off" for enterprise.

bparees · 2016-08-08T21:04:03Z

@smarterclayton @deads2k admission controller has been restored, but i'm not sure what template to have it instantiate. The problem:

We need to pick a template name for it to instantiate by default (today that name is "jenkins")
We need to register (or tell people to register) that template in the openshift namespace
Today we have 2 potential templates for them to register (jenkins-ephemeral and jenkins-persistent) and neither of them are named "jenkins" currently.
We can't name them both jenkins since they we can't register them both and make them both available to users.

So we can:
a) introduce a third template again (this PR is deleting what was that third template) that uses ephemeral storage
b) rename jenkins-ephemeral to jenkins
c) ???

any preference? (b) seems like the right answer but it's inconsistent with how we name all the other ephemeral vs persistent templates.

bparees · 2016-08-08T22:35:19Z

pkg/cmd/server/start/admission.go

@@ -41,6 +41,7 @@ var (

 func init() {
 	defaultOffPlugins.Insert("AlwaysPullImages")
+	defaultOffPlugins.Insert("JenkinsBootstrapper")


@deads2k i've disabled it via your preferred mechanism, but given that this mechanism exists, how do you feel about removing this field:
https://github.com/openshift/origin/blob/master/pkg/cmd/server/api/types.go#L325-L328

because it seems redundant to me.

I don't want to default this off. I want enabled to default to false.

you want what enabled setting to default to false?

@deads2k i've disabled it via your preferred mechanism, but given that this mechanism exists, how do you feel about removing this field:

Yes, lets remove that flag.

I don't want to default this off. I want enabled to default to false.

Let's say we default it on (that allows it be set to off). How would you distinguish between cases where ansible set the value to false and people were just waiting for it to be ready and the case where someone said "don't turn this on"? That would eliminate the ability for us to enable the feature when its ready.

Defaulting to off now, does not preclude defaulting to on later.

Allow jenkins auto-creation to be disabled from a single spot.

item 2 is still possible using the admission on/off mechanism

Default the entire config so an admin can see what is used

This is a general problem with admission plugins, why should jenkins be special in this case? The whole stanza has been reduced to an admission plugin (I think).

Why don't you want to remove the enabled flag? what's the difference between enabling this feature via the admission controller's config, and enabling it by enabling the admission controller itself?

Isn't enabling/disabling the controller what @deads2k's suggestion (which i've accepted) allows for?

moving @smarterclayton's comment to the right thread:

I don't consider the admission on off interface acceptable. The enabled flag was specifically put in to give all cluster admins a single, explicable point of control for jenkins auto creation. Admission fiddling is not usable, discoverable, or safe.

so are we going to remove the "disable random admission controllers" feature? because it's pretty confusing if we're going to have it and then not let people use it.

also i consider "disable this admission controller" more usable than "hey you want to disable this thing, well first you have to create a configuration for it, just to turn it off, because it's on by default if you don't configure it"

so are we going to remove the "disable random admission controllers" feature? because it's pretty confusing if we're going to have it and then not let people use it.

We can't remove that feature. It's the only sane way to remove a plugin from admission. The alternative (user takes ownership of the entire chain) is incredibly dangerous for a customer to do and is terrifying during upgrade (see the bugs I've gotten about clusterquota not enforcing because they took ownership of the admission chain).

smarterclayton · 2016-08-09T00:37:48Z

We should change the default config to point to jenkins-ephemeral

smarterclayton · 2016-08-09T00:37:55Z

AT the same time we disable the enabled flag.

bparees · 2016-08-09T04:56:34Z

We should change the default config to point to jenkins-ephemeral

you want the default config to expect to find a template named "jenkins-ephemeral" in the openshift namespace? I don't like that approach because it means an admin who wants to switch to a persistent template must stop their cluster and change the cluster config to point to different template name, or they must replace the "jenkins-ephemeral" template with one that's still named "jenkins-ephemeral" but is not actually ephemeral. Better to use a generic name so the admin can swap out the template w/o having to reconfigure the admission controller.

AT the same time we disable the enabled flag.

define "disable" and define which flag you're talking about. I still think we should be removing the "enabled" field that is part of the jenkins admission controller config because it's redundant to simply disabling the entire admission controller via the mechanism this PR is currently using.

maybe we need to chat in person so i can understand what you're proposing.

deads2k · 2016-08-09T12:10:26Z

We need to register (or tell people to register) that template in the openshift namespace

I'd rather not do this. I'd like to have an empty config be handled the same way we handle ProjectRequestTemplate today. The default configuration leaves the value empty. When it is empty, a coded default is used (template baked into code). That template is allowed to change between releases.

When it is specified, it is a namespace/name for the template to use. We also provide a command to show the current default so people can see what we'd do.

stevekuznetsov · 2016-08-09T13:00:40Z

hack/gen-bootstrap-bindata.sh

+    ${EXAMPLES}/db-templates/... \
+    ${EXAMPLES}/jenkins \
+    ${EXAMPLES}/jenkins/pipeline \
+    ${EXAMPLES}/quickstarts/...


While we're touching it we might as well make it pretty :)

output="${OUTPUT_PARENT}/pkg/bootstrap/bindata.go" "${GOPATH}/bin/go-bindata" -nocompress \ -nometadata \ -prefix "bootstrap" \ -pkg "bootstrap" \ -o "${output}" \ -ignore "README.md" \ -ignore application-template.json \ "${EXAMPLES}/image-streams/..." \ "${EXAMPLES}/db-templates/..." \ "${EXAMPLES}/jenkins" \ "${EXAMPLES}/jenkins/pipeline" \ "${EXAMPLES}/quickstarts/..."

Yeah, that makes all the difference when dealing in bash......

lol. and @stevekuznetsov sure, I thought I was already improving it, but i can go the rest of the way, just for you.

bparees · 2016-08-09T13:27:09Z

I'd rather not do this. I'd like to have an empty config be handled the same way we handle ProjectRequestTemplate today. The default configuration leaves the value empty. When it is empty, a coded default is used (template baked into code). That template is allowed to change between releases.

Although I see advantages to that approach, I don't think I want to bake a template into code. Right now admins/users can use "oc get template jenkins" to see the template we're instantiating on their behalf. Baking the default template into code makes that much harder (as you say, we would have to add a special command just for this capability that people would need to learn).

Also it should probably be an admin choice whether to change the template that's being instantiated when you move to a new release, otherwise you get projects with old and new templates and the admin may not even be aware that has happened (ie that they should go back and recreate jenkins in older projects because we patched something).

It also means we can't patch clusters in the field w/o either

sending them a new binary or
forcing them to install an override template and reconfigure their cluster to use it (which i assume requires a restart of the master to pick up the new config)

deads2k · 2016-08-09T13:32:13Z

Although I see advantages to that approach, I don't think I want to bake a template into code. Right now admins/users can use "oc get template jenkins" to see the template we're instantiating on their behalf. Baking the default template into code makes that much harder (as you say, we would have to add a special command just for this capability that people would need to learn).

I don't think that most people would even go looking for it. You'd get a "works out of the box" behavior (which doesn't happen today) and you'd still have all the power to being able to create your own template if you care. People who care wouldn't bother looking at what's created automatically, they'd grab the recommended template for their situation as they have to today.

The only big difference is that people who don't care wouldn't get greeted with failures they can't fix as an end user.

bparees · 2016-08-09T13:38:37Z

You'd get a "works out of the box" behavior (which doesn't happen today)

you get works out of the box behavior today for any "real" install that will have installed the expected templates (including oc cluster up). Backdooring resources into the system seems wrong to me.

The only big difference is that people who don't care wouldn't get greeted with failures they can't fix as an end user.

to you that's the only big difference. to me the big difference is all the things i listed. new tooling, non-standard approach, less discoverable, harder to perform maintenance/patching on.

csrwng · 2016-08-10T08:16:05Z

examples/jenkins/jenkins-ephemeral-template.json

      "iconClass": "icon-jenkins",
      "tags": "instant-app,jenkins"
    }
  },
-  "message": "A Jenkins service has been created in your project.  The username/password are admin/${JENKINS_PASSWORD}.",
+  "message": "A Jenkins service has been created in your project.  The username/password are admin/${JENKINS_PASSWORD}.  The tutorial at https://github.com/openshift/origin/blob/master/examples/jenkins/README.md contains more information about using this template.",


should you make the link a parameter so you can (more easily) point to a different one for OSE?

that's going to be a weird parameter since you're not actually expecting a user to provide their own value for it. (and OSE would still need to carry a patch to the template to use a different default value for the parameter?)

if OSE wants to point elsewhere, they can carry a patch to the template.

smarterclayton · 2016-08-10T20:51:14Z

Allowing users to disable admission controllers is useful. But my point is
that there are levels of safety and ease of explanation.

Jenkins is a premier, user focused feature. I want to say "turn Jenkins
off". Today, someone could probably figure that out in 30 seconds from
that description. The moment they touch admission control, all bets are
off.

On Wed, Aug 10, 2016 at 4:22 PM, OpenShift Bot [email protected]
wrote:

Origin Action Required: Pull request cannot be automatically merged,
please rebase your branch from latest HEAD and push again

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10260 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p2yp660uPPQSpjm338JHFSI-weaCks5qejL8gaJpZM4Jer_v
.

bparees · 2016-08-10T21:48:08Z

Jenkins is a premier, user focused feature. I want to say "turn Jenkins off". Today, someone could probably figure that out in 30 seconds from that description. The moment they touch admission control, all bets are off.

both things are part of the same kubeconfig file, no? i don't see how one is any easier than the other to figure out.

smarterclayton · 2016-08-11T00:23:18Z

jenkins.enabled = false is about as clear a config flag as I can possibly imagine.

deads2k · 2016-08-12T11:35:59Z

we can change the meaning of "unspecified" on the enablement field to mean "enabled" instead of "disabled" (is that a problematic API change?).

I think that's a reasonable change to make if they haven't expressed a preference.

changes the default template name that is instantiated to "jenkins-ephemeral", admins will have to provide a jenkins admission controller config if they want to use a different template, eg a persistent one (or register their other template under the name "jenkins-ephemeral")

I still think we should follow the project-template pattern to work out of the box and provide flexibility. But I'm also tired of arguing.

smarterclayton · 2016-08-12T13:41:43Z

Here are the things that I want - I don't have time to read the PR right
now.

oc cluster up and normal openshift start master enable jenkins by
default (with ephemeral)
If you guys want to rename "enabled" to "autoProvisionEnabled" I'm ok
with that - we have to do that before 1.3.0 or never change it.
ansible needs to turn jenkins off.

If this PR does those three things (or just 1 and 3) I'm ok with that.

On Fri, Aug 12, 2016 at 7:36 AM, David Eads [email protected]
wrote:

we can change the meaning of "unspecified" on the enablement field to
mean "enabled" instead of "disabled" (is that a problematic API change?).

I think that's a reasonable change to make if they haven't expressed a
preference.

changes the default template name that is instantiated to
"jenkins-ephemeral", admins will have to provide a jenkins admission
controller config if they want to use a different template, eg a persistent
one (or register their other template under the name "jenkins-ephemeral")

I still think we should follow the project-template pattern to work out of
the box and provide flexibility. But I'm also tired of arguing.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10260 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p_GjQ6FiCs-wMfOWpcRw9VungU-tks5qfFqhgaJpZM4Jer_v
.

deads2k · 2016-08-12T14:29:50Z

normal openshift start master enable jenkins by
default (with ephemeral)

@bparees can you think of way to do this that doesn't involve manual configuration or using the existing project-template pattern?

bparees · 2016-08-12T14:43:26Z

@smarterclayton

oc cluster up and normal openshift start master enable jenkins by default (with ephemeral)

this PR does not currently do that because @danmcp and I discussed and both think the default behavior should be consistent across origin/OCP/online. We probably need an in person discussion to get to a conclusion on that.

If you guys want to rename "enabled" to "autoProvisionEnabled" I'm ok with that - we have to do that before 1.3.0 or never change it.

done.

ansible needs to turn jenkins off.

this PR doesn't do that itself, of course, but the mechanism is in place such that ansible can configure it off or on. But that is not currently needed pending the resolution of (1).

bparees · 2016-08-12T14:48:36Z

@bparees can you think of way to do this that doesn't involve manual configuration or using the existing project-template pattern?

not sure what you mean. Today (since the default is that this behavior is off) there are two things you must configure to enable the feature:

configure the admission controller by setting AutoProvisionEnabled to true
register the jenkins-ephemeral template in the openshift namespace (which isn't even necessary if you use oc cluster up, or ansible install to create your cluster)

(1) is pending the discussion with @danmcp and @smarterclayton
(2) doesn't strike my as overly burdensome given that we already expect you to register all the other "out of the box" templates to do anything interesting with your cluster.

deads2k · 2016-08-12T16:18:22Z

(2) doesn't strike my as overly burdensome given that we already expect you to register all the other "out of the box" templates to do anything interesting with your cluster.

That's the classic: "it's installed and enabled, but it won't work unless you complete a series of configuration steps". Technically true, technically enabled about the of box, guaranteed to fail 100% of the time without additional non-out-of-the-box steps.

bparees · 2016-08-12T16:29:24Z

That's the classic: "it's installed and enabled, but it won't work unless you complete a series of configuration steps". Technically true, technically enabled about the of box, guaranteed to fail 100% of the time without additional non-out-of-the-box steps.

it's part of the install for oc cluster up and OCP. hardcoding it for origin does not make sense to me. (hardcoding resource definitions period strikes me as horrible)

deads2k · 2016-08-12T16:42:25Z

it's part of the install for oc cluster up and OCP. hardcoding it for origin does not make sense to me. (hardcoding resource definitions period strikes me as horrible)

Just because there's a default doesn't mean people have to use it. They can choose to do something else. Things that do this today to make them reasonable to use:

new project templates
oadm registry
oadm router

Those could all take the approach: "nothing will work by default because we don't want to code in a default", but instead they took the approach of "I'll code up a default. You can do someone different if you want, but this gets you started easily".

bparees · 2016-08-12T16:59:56Z

Those could all take the approach: "nothing will work by default because we don't want to code in a default", but instead they took the approach of "I'll code up a default. You can do someone different if you want, but this gets you started easily".

and i'd argue that was wrong for those too.

we have an open, flexible, resource definition based system and then we've decided instead of making that easily consumable for a new installation, we'll just hardcode stuff into an opaque static binary and sideload it via a separate mechanism instead because that was convenient for us. What does the default registry template look like for a cluster? who knows! go instantiate one and find out or go look at the go code.

And for a practicality argument: I am not maintaining this template in 2 places (one hardcoded and another as a "sample" for people who want to have a starting point for customization). And then when people have issues, have to guess at what version of the hardcoded template they have because they can't easily inspect it. And tell them that because we shipped broken code, they now have to override the hardcoded template by going down a path they weren't going down previously to register a new template, instead of just updating an existing template.

deads2k · 2016-08-12T17:07:40Z

And for a practicality argument: I am not maintaining this template in 2 places (one hardcoded and another as a "sample" for people who want to have a starting point for customization). And then when people have issues, have to guess at what version of the hardcoded template they have because they can't easily inspect it. And tell them that because we shipped broken code, they now have to override the hardcoded template by going down a path they weren't going down previously to register a new template, instead of just updating an existing template.

I'll note that we've never ever actually had that problem in the places we've done this and the solution to double maintenance is as simple as just serializing out your template.

bparees · 2016-08-12T17:53:10Z

I'll note that we've never ever actually had that problem in the places we've done this and the solution to double maintenance is as simple as just serializing out your template.

would you at least agree that hardcoding the template definition into the binary would not be your preferred solution if we had a better install experience that could handle auto-registering templates that were part of the product?

deads2k · 2016-08-12T18:10:29Z

would you at least agree that hardcoding the template definition into the binary would not be your preferred solution if we had a better install experience that could handle auto-registering templates that were part of the product?

I'm honestly unsure. It's a thing I hated so much at IBM, nothing worked out of the box and you had to use some blessed installer or spend forever wiring it up by hand.

I think that I'd come down on the side of, binary should just work (hard coded defaults if necessary) and I'd allow an installer to wire up and auto-register templates if it wanted to. I think that would resolve both the "I want it to work case" and the "I want ansible installs to always wire up a template" cases.

bparees · 2016-08-18T20:56:40Z

@danmcp @smarterclayton ok i'm about as happy w/ the state of this PR as i'm likely to get:

default behavior for any cluster is that autoprovision is disabled. can be enabled by configuring the admission controller
default behavior for the admission controller (when enabled) is to look for a template named "jenkins-ephemeral" in the openshift namespace.

Docs updates pending.

Barring a last minute veto, i'll be merging this tonight.

Things people won't be happy with:

@smarterclayton because it disable autoprovision everywhere
@deads2k because autoprovisioning default behavior (when enabled) still relies on an external template that must be registered, not a hardcoded template
@bparees and @deads2k because we have 2 ways to disable the admission controller behaivor (the generic "Disable admission controller X" mechanism and the specific "hey jenkins bootstrapper, your config says you are disabled" mechanism)
@bparees because the default template name used by the admission controller is "jenkins-ephemeral", meaning for admins who really want to use this, they must(after enabling the controller) not only register a proper persistent template, but also update the admission controller config to point to that template (which admittedly they can do when they are enabling the controller, so not a huge deal..but will be a bigger deal when it's enabled by default so you wouldn't normally need to touch the config.)

So i think everyone has given up something here. :)

openshift-bot · 2016-08-19T08:47:29Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8267/) (Image: devenv-rhel7_4882)

deads2k · 2016-08-19T12:42:56Z

Things people won't be happy with:

I didn't see @danmcp in there, I'll be he's happy with everything!

danmcp · 2016-08-19T13:10:19Z

@deads2k Ignorance is bliss.

bparees · 2016-08-19T17:48:53Z

[merge]

openshift-bot · 2016-08-19T18:47:16Z

Evaluated for origin merge up to 67b51bf

bparees · 2016-08-19T19:19:13Z

#10080
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8262/
[test]

openshift-bot · 2016-08-19T19:22:21Z

Evaluated for origin test up to 67b51bf

openshift-bot · 2016-08-19T20:47:58Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8270/)

bparees force-pushed the disable_autojenkins branch 2 times, most recently from c9d31a3 to e410633 Compare August 8, 2016 04:41

bparees force-pushed the disable_autojenkins branch from e410633 to a2ab01f Compare August 8, 2016 13:13

bparees mentioned this pull request Aug 8, 2016

Jenkins ephemeral template (installed to dedicated hack machine) doesn't have edit permission #10257

Closed

deads2k reviewed Aug 8, 2016
View reviewed changes

bparees force-pushed the disable_autojenkins branch from a2ab01f to 8464a8d Compare August 8, 2016 20:58

bparees force-pushed the disable_autojenkins branch from 8464a8d to 66fe1bf Compare August 8, 2016 22:34

bparees reviewed Aug 8, 2016
View reviewed changes

stevekuznetsov reviewed Aug 9, 2016
View reviewed changes

bparees force-pushed the disable_autojenkins branch from 66fe1bf to edcd36a Compare August 9, 2016 14:25

csrwng reviewed Aug 10, 2016
View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2016

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 12, 2016

bparees force-pushed the disable_autojenkins branch from 69a199c to 70c66e8 Compare August 12, 2016 13:54

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 16, 2016

bparees force-pushed the disable_autojenkins branch from 70c66e8 to 3c98c43 Compare August 18, 2016 20:50

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2016

disable jenkins auto-deployment

67b51bf

bparees force-pushed the disable_autojenkins branch from 3c98c43 to 67b51bf Compare August 18, 2016 23:24

openshift-bot merged commit 1f54bd6 into openshift:master Aug 19, 2016

bparees deleted the disable_autojenkins branch August 22, 2016 13:19

disable jenkins auto-deployment #10260

disable jenkins auto-deployment #10260

Conversation

bparees commented Aug 8, 2016 • edited Loading

bparees commented Aug 8, 2016

bparees commented Aug 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Aug 8, 2016

bparees commented Aug 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Aug 9, 2016

smarterclayton commented Aug 9, 2016

bparees commented Aug 9, 2016

deads2k commented Aug 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Aug 9, 2016

deads2k commented Aug 9, 2016

bparees commented Aug 9, 2016

csrwng Aug 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Aug 10, 2016

bparees commented Aug 10, 2016

smarterclayton commented Aug 11, 2016 via email

deads2k commented Aug 12, 2016

smarterclayton commented Aug 12, 2016

deads2k commented Aug 12, 2016

bparees commented Aug 12, 2016

bparees commented Aug 12, 2016

deads2k commented Aug 12, 2016

bparees commented Aug 12, 2016

deads2k commented Aug 12, 2016

bparees commented Aug 12, 2016

deads2k commented Aug 12, 2016

bparees commented Aug 12, 2016

deads2k commented Aug 12, 2016

bparees commented Aug 18, 2016

openshift-bot commented Aug 19, 2016 • edited Loading

deads2k commented Aug 19, 2016

danmcp commented Aug 19, 2016

bparees commented Aug 19, 2016

openshift-bot commented Aug 19, 2016

bparees commented Aug 19, 2016

openshift-bot commented Aug 19, 2016

openshift-bot commented Aug 19, 2016

bparees commented Aug 8, 2016 •

edited

Loading

csrwng Aug 10, 2016 •

edited

Loading

openshift-bot commented Aug 19, 2016 •

edited

Loading