deploy: fix the owner reference kind to be rc #13996

mfojtik · 2017-05-02T14:31:17Z

This PR will:

Fix the "kind" field in the ownerRef from deployerPod to point to RC instead of DC
Automatically set ownerRef from lifecycle hook pods created by deployer pods to RC (so we clean them up when the RC is removed)
Fix infra policy to use deployment SA instead of deploymentConfig SA in deployment controller
Fix permissions on pods for deploymentConfig SA (don't need to patch, update, delete or create)
Use the SA client in deploymentConfig controller instead of privileged client

mfojtik · 2017-05-02T14:34:44Z

[test]

mfojtik · 2017-05-02T14:45:46Z

pkg/deploy/controller/deploymentconfig/controller.go

@@ -435,7 +436,9 @@ func (c *DeploymentConfigController) cleanupOldDeployments(existingDeployments [
 			continue
 		}

-		err := c.rn.ReplicationControllers(deployment.Namespace).Delete(deployment.Name, nil)
+		err := c.rn.ReplicationControllers(deployment.Namespace).Delete(deployment.Name, &metav1.DeleteOptions{


@Kargakis check.

deads2k · 2017-05-02T14:48:55Z

pkg/deploy/controller/deployment/controller.go

 				APIVersion: "v1",
-				Kind:       deployapi.Kind("DeploymentConfig").Kind,
+				Kind:       kapi.Kind("ReplicationController").Kind,


@ncdc any chance this is the source of your problem? The names wouldn't match, right?

0xmichalis · 2017-05-02T14:50:25Z

Needs a test that verifies GC of deployers works.

deads2k · 2017-05-02T14:50:37Z

pkg/deploy/controller/deployment/controller.go

 				APIVersion: "v1",
-				Kind:       deployapi.Kind("DeploymentConfig").Kind,
+				Kind:       kapi.Kind("ReplicationController").Kind,


Actually, @mfojtik this pod is more related to the DC than the RC and should be cleaned up when the DC is deleted, right? That would mean the kind was correct and the name and UID were wrong.

it will be cleaned up because when you delete DC that should trigger deletion of RC and that should remove the deployer pod. Similarely when you set revisionHistory, this should automatically remove failed deployer pods (success deployer pods are removed automatically) as we delete old RC. correct @Kargakis ?

@mfojtik correct, this pod is more related to the RC than the DC.

We also need to set finalizers in the RCs for enabling a cascading deletion from a DC to reach pods but that should be part of https://trello.com/c/B10xLQ6L/927-set-owner-refs-in-rcs-owned-by-dcs.

0xmichalis · 2017-05-02T14:51:12Z

Needs a test that verifies GC of deployers works.

@mfojtik I think you can re-use the existing test for cleaning up older RCs.

ncdc · 2017-05-02T14:52:23Z

What I'd really like to see is a test that can reliably reproduce the transient deletion we saw in Jenkins that is then fixed by this PR (in whatever form it ends up in).

mfojtik · 2017-05-03T10:44:31Z

@ncdc @Kargakis @deads2k does kube GC even check the "kind" ? it seems like the UUID is correct (it points to the RC). Also i'm not able to reproduce the failure Andy saw... I tried:

Make pre-hook with sleep 120 (in theory the deployer pod should be erased prematurely by GC?)
Make pre-hook with abort policy and exit 1, which will cause deployment controller to keep the pod around (not removed by deployment controller)... it works as expected...
Same as 2) but set revisionHistory limit to 2, I see all the failed deployer pods and hook pods sitting around and they are never garbage collected (with this broken, I would expect they will be vanished as they point to an non-existent DC (the UUID is RC).

... unless the GC only checks the UUID and not the Kind. In that case the cause of the problem is somewhere else and this is just a cosmetic issue.

EDIT: @Kargakis do we have logic in place when I delete the RC and the failed deployer pod is deleted automatically as well? (because that is what I see)... also reminds me that we should probably also set the owner reference for the hook pods...

mfojtik · 2017-05-03T11:07:22Z

OK, so it seems like the GC honors the UID from the owner reference, which means this is not fixing #13995 but we should fix this anyway...

i'll add owner reference to hook pods in this PR.

0xmichalis · 2017-05-03T11:14:11Z

Make sure that when we cleanup RCs based on revisionHistoryLimit, we delete with propagationPolicy set to Background.

ncdc · 2017-05-03T11:25:38Z

The failure that I originally found was from a CI run for #13886. It didn't look like that PR would cause the problem. Either way, I still haven't reasoned through how this could have happened. That's why I want a test case that can "slow down time" and try to be prescriptive about what happens and see if we can reproduce.

mfojtik · 2017-05-03T11:33:17Z

@ncdc sure, I will work on that test, I think this PR is not related to that issue at this point.

@deads2k @liggitt @enj will you guys have any objections granting the "deployer" SA ability to delete pods in the project? When I add ownerRef in a hook pod created by the deployer I'm hitting this error:

 cannot set an ownerRef on a resource you can't delete: User "system:serviceaccount:test:deployer" cannot delete pods in project

There are two options we discussed with @Kargakis:

Grant deployer delete
Update the failed hook pods (which are subject of GC when the RC is removed) with ownerRef from the controller (basically when a deployer controller see a failed hook pod, it automatically patches it with the ownerRef).

I like 1) because it is cleaner, but less secure, but will take an advise from you :)

EDIT: so i tried the controller approach and got

I0502 21:06:11.518767    6460 deployment_controller.go:217] error saetting owner references: User "system:serviceaccount:openshift-infra:deploymentconfig-controller" cannot "patch" "pods" with name "nginx-15-hook-pre" in project "test"

but I guess making the controller more privileged is better than making the SA more privileged?

mfojtik · 2017-05-03T12:45:33Z

pkg/cmd/server/origin/master_config.go

@@ -998,7 +998,7 @@ func (c *MasterConfig) DeploymentConfigInstantiateClients() (*osclient.Client, k

 // DeploymentControllerClients returns the deployment controller client objects
 func (c *MasterConfig) DeploymentControllerClients() (*osclient.Client, kclientsetinternal.Interface, kclientsetexternal.Interface) {
-	_, osClient, internalKubeClientset, externalKubeClientset, err := c.GetServiceAccountClients(bootstrappolicy.InfraDeploymentConfigControllerServiceAccountName)
+	_, osClient, internalKubeClientset, externalKubeClientset, err := c.GetServiceAccountClients(bootstrappolicy.InfraDeploymentControllerServiceAccountName)


@deads2k fyi

mfojtik · 2017-05-03T12:46:58Z

pkg/cmd/server/bootstrappolicy/infra_sa_policy.go

@@ -208,7 +208,7 @@ func init() {
 				},
 				// DeploymentController.podClient
 				{
-					Verbs:     sets.NewString("get", "list", "create", "watch", "delete", "update"),
+					Verbs:     sets.NewString("get", "list", "watch"),


@deads2k down here we have:

{ Verbs: sets.NewString("create", "update", "patch"), Resources: sets.NewString("events"), },

and this is copied in other policies... do we really need PATCH and UPDATE for events?

Maybe patching/updating events has to do with event compression?

enj · 2017-05-05T21:48:16Z

will you guys have any objections granting the "deployer" SA ability to delete pods in the project

@mfojtik I am missing some context on this PR but shouldn't you use a custom controller SA for this? @deads2k's #14033 is working towards that. Then you could grant that SA whatever permissions it needed. Maybe there are other issues I am missing in regards to this being inside a project...

I cannot comment on if you actually have other viable solutions to the problem as a whole.

0xmichalis · 2017-05-05T21:53:49Z

@mfojtik I am missing some context on this PR but shouldn't you use a custom controller SA for this? @deads2k's #14033 is working towards that. Then you could grant that SA whatever permissions it needed. Maybe there are other issues I am missing in regards to this being inside a project...

I cannot comment on if you actually have other viable solutions to the problem as a whole.

We are already using a custom sa for the deployer, the question is "do we want to give pod deletion power to the deployer?"

0xmichalis · 2017-05-05T21:56:00Z

pkg/deploy/controller/deployment/deployment_controller.go

+	}
+
+	encoder := kapi.Codecs.LegacyCodec(kapi.Registry.EnabledVersions()...)
+	errors := []error{}


var errors []error

0xmichalis · 2017-05-05T22:02:11Z

pkg/deploy/controller/deployment/deployment_controller.go

+		}
+		newPod, ok := objCopy.(*kapi.Pod)
+		if !ok {
+			errors = append(errors, fmt.Errorf("object %#+v is not a pod", objCopy))


It's unlike that this error will ever be hit but if you want it, that's fine.

@Kargakis for the sake of somebody will copy&paste this code somewhere else :-)

enj · 2017-05-05T22:08:22Z

We are already using a custom sa for the deployer, the question is "do we want to give pod deletion power to the deployer?"

@Kargakis Ah OK. Not really sure on that one.

mfojtik · 2017-05-09T09:46:10Z

@deads2k @Kargakis do we want to merge this or hold? (this is a bugfix)

0xmichalis · 2017-05-09T11:28:40Z

pkg/deploy/controller/deployment/deployment_controller.go

+		oldPodBytes, err := runtime.Encode(encoder, pod)
+		if err != nil {
+			errors = append(errors, err)
+		}


Missing a continue?

good catch, fixed.

0xmichalis · 2017-05-09T13:11:19Z

@deads2k @Kargakis do we want to merge this or hold? (this is a bugfix)

I am fine with merging.

mfojtik · 2017-05-19T14:53:02Z

@deads2k rolled back the changes to ICT controller, will open a separate PR for that once this merge.

…ler init func

mfojtik · 2017-05-22T11:57:22Z

@bparees @coreydaley pls check, I made some tweaks to accomodate the cleanup policy (i think the build controller now needs ability to delete builds and create build configs?)

openshift-bot · 2017-05-22T12:13:24Z

Evaluated for origin test up to c3d4af7

openshift-bot · 2017-05-22T12:21:21Z

Evaluated for origin merge up to c3d4af7

openshift-bot · 2017-05-22T12:21:28Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/744/) (Base Commit: a666b7c) (Image: devenv-rhel7_6247)

openshift-bot · 2017-05-22T13:48:42Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1605/) (Base Commit: a666b7c)

