Add 'oc set probe' for setting readiness and liveness #7331

smarterclayton · 2016-02-16T04:58:11Z

Adds a new helper which should be used for getting patches for
changes (can be abstracted further).

deads2k · 2016-02-16T21:08:39Z

pkg/cmd/cli/cmd/set/probe.go

+ Mapper meta.RESTMapper
+
+ PrintObject func(runtime.Object) error
+ UpdatePodSpecForObject func(runtime.Object, func(spec *kapi.PodSpec) error) (bool, error)


please start typing these or this will become another Factory.

For a single use field the type doesn't make anything better less
understandable (I have to go find the method, add Godoc to it, etc). Also,
Factory isn't the wrong pattern - this is the simplest way to express a set
of common behaviors that multiple commands use that can be abstracted.
Passing the factory onto Options is definitely the wrong pattern.

On Tue, Feb 16, 2016 at 4:08 PM, David Eads notifications@github.com
wrote:

In pkg/cmd/cli/cmd/set/probe.go
#7331 (comment):

Filenames []string

ContainerSelector string

Selector string

All bool

Builder *resource.Builder

Infos []*resource.Info

Encoder runtime.Encoder

ShortOutput bool

Mapper meta.RESTMapper

PrintObject func(runtime.Object) error

UpdatePodSpecForObject func(runtime.Object, func(spec *kapi.PodSpec) error) (bool, error)

please start typing these or this will become another Factory.

—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/7331/files#r53078585.

Factory with a bunch of specialized function types that themselves end up being overridden by other things that just happen to know they want exactly the same shape of function without an named type makes rebasing and keeping the bits in sync very difficult. It has also make it nearly impossible to split the Factory apart into sub-chunks that have bits that we want.

Splitting the factory isn't really worth it - seventeen different
interfaces being passed in 5 different shapes of interface holder objects
to a hundred commands is madness. Going from function pointers to
interfaces is a worthy goal, it just isn't on my list of "things that are
causing users or programmers problems"

On Wed, Feb 17, 2016 at 9:36 AM, David Eads notifications@github.com
wrote:

In pkg/cmd/cli/cmd/set/probe.go
#7331 (comment):

Filenames []string

ContainerSelector string

Selector string

All bool

Builder *resource.Builder

Infos []*resource.Info

Encoder runtime.Encoder

ShortOutput bool

Mapper meta.RESTMapper

PrintObject func(runtime.Object) error

UpdatePodSpecForObject func(runtime.Object, func(spec *kapi.PodSpec) error) (bool, error)

Factory with a bunch of specialized function types that themselves end up
being overridden by other things that just happen to know they want exactly
the same shape of function without an named type makes rebasing and keeping
the bits in sync very difficult. It has also make it nearly impossible to
split the Factory apart into sub-chunks that have bits that we want.

—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/7331/files#r53171584.

This is now using pointers instead of flag set.

On Wed, Feb 17, 2016 at 9:55 AM, Clayton Coleman ccoleman@redhat.com
wrote:

Splitting the factory isn't really worth it - seventeen different
interfaces being passed in 5 different shapes of interface holder objects
to a hundred commands is madness. Going from function pointers to
interfaces is a worthy goal, it just isn't on my list of "things that are
causing users or programmers problems"

On Wed, Feb 17, 2016 at 9:36 AM, David Eads notifications@github.com
wrote:

In pkg/cmd/cli/cmd/set/probe.go
#7331 (comment):

Filenames []string

ContainerSelector string

Selector string

All bool

Builder *resource.Builder

Infos []*resource.Info

Encoder runtime.Encoder

ShortOutput bool

Mapper meta.RESTMapper

PrintObject func(runtime.Object) error

UpdatePodSpecForObject func(runtime.Object, func(spec *kapi.PodSpec) error) (bool, error)

Factory with a bunch of specialized function types that themselves end up
being overridden by other things that just happen to know they want exactly
the same shape of function without an named type makes rebasing and keeping
the bits in sync very difficult. It has also make it nearly impossible to
split the Factory apart into sub-chunks that have bits that we want.

—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/7331/files#r53171584.

it just isn't on my list of "things that are
causing users or programmers problems"

