PodPreset initializer implementation #1754

jpeeler · 2018-02-21T22:19:30Z

No description provided.

jpeeler · 2018-02-21T22:20:32Z

This supercedes #1195 (but still WIP currently)

MHBauer · 2018-02-21T22:31:49Z

@jberkhahn this should interest you. It has the binding stuff.

pmorie · 2018-02-21T22:39:04Z

I'm not sure why github isn't complaining about this - it looks like it needs rebase.

jberkhahn

Reviewed with @MHBauer

jberkhahn · 2018-03-05T22:05:36Z

charts/catalog/templates/apiserver-deployment.yaml

@@ -64,6 +64,10 @@ spec:
        {{- if .Values.apiserver.serveOpenAPISpec }}
        - --serve-openapi-spec
        {{- end }}
+        {{- if .Values.apiserver.featureGates }}


Why does this feature-gate flag have no argument associated with it? @MHBauer thinks it should one like the OriginatingIdentity on line 61. Either we should have them all be separate, or they should all be jumbled together.

There doesn't seem to be any added gates in this PR. Why does this exist in this PR at all?

Good point, it's easier to specify a boolean rather than the feature gate parameter.

jberkhahn · 2018-03-05T22:17:38Z

pkg/controller/podpreset/admission.go

@@ -0,0 +1,355 @@
+/*


Wouldn't it make more sense for this file to be in plugins/pkg/admission with the other admission controllers?

I don't think so. The other admission controllers are for the main controller while this admission controller is for the pod preset controller.

It's not an admission controller, per se - it's a controller that implements an initializer. Fine semantic difference, but this should not go under plugin/pkg

Needs to be called "initializer" or something, and the admit should become initialize. I don't want there to be any cross talk of idea/names since they are so different in their lifecycle.

jberkhahn · 2018-03-05T22:20:37Z

pkg/controller/podpreset/admission.go

+
+// mergeVolumes merges given list of Volumes with the volumes injected by given
+// podPresets. It returns an error if it detects any conflict during the merge.
+func mergeVolumes(volumes []corev1.Volume, podPresets []*settingsapi.PodPreset) ([]corev1.Volume, error) {


Is there really no generic methods we can use from PodPreset code over in Kubernetes/Kubernetes? Do we really have to rewrite all of this merge stuff ourselves?

Not sure, but the original podpreset code was checked into Kubernetes using the same functions.

jberkhahn · 2018-03-05T22:25:53Z

pkg/controller/podpreset/admission.go

+		pod.Spec.Containers[i] = ctr
+	}
+
+	// add annotation


This comment is kind of cryptic. Maybe "Initialize annotation map for pod preset information" to say that we using it right now. @MHBauer

jberkhahn · 2018-03-05T22:32:23Z

pkg/controller/podpreset/admission_test.go

+	servicecataloginformers "github.com/kubernetes-incubator/service-catalog/pkg/client/informers_generated/externalversions"
+)
+
+func TestMergeEnv(t *testing.T) {


Is there any reason not to add t.parallel? From: @MHBauer

For unit tests like these, I don't see any benefit to adding parallel. If the concern was for speed, test execution of all the podpreset tests on my system takes 0.059s.

jberkhahn · 2018-03-05T22:35:22Z

pkg/controller/podpreset/admission_test.go

+	}
+}
+
+// NewTestAdmission provides an admission plugin with test implementations of internal structs.  It uses


Why is this block here?

jberkhahn · 2018-03-05T23:14:01Z

pkg/controller/podpreset/controller.go

+		return fmt.Errorf("failed to retrieve pod %s in ns: %v : %v", podName, ns, err)
+	}
+
+	if !needsInitialization(pod) {


It seems weird to @MHBauer and I that we need this check. It seems to me that if we get here and the pod no longer needs initialization something has Gone Wrong because we check if the pod still needs initialization in both places. Is there something we're not seeing?

I believe you're correct, will remove.

MHBauer · 2018-03-07T22:59:12Z

It's written as an admission controller, but it can't really be one, because we'll never be able to see pods before the core apiserver. Since we obviously can't get this code into the core to run, we have to find another way.

As an initializer, we need to make sure this code is running. It could probably be done as either another webserver on the controller, or a completely separate entity which means it needs a new image, and possibly other things.

@jpeeler as this is in-progress, depending on how much time you have to push it forward, I think @jberkhahn and I are very interested in getting this through to the end.

As in-progress, and given how much more work seems to be required, not sure if we want to bring the work in, in stages, or try and finish it all in one shot.

jpeeler · 2018-03-19T19:12:20Z

I added a few files in contrib/examples/podpresets to make this easier to understand how this works. To be clear though, the integration with bindings will come in a later PR.

jberkhahn · 2018-03-21T21:58:25Z

cmd/controller-manager/app/controller_manager.go

@@ -409,7 +410,7 @@ func newPodSharedInformer(kubeClient kubernetes.Interface) cache.SharedIndexInfo
 				return kubeClient.CoreV1().Pods(v1.NamespaceAll).Watch(lo)
 			},
 		},
-		&v1core.Pod{},
+		&corev1.Pod{},


Why did we switch the package here and nowhere else?

MHBauer · 2018-03-21T23:35:04Z

contrib/examples/podpresets/initializer.yaml

@@ -0,0 +1,17 @@
+apiVersion: admissionregistration.k8s.io/v1alpha1
+kind: InitializerConfiguration


I think we need this in the helm chart.

Good idea, added.

jberkhahn · 2018-03-22T00:43:24Z

Hmm, I tried demo-ing this, but I can't get the initializer to work. I create it, but when I go to create the pod, the logs show as no initializers found for the pod, so it skips it.

jpeeler · 2018-03-22T16:26:10Z

I added the initializer configuration to be deployed with helm, so to repeat the testing I've been doing from contrib/examples/podpresets:

kubectl create -f podpreset-preset.yaml
kubectl create -f pod.yaml
kubectl describe po apod

Verify that pod has mutation annotation and added port environment variable.

jpeeler · 2018-03-26T20:50:01Z

Prerequisite commands I've been using to test with:

To start k8s, in kubernetes repo:
KUBELET_FLAGS="--serialize-image-pulls=false" ALLOW_SECURITY_CONTEXT=true LOG_LEVEL=5 hack/local-up-cluster.sh -O

To start helm, in catalog repo:
helm install charts/catalog --name catalog --namespace=catalog --set image=service-catalog:canary,imagePullPolicy=Never,apiserver.podPresetEnabled=true

MHBauer · 2018-03-26T22:00:27Z

I may have started something in the wrong order. I see successfully initialized pods.

MHBauer · 2018-03-27T18:37:44Z

Is there any possibility that the pods created by helm install would get held up by the creation of the intializer that helm is created? Trying to debug why I failed previously.

EronWright · 2018-03-27T20:45:58Z

There's an implied ordering here between PodPresets and Pods. Imagine I had a helm chart containing a Deployment and a PodPreset that should be applied to the pods created by the deployment. There's no guarantee that the preset is in place before the pods are created, correct? Is there a mitigation for this or do we assume that pod presets are generally established long before the relevant consumer is deployed?

pmorie · 2018-03-27T21:02:40Z

Imagine I had a helm chart containing a Deployment and a PodPreset that should be applied to the pods created by the deployment.

Good question. Right now there is no guarantee of ordering that PodPresets make. They only affect pods that are created after the pod preset itself is created.

One next step for this work is to allow deployments to be triggered by the creation of a PodPreset that matches the deployment's label selector. So, for example - if you have a deployment, and later, a PodPreset is created that matches that deployment's label selectors, a new generation of pods will be created.

pmorie

I think this is LGTM modulo some minor nits (see below).

I would say we should have a very basic e2e before this gets merged.

pmorie · 2018-03-27T21:06:03Z

charts/catalog/templates/podpreset-initializer.yaml

+      # apiGroups, apiVersion, resources all support wildcard "*".
+      # "*" cannot be mixed with non-wildcard.
+      - apiGroups:
+          - "*"


I think this should probably just be "" (the 'special' name for the core API group)

pmorie · 2018-03-27T21:07:11Z

cmd/controller-manager/app/controller_manager.go

@@ -360,9 +367,55 @@ func StartControllers(s *options.ControllerManagerServer,
 	glog.V(1).Info("Starting shared informers")
 	informerFactory.Start(stop)

+	if availableResources[podPresetGVR] {


I think we may want to tease this into a separate controller, or at least one that you can run without running the entire service catalog controller, but this is sufficient for now.

pmorie · 2018-03-27T21:07:32Z

cmd/controller-manager/app/controller_manager.go

+			glog.Errorf("error creating podpreset initializer : %v", err)
+			return err
+		}
+		// TODO(droot): check appropriate concurrency for initializer workers


Is this a @jpeeler todo now?

a better way is to use TODO(#issue number):

pmorie · 2018-03-27T21:08:28Z

contrib/examples/podpresets/podpreset-preset.yaml

@@ -0,0 +1,11 @@
+apiVersion: settings.servicecatalog.k8s.io/v1alpha1


how about just podpreset.yaml as the name of this file?

pmorie · 2018-03-27T21:09:04Z

pkg/controller/podpreset/admission.go

@@ -0,0 +1,355 @@
+/*


It's not an admission controller, per se - it's a controller that implements an initializer. Fine semantic difference, but this should not go under plugin/pkg

MHBauer · 2018-03-27T21:37:14Z

There appears to be a race condition with the intializers and the pods created for svc-cat.

Can we ignore pods for svc-cat somehow?

MHBauer · 2018-03-27T21:37:52Z

For example, this is what I see

$ kubectl get pods -n catalog --include-uninitialized catalog-catalog-apiserver-5bc7c8d7fc-hwp56 -o yaml
apiVersion: v1
kind: Pod
metadata:
  creationTimestamp: 2018-03-27T21:33:39Z
  generateName: catalog-catalog-apiserver-5bc7c8d7fc-
  initializers:
    pending:
    - name: podpreset.initializer.k8s.io
  labels:
    app: catalog-catalog-apiserver
    chart: catalog-0.1.8
    heritage: Tiller
    pod-template-hash: "1673748397"
    release: catalog
    releaseRevision: "1"
  name: catalog-catalog-apiserver-5bc7c8d7fc-hwp56

MHBauer · 2018-03-28T04:10:15Z

I need to retract my "let's put it in helm" statement. I don't see anyway to prevent that from happening.

We can exempt our pods from all initialization, but maybe some other initializer was present, we'd skip the extra one.

Even just sequencing the add of the initializer after the start of the controller is problematic, as if the controller pod ever needs to be destroyed and recreated, the initializer needs to be disabled for that time period. So things will miss initialization during that time period.

Is there some special exemption here that I don't know about? It seems completely untenable.

jpeeler · 2018-03-28T21:34:42Z

Turns out there really isn't a good way to prevent this ordering issue. I'm looking into not using initializers and instead using a mutating webhook.

MHBauer · 2018-05-25T23:39:27Z

@jpeeler still active on finding a solution?

jpeeler · 2018-05-28T16:45:02Z

I'm working on a solution in jpeeler/podpreset-crd. Once it's ready for merging into catalog, I'll close this PR with a link to that PR. Though if people want to just close this PR, then that's fine too.

pmorie · 2018-05-29T22:11:47Z

We should probably close this PR. Jeff, will you prepareademo for the next SIG meeting?

…

On Mon, May 28, 2018 at 6:45 PM Jeff Peeler ***@***.***> wrote: I'm working on a solution in jpeeler/podpreset-crd. Once it's ready for merging into catalog, I'll close this PR with a link to that PR. Though if people want to just close this PR, then that's fine too. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1754 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAWXmCnenvZErpgowuPt4ObYLimX_u2Nks5t3CmPgaJpZM4SObV-> .

jpeeler · 2018-05-29T22:29:23Z

I can demo next week.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 21, 2018

PodPreset initializer implementation

7f9d50c

jpeeler force-pushed the pr1195-rebase branch from fccabb6 to 4a1a785 Compare February 21, 2018 22:43

jberkhahn reviewed Mar 5, 2018

View reviewed changes

jpeeler force-pushed the pr1195-rebase branch from eebc8da to c572ea6 Compare March 19, 2018 19:06

jpeeler changed the title ~~WIP: PodPreset initializer implementation~~ PodPreset initializer implementation Mar 19, 2018

jpeeler force-pushed the pr1195-rebase branch from c572ea6 to e7ad772 Compare March 20, 2018 15:30

jberkhahn reviewed Mar 21, 2018

View reviewed changes

MHBauer reviewed Mar 21, 2018

View reviewed changes

post rebase changes

0bc9bc7

jpeeler force-pushed the pr1195-rebase branch from e7ad772 to 0bc9bc7 Compare March 22, 2018 16:22

pmorie reviewed Mar 27, 2018

View reviewed changes

jpeeler closed this May 29, 2018

		@@ -0,0 +1,17 @@
		apiVersion: admissionregistration.k8s.io/v1alpha1
		kind: InitializerConfiguration

		@@ -0,0 +1,11 @@
		apiVersion: settings.servicecatalog.k8s.io/v1alpha1

PodPreset initializer implementation #1754

PodPreset initializer implementation #1754

Conversation

jpeeler commented Feb 21, 2018

jpeeler commented Feb 21, 2018

MHBauer commented Feb 21, 2018

pmorie commented Feb 21, 2018

jberkhahn left a 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

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

MHBauer commented Mar 7, 2018

jpeeler commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jberkhahn commented Mar 22, 2018

jpeeler commented Mar 22, 2018

jpeeler commented Mar 26, 2018

MHBauer commented Mar 26, 2018

MHBauer commented Mar 27, 2018

EronWright commented Mar 27, 2018

pmorie commented Mar 27, 2018

pmorie left a 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

n3wscott Mar 29, 2018 • edited

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

MHBauer commented Mar 27, 2018

MHBauer commented Mar 27, 2018

MHBauer commented Mar 28, 2018

jpeeler commented Mar 28, 2018

MHBauer commented May 25, 2018

jpeeler commented May 28, 2018

pmorie commented May 29, 2018 via email

jpeeler commented May 29, 2018

n3wscott Mar 29, 2018 •

edited