deploy: remove generic deployment trigger controller #14910

mfojtik · 2017-06-27T12:45:56Z

Make main deployment config controller trigger rollouts on config changes and image changes.

This replaces the generictrigger controller which was calling instantiate to trigger rollouts, but we don't want to call instantiate from a controller.

This also handles the logic of initial deployment rollouts (with ICT/without ICT/...). The image change trigger is now delegated to @smarterclayton trigger controller so we only observe an image change which we treat differently from a config change.

This should be fully backward compatible with old logic.

mfojtik · 2017-06-27T12:46:10Z

[test]

@Kargakis @deads2k one less controller to worry about?

mfojtik · 2017-06-27T12:47:15Z

pkg/deploy/controller/deploymentconfig/deploymentconfig_controller.go

@@ -63,6 +63,8 @@ type DeploymentConfigController struct {

 	// queue contains deployment configs that need to be synced.
 	queue workqueue.RateLimitingInterface
+	// instantiateQueue contains deployment config that should be instantiated.
+	instantiateQueue workqueue.RateLimitingInterface


@Kargakis i didn't want to pollute the main DC controller queue with instantiate calls, that's why I made separate queue just for instantiating (terrible word).

0xmichalis · 2017-06-27T12:52:07Z

pkg/deploy/controller/deploymentconfig/factory.go

+		return false
+	}
+
+	if _, err := c.dn.DeploymentConfigs(dc.Namespace).Instantiate(deployutil.DefaultInstantiateRequestForConfig(dc.Name)); err != nil {


No, please, we should stop using instantiate inside a controller. Also I was imagining that we could do everything in a single queue.

@Kargakis what you want to do instead? just update latest version?

We need to move away from the assumption that bumping latestVersion starts a deployment because that is a status field.

mfojtik · 2017-06-27T16:35:52Z

@Kargakis PTAL

[test]

mfojtik · 2017-06-27T16:37:00Z

(this need a unit test to guard us from adding labels/annotations to the RC templates otherwise bad things will happen).

mfojtik · 2017-06-27T16:37:12Z

@tnozicka for the review as well :)

0xmichalis · 2017-06-27T16:44:17Z

pkg/cmd/server/bootstrappolicy/controller_policy.go

@@ -95,18 +95,6 @@ func init() {
 			rbac.NewRule("create", "get", "list", "watch", "update", "patch", "delete").Groups(kapiGroup).Resources("replicationcontrollers").RuleOrDie(),


Break down the removal of this controller into a separate commit.

0xmichalis · 2017-06-27T16:45:33Z

pkg/deploy/util/util.go

+		return true, "", err
+	}
+	rcTemplate := strippedRc.Spec.Template
+	// there is difference between map[string]string(nil) vs. map[string]string{}


of course there is :)

just for clarification if somebody will wonder why i'm doing below that ;-)

0xmichalis · 2017-06-27T16:48:30Z

pkg/deploy/util/util.go

+	if err != nil {
+		return nil, err
+	}
+	delete(copy.Spec.Template.Annotations, deployapi.DeploymentVersionAnnotation)


Follow the upstream pattern: https://github.com/kubernetes/kubernetes/blob/ede78d9ee7706b5d5dab943e2b56f373308f578e/pkg/controller/deployment/util/deployment_util.go#L281

looks nicer, sure

0xmichalis · 2017-06-27T16:55:13Z

pkg/deploy/controller/deploymentconfig/deploymentconfig_controller.go

+	// If we already have latest deployment running but the config template does not match
+	// the latest deployment assume this is a config change and trigger a new deployment if
+	// the config change trigger is enabled.
+	if deployutil.HasChangeTrigger(config) && !config.Spec.Paused {


Break this out in its own func

0xmichalis · 2017-06-27T19:06:37Z

Fixes #13763

mfojtik · 2017-06-27T19:24:48Z

@Kargakis addressed comments, but the integration tests looks bad.. seems like ICT is broken... i thought we now use generic triggers for ICT changes?

mfojtik · 2017-06-27T20:49:15Z

@Kargakis this got pretty complicated now :-) basically @smarterclayton controller is doing config change (to replace images)... however we have to record that as image change... you can also have DC with just ICT and without CTT... So the logic to handle this gets a bit crazier now.