Wait, I'm a programmer and it causes me problems on a rebase! Take the last one as a for instance. I spent hours chasing down the duplicate definitions of one logical type for a clientmapper function.

Sure, the interface stuff would help that (not opposed to that change). Am
opposed to splitting up factory or anything other than "Factory is a struct
{ interface1 interface2 interface3 }" which I still think is vulnerable to
the problem you describe.

On Wed, Feb 17, 2016 at 10:37 AM, David Eads notifications@github.com
wrote:

In pkg/cmd/cli/cmd/set/probe.go
#7331 (comment):

Filenames []string

ContainerSelector string

Selector string

All bool

Builder *resource.Builder

Infos []*resource.Info

Encoder runtime.Encoder

ShortOutput bool

Mapper meta.RESTMapper

PrintObject func(runtime.Object) error

UpdatePodSpecForObject func(runtime.Object, func(spec *kapi.PodSpec) error) (bool, error)

it just isn't on my list of "things that are
causing users or programmers problems"

Wait, I'm a programmer and it causes me problems on a rebase! Take the
last one as a for instance. I spent hours chasing down the duplicate
definitions of one logical type for a clientmapper function.

—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/7331/files#r53181031.

smarterclayton · 2016-02-16T23:13:56Z

UI depends on kubernetes/kubernetes#21341

fabianofranz · 2016-02-18T17:50:16Z

pkg/cmd/cli/cmd/set/probe.go

