Add PodPreset support by pmorie · Pull Request #917 · kubernetes-retired/service-catalog

pmorie · 2017-06-06T15:29:08Z

Closes #266

Incorporates lessons learned in #760

arschles · 2017-06-07T18:30:48Z

@pmorie if this closes #266, can you say so in the OP?

duglin · 2017-06-07T18:42:54Z

+// Binding.
+type AlphaPodPresetTemplate struct {
+	// Name is the name of the PodPreset to create.
+	Name string


can we make this optional? And let it default to the name of the binding?

I'm fine doing that, but would rather do it in a follow-up (with label selector) if that's alright with you.

duglin · 2017-06-07T18:44:04Z

+
+// AlphaPodPresetTemplate represents how a PodPreset should be created for a
+// Binding.
+type AlphaPodPresetTemplate struct {


Is the word "template" necessary? Or are you trying to hammer home the idea that its not a true PodPreset its just a pseudo-one?

I chose the name because I expect that we'll converge on a type in the core called 'PodPresetTemplate', which will eventually be the name of the same field.

But yeah, driving home the idea that this isn't a full pod preset is also a goal

vaikas · 2017-06-07T18:31:29Z

+// AlphaPodPresetTemplate represents how a PodPreset should be created for a
+// Binding.
+type AlphaPodPresetTemplate struct {
+	// Name is the name of the PodPreset to create.


Name of the PodPreset to create.

Is this comment about the format of the godoc comment? This is the style we use elsewhere in godoc on fields (at least in types.go)

vaikas · 2017-06-07T18:34:10Z

+type AlphaPodPresetTemplate struct {
+	// Name is the name of the PodPreset to create.
+	Name string
+	// Selector is the LabelSelector of the PodPreset to create.


vaikas · 2017-06-07T18:44:48Z

+// Binding.
+type AlphaPodPresetTemplate struct {
+	// Name is the name of the PodPreset to create.
+	Name string


Any reason to not just use the name of the binding for this?

duglin · 2017-06-07T18:49:01Z

+	// Name is the name of the PodPreset to create.
+	Name string
+	// Selector is the LabelSelector of the PodPreset to create.
+	Selector metav1.LabelSelector


not 100% sure but I think you have this so its a required field, is that correct? If so, can we make it optional so that all new pods in this namespace will be injected?

It's a required field; I would be okay with a default, but would prefer to do the default in a follow-up if that's cool with you.

arschles · 2017-06-07T18:53:08Z

@pmorie I'm fine with this as-is, and to recap what we talked about on Slack, you're gonna file an issue to add a flag and admission controller to turn off pod preset functionality.

arschles · 2017-06-07T18:56:50Z

@pmorie CI failure looks legit:

--- FAIL: TestReconcileBindingDeleteWithPodPreset (0.00s)
	controller_test.go:1036: []
	controller_test.go:1006: goroutine 77 [running]:
		runtime/debug.Stack(0x189e63d, 0x8, 0x434501)
			/usr/local/go/src/runtime/debug/stack.go:24 +0x87
		github.com/kubernetes-incubator/service-catalog/pkg/controller.fatalf(0xc4201cc680, 0x1b88c3b, 0x33, 0xc42019ac30, 0x3, 0x3)
			/go/src/github.com/kubernetes-incubator/service-catalog/pkg/controller/controller_test.go:1006 +0x34
		github.com/kubernetes-incubator/service-catalog/pkg/controller.testNumberOfActions(0xc4201cc680, 0x0, 0x0, 0x1bc62d0, 0x25cecd8, 0x0, 0x0, 0x2, 0xc42009b578)
			/go/src/github.com/kubernetes-incubator/service-catalog/pkg/controller/controller_test.go:1037 +0x31d
		github.com/kubernetes-incubator/service-catalog/pkg/controller.assertNumberOfActions(0xc4201cc680, 0x25cecd8, 0x0, 0x0, 0x2)
			/go/src/github.com/kubernetes-incubator/service-catalog/pkg/controller/controller_test.go:1022 +0x7f
		github.com/kubernetes-incubator/service-catalog/pkg/controller.TestReconcileBindingDeleteWithPodPreset(0xc4201cc680)
			/go/src/github.com/kubernetes-incubator/service-catalog/pkg/controller/controller_binding_test.go:678 +0x10b8
		testing.tRunner(0xc4201cc680, 0x1bc6128)
			/usr/local/go/src/testing/testing.go:657 +0x108
		created by testing.(*T).Run
			/usr/local/go/src/testing/testing.go:697 +0x544
		
	controller_test.go:1007: Unexpected number of actions: expected 2, got 0
E0607 18:54:52.410666    1865 controller_broker.go:328] ServiceClass "test-serviceclass" already exists with OSB guid "notTheSame", received different guid "SCGUID"
E0607 18:54:52.420657    1865 controller_broker.go:322] ServiceClass "test-serviceclass" for Broker "test-broker" already exists for Broker "notTheSame"

pmorie · 2017-06-07T20:15:53Z

I may have pushed the wrong version of this code - hang tight.

pmorie · 2017-06-08T20:51:20Z

I've removed the flaky test - spent all day debugging this and I'm out of time for now. I'm going to resubmit a PR with those commits, but for now I would like to get this in, since it works in my manual tests and adds happy-path test coverage for bind, and in an immediate follow-up continue debugging.

pmorie · 2017-06-08T20:52:42Z

Actually, the test itself was not necessarily flaky - it consistently passed everywhere but in travis, where it consistently failed, which doesn't fit my normal definition of a flake.

pmorie · 2017-06-13T22:26:30Z

Created #922 to gate this feature

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 6, 2017

pmorie added this to the 0.0.10 milestone Jun 6, 2017

pmorie mentioned this pull request Jun 7, 2017

Determine whether controller should update secrets #921

Closed

pmorie force-pushed the pod-preset branch from fa87041 to 5a59685 Compare June 7, 2017 18:17

pmorie changed the title ~~WIP: Add PodPreset support~~ Add PodPreset support Jun 7, 2017

duglin reviewed Jun 7, 2017

View reviewed changes

vaikas reviewed Jun 7, 2017

View reviewed changes

duglin reviewed Jun 7, 2017

View reviewed changes

arschles added the LGTM1 label Jun 7, 2017

pmorie force-pushed the pod-preset branch 2 times, most recently from 4cf6a35 to 86263b3 Compare June 8, 2017 14:51

pmorie closed this Jun 8, 2017

pmorie reopened this Jun 8, 2017

pmorie force-pushed the pod-preset branch from 79ddddf to 40d7eb2 Compare June 8, 2017 20:22

pmorie added 2 commits June 8, 2017 16:42

Add support for PodPreset

662095c

Regen code

6472d5e

pmorie force-pushed the pod-preset branch from 40d7eb2 to 6472d5e Compare June 8, 2017 20:45

pmorie mentioned this pull request Jun 8, 2017

Instance never provisioned should not invoke broker #891

Merged

vaikas added the LGTM2 label Jun 9, 2017

jpeeler added the LGTM3 label Jun 9, 2017

jpeeler merged commit cbfa39b into kubernetes-retired:master Jun 9, 2017

nilebox mentioned this pull request Jun 23, 2017

Problem with env vars for creds #969

Closed

pmorie mentioned this pull request Jun 28, 2017

Add a PodPresetBinding resource #994

Closed

Conversation

pmorie commented Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arschles commented Jun 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arschles commented Jun 7, 2017

Uh oh!

arschles commented Jun 7, 2017

Uh oh!

pmorie commented Jun 7, 2017

Uh oh!

pmorie commented Jun 8, 2017

Uh oh!

pmorie commented Jun 8, 2017

Uh oh!

pmorie commented Jun 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pmorie commented Jun 6, 2017 •

edited

Loading