bparees · 2017-05-22T17:29:17Z

@bparees @coreydaley pls check, I made some tweaks to accomodate the cleanup policy (i think the build controller now needs ability to delete builds and create build configs?)

@mfojtik looks like this merged before we had a chance to look at it... why would the build controller need the ability to create buildconfigs?

0xmichalis · 2017-05-22T17:35:55Z

@mfojtik looks like this merged before we had a chance to look at it... why would the build controller need the ability to create buildconfigs?

If you delete something with an owner ref, you have to be able to create owners. I think.

0xmichalis · 2017-05-22T17:38:33Z

If you delete something with an owner ref, you have to be able to create owners. I think.

No, I confused this with another rule: adding owner refs to things means you can also delete them. Not sure about this one.

mfojtik · 2017-05-22T20:06:17Z

sorry it was not create but delete build and get bc (https://github.com/openshift/origin/blob/master/pkg/cmd/server/origin/controller/build.go#L59)

bparees · 2017-05-22T20:09:12Z

ok yeah those make sense.

0xmichalis · 2017-05-23T00:08:37Z

pkg/deploy/controller/deploymentconfig/deploymentconfig_controller.go

@@ -428,7 +429,10 @@ func (c *DeploymentConfigController) cleanupOldDeployments(existingDeployments [
 			continue
 		}

-		err := c.rn.ReplicationControllers(deployment.Namespace).Delete(deployment.Name, nil)
+		policy := metav1.DeletePropagationBackground
+		err := c.rn.ReplicationControllers(deployment.Namespace).Delete(deployment.Name, &metav1.DeleteOptions{


@tnozicka fyi this is related to DeploymentConfig GC

mfojtik force-pushed the fix-owner-ref branch from 66548ed to 93687c3 Compare May 2, 2017 14:33

mfojtik requested a review from 0xmichalis May 2, 2017 14:34

mfojtik force-pushed the fix-owner-ref branch from 307b8b5 to c8a7f49 Compare May 2, 2017 14:45

mfojtik commented May 2, 2017

View reviewed changes

mfojtik force-pushed the fix-owner-ref branch from c8a7f49 to 9ec3e4c Compare May 2, 2017 14:48

deads2k reviewed May 2, 2017

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2017

mfojtik force-pushed the fix-owner-ref branch from 9ec3e4c to 1794bfc Compare May 3, 2017 08:57

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2017

mfojtik commented May 3, 2017

View reviewed changes

0xmichalis reviewed May 5, 2017

View reviewed changes

mfojtik force-pushed the fix-owner-ref branch from 2a4c744 to 0e8a1ee Compare May 9, 2017 09:37

0xmichalis reviewed May 9, 2017

View reviewed changes

mfojtik force-pushed the fix-owner-ref branch from b4a2292 to d0d9ed0 Compare May 19, 2017 14:52

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

deads2k and others added 8 commits May 22, 2017 10:45

move openshift controller roles to system:openshift:controller:*

714f56a

rewire build controller initialization to use a controller init func

bb7c275

deploy: fix the owner reference kind to be rc

aa35b69

deploy: set background propagation policy for old deployment cleanup

8ca79b0

deploy: automatically set ownerRef for hook pods when rollout fail

f10dae7

deploy: rename deployment controller to deployer controller

a622059

deploy: use correct client for deployer controller

6ddf8dc

auth: add rbac roles for deployments

4bd76cf

mfojtik force-pushed the fix-owner-ref branch from d0d9ed0 to 5266887 Compare May 22, 2017 10:22

deploy: rewire deployment controllers initialization to use a control…

c3d4af7

…ler init func

mfojtik force-pushed the fix-owner-ref branch from 5266887 to c3d4af7 Compare May 22, 2017 11:23

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 22, 2017

openshift-bot merged commit dc33c36 into openshift:master May 22, 2017

0xmichalis reviewed May 23, 2017

View reviewed changes

This was referenced May 23, 2017

Refactor controller initialization (part 2) #14293

Merged

Garbage collector deleted deployer pod prematurely #13995

Closed

mfojtik deleted the fix-owner-ref branch September 5, 2018 21:07

deploy: fix the owner reference kind to be rc #13996

deploy: fix the owner reference kind to be rc #13996

Conversation

mfojtik commented May 2, 2017 • edited Loading

mfojtik commented May 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmichalis commented May 2, 2017

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

0xmichalis commented May 2, 2017

ncdc commented May 2, 2017

mfojtik commented May 3, 2017 • edited Loading

mfojtik commented May 3, 2017

0xmichalis commented May 3, 2017 • edited Loading

ncdc commented May 3, 2017

mfojtik commented May 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enj commented May 5, 2017

0xmichalis commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enj commented May 5, 2017

mfojtik commented May 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmichalis commented May 9, 2017

mfojtik commented May 19, 2017

mfojtik commented May 22, 2017

openshift-bot commented May 22, 2017

openshift-bot commented May 22, 2017

openshift-bot commented May 22, 2017 • edited Loading

openshift-bot commented May 22, 2017

bparees commented May 22, 2017

0xmichalis commented May 22, 2017

0xmichalis commented May 22, 2017

mfojtik commented May 22, 2017

bparees commented May 22, 2017

Choose a reason for hiding this comment

mfojtik commented May 2, 2017 •

edited

Loading

mfojtik commented May 3, 2017 •

edited

Loading

0xmichalis commented May 3, 2017 •

edited

Loading

mfojtik commented May 3, 2017 •

edited

Loading

openshift-bot commented May 22, 2017 •

edited

Loading