+ ContainerSelector: "*",
+ }
+ cmd := &cobra.Command{
+ Use: "probe RESOURCE/NAME [--readiness|--liveness] ...",


At least one of readiness or liveness are required, so it doesn't take brackets here: --readiness|--liveness.

I'd suggest this usage line:

probe (RESOURCE/NAME | RESOURCE --all) --readiness|--liveness [--get-url=URL] [--open-tcp=PORT] [-- COMMAND]

That's probably too long to fit on a normal screen, but I'll check it

Probably. Resource arg can be just RESOURCE/NAME, we do that already in other commands even when they accept --all. If the rest fits it would be good, makes it pretty clear what options you have.

smarterclayton · 2016-02-18T23:29:57Z

[test]

smarterclayton · 2016-02-20T18:39:48Z

https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/11465/console

FAILURE after 0.172s: hack/../test/cmd/basicresources.sh:195: executing 'oc new-project --token=eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJrdWJlcm5ldGVzL3NlcnZpY2VhY2NvdW50Iiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9uYW1lc3BhY2UiOiJjbWQtYmFzaWNyZXNvdXJjZXMiLCJrdWJlcm5ldGVzLmlvL3NlcnZpY2VhY2NvdW50L3NlY3JldC5uYW1lIjoiYnVpbGRlci10b2tlbi03bHFvOSIsImt1YmVybmV0ZXMuaW8vc2VydmljZWFjY291bnQvc2VydmljZS1hY2NvdW50Lm5hbWUiOiJidWlsZGVyIiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9zZXJ2aWNlLWFjY291bnQudWlkIjoiMjkzNmRjMDMtZDc5ZC0xMWU1LWJhZTItMGU1ZjM5ZTE0YWVmIiwic3ViIjoic3lzdGVtOnNlcnZpY2VhY2NvdW50OmNtZC1iYXNpY3Jlc291cmNlczpidWlsZGVyIn0.RCkUFAbGOjo5XeHFImi8JalIK93-dGHRIxQoHiXQ7AHg70bN6uAxuxX9Domp990JKAWsxnq6U9rJqWqJ7FcheIJx5ZniWNP-UgiIPliF9lBDxIJUk5ofB0y-snxCp3FgD9efzlBzsUwI72AYff3juLR5yY5UYUAKXDP9nEVulWXp4J26rYY-FTQAAjTPaEcDendU2ZUNxxiuprRiV6so_ZPFMZTJLcVe792LpsBc5jmm3yBSaIChyxzTNLKIDA-IaoBdNWAKrID9LQg7O5JTK9l3vMOuWiiXdFzr_YngaWgkdXWO53UtTTTF7ystxEJoD1qLP3ifq6KVikn4tBMTCAeyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJrdWJlcm5ldGVzL3NlcnZpY2VhY2NvdW50Iiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9uYW1lc3BhY2UiOiJjbWQtYmFzaWNyZXNvdXJjZXMiLCJrdWJlcm5ldGVzLmlvL3NlcnZpY2VhY2NvdW50L3NlY3JldC5uYW1lIjoiYnVpbGRlci10b2tlbi11c3dwZiIsImt1YmVybmV0ZXMuaW8vc2VydmljZWFjY291bnQvc2VydmljZS1hY2NvdW50Lm5hbWUiOiJidWlsZGVyIiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9zZXJ2aWNlLWFjY291bnQudWlkIjoiMjkzNmRjMDMtZDc5ZC0xMWU1LWJhZTItMGU1ZjM5ZTE0YWVmIiwic3ViIjoic3lzdGVtOnNlcnZpY2VhY2NvdW50OmNtZC1iYXNpY3Jlc291cmNlczpidWlsZGVyIn0.fPhtgcnwnhQfL7gwYv_UGvZs5s4I3m0lqJ0I9AAq6dv_eWzbNZYDVfRubQNyo4TunMKZOjNfxOyAI6rOKa_l-SXQvRn3UNpp2OsLGPMlvEqcqu_IMsloy2A6d3292uzfd2kPZEGeSNyj8x1cJZNP6-WG578qFPuGRbfrh-RFnA1mpjLoadve9i2ickGC8sa3HqDD7DEvPrxw9D_TfrzZIPkPktqtVC6yrX5yBh_WIH1Zejxf5z8a-z3c7znTqRTsk4HuMcSQwpbfjcqBu7zSbVqrhZCAb3y-BT-BBFjvNP3glalGst86n8HPxeeaIwC3MoC8oKN5XUX3VXrEAHVReAeyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJrdWJlcm5ldGVzL3NlcnZpY2VhY2NvdW50Iiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9uYW1lc3BhY2UiOiJjbWQtYmFzaWNyZXNvdXJjZXMiLCJrdWJlcm5ldGVzLmlvL3NlcnZpY2VhY2NvdW50L3NlY3JldC5uYW1lIjoiYnVpbGRlci10b2tlbi0xanV5byIsImt1YmVybmV0ZXMuaW8vc2VydmljZWFjY291bnQvc2VydmljZS1hY2NvdW50Lm5hbWUiOiJidWlsZGVyIiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9zZXJ2aWNlLWFjY291bnQudWlkIjoiMjkzNmRjMDMtZDc5ZC0xMWU1LWJhZTItMGU1ZjM5ZTE0YWVmIiwic3ViIjoic3lzdGVtOnNlcnZpY2VhY2NvdW50OmNtZC1iYXNpY3Jlc291cmNlczpidWlsZGVyIn0.pOFxtYpsUhUUbbImVtgO5HDuI29w7Da7QWM0xrydRQvm8u4UyrHVNRRXjsnStVfFIydoC6KMq2HbAyHSyzVYIrDwz8E7q7qMKaED67zJesWfXrRwgW4v1kvcZyywzU9ufa3q0StVoSI1o-loy7VkzmxX4i9SexWymqK8KspywlqKp07p7K3JuIyF5vw2TV9U5TlBmn7tf5_qmiTOHwusTS6efJeaM-YeCIkj0-Y1buA3KdSObsvr-FKkHBliFr_xKpH_9sUUAOwXJ2RJBXXRWOXL45DZNARRkO-lgX_dD0zEOUptxEjzJZSrU770djqEdEri2QATF651xYDF4wfqKA will-fail' expecting failure and text 'Error from server: You may not request a new project via this API': the output content test failed
There was no output from the command.
Standard error from the command:
Error from server: the server has asked for the client to provide credentials
!!! Error in hack/../test/cmd/../../hack/cmd_util.sh:195
    'return 1' exited with status 1
Call stack:
    1: hack/../test/cmd/../../hack/cmd_util.sh:195 os::cmd::expect_failure_and_text(...)
    2: hack/../test/cmd/basicresources.sh:195 main(...)
Exiting with status 1

smarterclayton · 2016-02-20T18:40:01Z

[test]

smarterclayton · 2016-02-20T18:40:43Z

This is ready for final review @fabianofranz or @Kargakis

fabianofranz · 2016-02-22T00:59:08Z

pkg/cmd/cli/cmd/set/probe.go

+ cmd.Flags().BoolVar(&options.All, "all", options.All, "Select all resources in the namespace of the specified resource types")
+ cmd.Flags().StringSliceVarP(&options.Filenames, "filename", "f", options.Filenames, "Filename, directory, or URL to file to use to edit the resource.")
+
+ cmd.Flags().BoolVar(&options.Remove, "remove", options.Remove, "If true, remove the specified probe(s).")


Add --remove-all like in triggers?

There are only two, so it didn't seem like we needed it. Triggers are a
list, this is more of a named struct.

On Sun, Feb 21, 2016 at 7:59 PM, Fabiano Franz notifications@github.com
wrote:

In pkg/cmd/cli/cmd/set/probe.go
#7331 (comment):

// TODO: move met to kcmdutil

if err == cmdutil.ErrExit {

os.Exit(1)

}

kcmdutil.CheckErr(err)

}

},

}