mfojtik · 2017-06-28T10:02:35Z

@Kargakis

ok      TestTriggers_MultipleICTs
ok      TestTriggers_configChange
ok      TestTriggers_imageChange
ok      TestTriggers_imageChange_nonAutomatic
ok      TestTriggers_manual

victory!

mfojtik · 2017-06-28T10:07:50Z

re-[test]

mfojtik · 2017-06-28T10:10:09Z

pkg/deploy/util/util.go

+	config.Status.Details = new(deployapi.DeploymentDetails)
+	config.Status.Details.Causes = []deployapi.DeploymentCause{{
+		Type:         deployapi.DeploymentTriggerOnImageChange,
+		ImageTrigger: &deployapi.DeploymentCauseImageTrigger{From: api.ObjectReference{Kind: "DockerImage", Name: name}},


@Kargakis i guess this is ok, but just to be sure, this will always record the docker image (not image stream)... to do that I will have to search the triggers and find the trigger with lastTriggeredImage matching the name... can do that but don't want to complicate this if we can avoid it. wdyt?

mfojtik · 2017-06-28T10:11:33Z

pkg/deploy/util/util.go

@@ -375,13 +504,17 @@ func MakeDeploymentV1(config *deployapi.DeploymentConfig, codec runtime.Codec) (
 	for k, v := range config.Spec.Template.Labels {
 		podLabels[k] = v
 	}
+	// NOTE: You need to update the labelsToSkip variable in pkg/deploy/util when
+	// adding a new label here.


@Kargakis guess this check is enough? i don't think we want to keep adding more annotations or labels but if we do, this will remind you to update the list.

mfojtik · 2017-06-28T11:55:08Z

pkg/deploy/controller/deploymentconfig/deploymentconfig_controller.go

@@ -181,6 +181,21 @@ func (c *DeploymentConfigController) Handle(config *deployapi.DeploymentConfig)
 		c.recorder.Eventf(config, v1.EventTypeNormal, "DeploymentAwaitingCancellation", "Deployment of version %d awaiting cancellation of older running deployments", config.Status.LatestVersion)
 		return fmt.Errorf("found previous inflight deployment for %s - requeuing", deployutil.LabelForDeploymentConfig(config))
 	}
+	// Process triggers and start an initial rollouts
+	configCopy, err := deployutil.DeploymentConfigDeepCopy(config)


@Kargakis this is needed to avoid cache mutation (i believe)

openshift-bot · 2017-10-05T06:48:11Z

/retest

Please review the full test history for this PR and help us cut down flakes.

mfojtik · 2017-10-05T08:45:26Z

EDIT: So I was wrong, we used to compare the encoded DC template with the current DC but this PR switched that to compare RC template vs. DC template (which is what upstream does). I haven't realized that is big change and can cause a lot of rollouts and vaporize all changes users does to their RCs (which they should never-ever touch, but Origin allows that..)

@bparees @jim-minter @tnozicka so I figured out what caused the continuous rollouts for the mysql deployment config. Basically the DC generated with new-app (or I assume template) has RC template with "name":

  template:
    metadata:
      annotations:
        openshift.io/generated-by: OpenShiftNewApp
      creationTimestamp: null
      labels:
        app: cakephp-mysql-persistent
        name: mysql
      name: mysql # <--

However, when we create the deployment (RC) from this template, we don't copy config.Spec.Template.Name into the deployment. On reconcile, when the deployment config controller runs, it sees this as a configuration diff:

I1005 08:25:14.891263   27502 deploymentconfig_controller.go:610] Rolling out #6 deployment for myproject/mysql caused by config change, dif
f:.
object.ObjectMeta.Name:
  a: "mysql"
  b: ""

(thanks God I added this diffs ;-) This means the DC controller will keep bumping the LatestVersion because it thinks there was a config change in the DC but since we never copy the "name" into the RC it will do that forever... This only affects templates that has "name" set, which is the reason why none of our deployment conformance tests captured this. Also this was broken for a long time, so I dunno how the old config change trigger controller managed this....

EDIT: It also open a question whether we should copy everything from the template ObjectMeta to the RC to prevent this from happening if somebody sets 'namespace' for example, or whatever other field in ObjectMeta. Other option is to completely ignore the the ObjectMeta.

mfojtik · 2017-10-05T09:18:34Z

actually copying the name/etc might not be a good solution here because it will cause all existing DC with latest deployment not having them to trigger which might lead to a mass redeployment in online or in big clusters....

mfojtik · 2017-10-05T09:31:07Z

@tnozicka i'm leaning toward to just ignore comparing the name in config change trigger for now to avoid mass rollouts. I will create issue to track this in 3.8.

tnozicka · 2017-10-05T10:22:21Z

/lgtm

…paring rc template

mfojtik · 2017-10-05T12:12:34Z

pkg/apps/apis/apps/test/ok.go

@@ -166,6 +166,7 @@ func OkPodTemplate() *kapi.PodTemplateSpec {
 			DNSPolicy:                     kapi.DNSClusterFirst,
 			TerminationGracePeriodSeconds: &one,
 			SchedulerName:                 kapi.DefaultSchedulerName,
+			SecurityContext:               &kapi.PodSecurityContext{},


@Kargakis @deads2k @tnozicka honestly, I have no idea why this is needed.... I spend last hour or so trying to figure out why the TestHandle() is failing, but without this the HasLatestPodTemplate() will see a diff where this field is nil vs. defaulted...

Just want to double check, but the codec i'm passing down to HasLatestPodTemplate is the same as we passed to the original trigger (annotationCodec), so I have no idea what happened there...

also let this serve as argument for removing the annotations with encoded deployment configs

@mfojtik I am not sure about this, it has been nil even before no matter the codec in tests - it might be sign of something else going wrong

@tnozicka it might well be that previous state was wrong :-) i checked the encoded DC in RC and they seems correct (in running server).

so my theory is this:

Before we haven't decoded the DC nor we did the deepequal in main deployment config controller, because that was part of generic trigger.

The SecurityContext{} is set to empty by default, see kube testing for pod_specs.

So I think this change is fine.

mfojtik · 2017-10-05T14:06:13Z

@tnozicka and we are green \o/

jim-minter · 2017-10-05T14:17:49Z

@mfojtik are there any opportunities to increase test coverage in this codebase as part of your PR, e.g. around the case of repeated redeployments?

mfojtik · 2017-10-05T15:24:59Z

@jim-minter i will want to do some trigger tests as follow ups. The continuous rollouts was broken because of the new logic that this PR introduced. It is good that it was found via templateinstance, but we should really have unit tests for this.

tnozicka · 2017-10-05T16:05:40Z

I am ok with follow up
/lgtm

openshift-merge-robot · 2017-10-05T16:19:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mfojtik, tnozicka

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/apps/OWNERS~~ [mfojtik]
~~pkg/cmd/OWNERS~~ [mfojtik]
~~test/extended/OWNERS~~ [mfojtik]
~~test/integration/OWNERS~~ [mfojtik]
~~test/testdata/OWNERS~~ [mfojtik]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

tnozicka · 2017-10-05T18:05:44Z

Flake #16370
/test unit
Flake #11315
/test extended_networking_minimal

openshift-merge-robot · 2017-10-05T19:17:33Z

Automatic merge from submit-queue.

deads2k · 2017-10-05T20:12:55Z

/retest

mfojtik commented Jun 27, 2017

View reviewed changes

0xmichalis reviewed Jun 27, 2017

View reviewed changes

mfojtik force-pushed the remove-generic-trigger branch from 517b825 to d7f2e8d Compare June 27, 2017 12:52

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

mfojtik force-pushed the remove-generic-trigger branch 2 times, most recently from c2271e9 to 9f86a34 Compare June 27, 2017 16:35

0xmichalis reviewed Jun 27, 2017

View reviewed changes

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

mfojtik force-pushed the remove-generic-trigger branch from 9f86a34 to 93d23c2 Compare June 27, 2017 19:23

mfojtik added this to the 3.7.0 milestone Jun 27, 2017

mfojtik force-pushed the remove-generic-trigger branch 2 times, most recently from eb67a45 to 32425bb Compare June 28, 2017 09:41

mfojtik force-pushed the remove-generic-trigger branch from 32425bb to ab0fe2a Compare June 28, 2017 10:04

mfojtik commented Jun 28, 2017

View reviewed changes

mfojtik force-pushed the remove-generic-trigger branch from ab0fe2a to 32f43ec Compare June 28, 2017 11:51

mfojtik commented Jun 28, 2017

View reviewed changes

mfojtik force-pushed the remove-generic-trigger branch from 32f43ec to b16e766 Compare June 28, 2017 12:00

openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 5, 2017

mfojtik added 2 commits October 5, 2017 11:31

apps: remove deployment trigger controller

4c259e2

apps: update generations in extended test

0e6fb69

mfojtik force-pushed the remove-generic-trigger branch from 5d0e6ad to 1ec84c3 Compare October 5, 2017 09:32

mfojtik mentioned this pull request Oct 5, 2017

Trigger rollouts based on diff between DC template and RC template #16695

Open

mfojtik force-pushed the remove-generic-trigger branch from 1ec84c3 to 87f31f2 Compare October 5, 2017 10:13

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 5, 2017

apps: switch back to comparing encoded template config instead of com…

a505706

…paring rc template

mfojtik force-pushed the remove-generic-trigger branch from 87f31f2 to a505706 Compare October 5, 2017 12:09

openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 5, 2017

mfojtik commented Oct 5, 2017

View reviewed changes

mfojtik added the lgtm Indicates that a PR is ready to be merged. label Oct 5, 2017

openshift-merge-robot merged commit 3c845a0 into openshift:master Oct 5, 2017

This was referenced Oct 6, 2017

Multiple running deployments reported in a test - invariant violated #16003

Closed

Switch the generic trigger controller to use apps/deployments #14543

Closed

0xmichalis mentioned this pull request Oct 12, 2017

Collapse config change triggers into the main DC controller #13763

Closed

mfojtik deleted the remove-generic-trigger branch September 5, 2018 21:08

		@@ -95,18 +95,6 @@ func init() {
		rbac.NewRule("create", "get", "list", "watch", "update", "patch", "delete").Groups(kapiGroup).Resources("replicationcontrollers").RuleOrDie(),

deploy: remove generic deployment trigger controller #14910

deploy: remove generic deployment trigger controller #14910

Conversation

mfojtik commented Jun 27, 2017 • edited Loading

mfojtik commented Jun 27, 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

mfojtik commented Jun 27, 2017

mfojtik commented Jun 27, 2017

mfojtik commented Jun 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmichalis commented Jun 27, 2017

mfojtik commented Jun 27, 2017

mfojtik commented Jun 27, 2017

mfojtik commented Jun 28, 2017

mfojtik commented Jun 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-bot commented Oct 5, 2017

mfojtik commented Oct 5, 2017 • edited Loading

mfojtik commented Oct 5, 2017

mfojtik commented Oct 5, 2017

tnozicka commented Oct 5, 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

Choose a reason for hiding this comment

mfojtik commented Oct 5, 2017

jim-minter commented Oct 5, 2017

mfojtik commented Oct 5, 2017

tnozicka commented Oct 5, 2017 • edited Loading

openshift-merge-robot commented Oct 5, 2017

tnozicka commented Oct 5, 2017 • edited Loading

openshift-merge-robot commented Oct 5, 2017

deads2k commented Oct 5, 2017

mfojtik commented Jun 27, 2017 •

edited

Loading

mfojtik commented Oct 5, 2017 •

edited

Loading

tnozicka commented Oct 5, 2017 •

edited

Loading

tnozicka commented Oct 5, 2017 •

edited

Loading