kcmdutil.AddPrinterFlags(cmd)

cmd.Flags().StringVarP(&options.ContainerSelector, "containers", "c", options.ContainerSelector, "The names of containers in the selected pod templates to change - may use wildcards")

cmd.Flags().StringVarP(&options.Selector, "selector", "l", options.Selector, "Selector (label query) to filter on")

cmd.Flags().BoolVar(&options.All, "all", options.All, "Select all resources in the namespace of the specified resource types")

cmd.Flags().StringSliceVarP(&options.Filenames, "filename", "f", options.Filenames, "Filename, directory, or URL to file to use to edit the resource.")

cmd.Flags().BoolVar(&options.Remove, "remove", options.Remove, "If true, remove the specified probe(s).")

Add --remove-all like in triggers?

—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/7331/files#r53578238.

fabianofranz · 2016-02-22T01:06:12Z

Minor comments but LGTM.

0xmichalis · 2016-02-22T10:25:10Z

pkg/cmd/cli/cmd/set/probe.go

+ $ %[1]s probe dc/router --readiness --get-url=https://127.0.0.1:1936/stats
+
+ # Set only the initial-delay-seconds field on all deployments
+ $ %[1]s probe dc --all --readiness --initial-delay-seconds=30`


FWIW, most commands don't use --all and have the same effect.

For mutation I would prefer we be explicit

I would like to make requiring --all for any mutation command, and make that part of kubectl conventions. kubectl delete pods lacks specificity as to user intent, and for the other commands we are "safe" when someone does the simple thing, so we should be safe on mutation.

will this support 30s or 1m ? it always has to be seconds, right?

It has to be seconds because that's what the API supports. We could
support integer durations in the future, but practically this will always
be a number between 0 and 240

On Tue, Feb 23, 2016 at 8:05 AM, Michal Fojtik notifications@github.com
wrote:

In pkg/cmd/cli/cmd/set/probe.go
#7331 (comment):

$ %[1]s probe dc/registry --remove --readiness --liveness

Set an exec action as a liveness probe to run 'echo ok'

$ %[1]s probe dc/registry --liveness -- echo ok

Set a readiness probe to try to open a TCP socket on 3306

$ %[1]s probe rc/mysql --readiness --open-tcp=3306

Set an HTTP readiness probe for port 8080 and path /healthz over HTTP on the pod IP

$ %[1]s probe dc/webapp --readiness --get-url=http://:8080/healthz

Set an HTTP readiness probe over HTTPS on 127.0.0.1 for a hostNetwork pod

$ %[1]s probe dc/router --readiness --get-url=https://127.0.0.1:1936/stats

Set only the initial-delay-seconds field on all deployments

$ %[1]s probe dc --all --readiness --initial-delay-seconds=30`

will this support 30s or 1m ? it always has to be seconds, right?

—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/7331/files#r53776601.

smarterclayton · 2016-02-22T13:34:34Z

Updated, PTAL

smarterclayton · 2016-02-23T01:31:45Z

[test]

smarterclayton · 2016-02-23T12:59:26Z

[test]

On Tue, Feb 23, 2016 at 7:30 AM, OpenShift Bot notifications@github.com
wrote:

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

—
Reply to this email directly or view it on GitHub
#7331 (comment).

smarterclayton · 2016-02-23T15:15:06Z

Any other comments? Will merge at 11am EST otherwise

0xmichalis · 2016-02-23T15:30:01Z

LGTM

Adds a new helper which should be used for getting patches for changes (can be abstracted further).

smarterclayton · 2016-02-24T01:31:59Z

[merge]

openshift-bot · 2016-02-24T01:35:22Z

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

openshift-bot · 2016-02-24T01:35:23Z

Evaluated for origin merge up to b2de291

openshift-bot · 2016-02-24T01:35:23Z

Evaluated for origin test up to b2de291

openshift-bot · 2016-02-24T02:56:33Z

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

Merged by openshift-bot

smarterclayton mentioned this pull request Feb 16, 2016

Run user-provided command as part of build flow #6715

Merged

5 tasks

deads2k reviewed Feb 16, 2016
View reviewed changes

smarterclayton force-pushed the implement_set_probe branch from 479a7fc to 460110d Compare February 17, 2016 03:54

fabianofranz reviewed Feb 18, 2016
View reviewed changes

smarterclayton force-pushed the implement_set_probe branch 2 times, most recently from e8ada65 to 5bfb4c4 Compare February 18, 2016 23:29

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

smarterclayton force-pushed the implement_set_probe branch 2 times, most recently from 67f4062 to 1353b8c Compare February 19, 2016 22:34

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

smarterclayton force-pushed the implement_set_probe branch 2 times, most recently from da22041 to 6e28ecf Compare February 20, 2016 06:12

smarterclayton mentioned this pull request Feb 21, 2016

Proposal: First-generation of kubectl set * commands kubernetes/kubernetes#21648

Closed

smarterclayton force-pushed the implement_set_probe branch 2 times, most recently from 9242572 to 0f30adc Compare February 21, 2016 23:02

fabianofranz reviewed Feb 22, 2016
View reviewed changes

smarterclayton force-pushed the implement_set_probe branch from 0f30adc to c38eaef Compare February 22, 2016 01:52

0xmichalis reviewed Feb 22, 2016
View reviewed changes

smarterclayton force-pushed the implement_set_probe branch from c38eaef to f6c79df Compare February 22, 2016 13:33

Add 'oc set probe' for setting readiness and liveness

b2de291

Adds a new helper which should be used for getting patches for changes (can be abstracted further).

smarterclayton force-pushed the implement_set_probe branch from f6c79df to b2de291 Compare February 24, 2016 01:31

openshift-bot pushed a commit that referenced this pull request Feb 24, 2016

Merge pull request #7331 from smarterclayton/implement_set_probe

c1f0006

Merged by openshift-bot

openshift-bot merged commit c1f0006 into openshift:master Feb 24, 2016

Add 'oc set probe' for setting readiness and liveness #7331

Add 'oc set probe' for setting readiness and liveness #7331

Conversation

smarterclayton commented Feb 16, 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

smarterclayton commented Feb 16, 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

smarterclayton commented Feb 18, 2016

smarterclayton commented Feb 20, 2016

smarterclayton commented Feb 20, 2016

smarterclayton commented Feb 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianofranz commented Feb 22, 2016

Choose a reason for hiding this comment

smarterclayton Feb 22, 2016 via email

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

Set an exec action as a liveness probe to run 'echo ok'

Set a readiness probe to try to open a TCP socket on 3306

Set an HTTP readiness probe for port 8080 and path /healthz over HTTP on the pod IP

Set an HTTP readiness probe over HTTPS on 127.0.0.1 for a hostNetwork pod

Set only the initial-delay-seconds field on all deployments

smarterclayton commented Feb 22, 2016

smarterclayton commented Feb 23, 2016

smarterclayton commented Feb 23, 2016

smarterclayton commented Feb 23, 2016

0xmichalis commented Feb 23, 2016

smarterclayton commented Feb 24, 2016

openshift-bot commented Feb 24, 2016

openshift-bot commented Feb 24, 2016

openshift-bot commented Feb 24, 2016

openshift-bot commented Feb 24, 2016