Add labels/annotations to DeploymentStrategy #5149

pedro-r-marques · 2015-10-15T20:36:28Z

This PR allows for the deployer to work in a scenario where an application is using per-tier network segmentation.

For instance, the rails-postgresql-example app sets the label "name": rails-postgresql-example. OpenContrail creates a network segment for this project/tier. The postgresql database sits in the network "default" (by the fact that no label is specified).

The deploymentConfig uses the following labels:

+        "labels": {
+          "name": "rails-postgresql-example",
+          "uses": "default"
+        },

This allows the deployer to be in the same application as the application front-end and deployer-hook to access the postgresql db.

ironcladlou · 2015-10-15T20:51:32Z

cc @Kargakis @smarterclayton

The use case makes sense, but I'd like to get some wider discussion on the implementation. Today, the deployer/hook pods will inherit deploymentConfig.spec.template.spec.nodeSelector to allow the deployment logic to run in a pod scheduled similiarly to the application they're deploying. The label scenario seems similar conceptually: you want the deployer/hook pods to share properties of the application pods to facilitate interoperation.

My concern is confusion arising from inconsistent rules: for nodeSelector, the pod template is used, but for labels, the deploymentConfig itself is used.

I think at the time we decided on the current behavior, we explicitly avoided adding new fields to configure the deployer pods, opting instead for conventions. Does the label requirement warrant a new API field to provide the user with a precise way to express intent, or can we get away with more conventions?

smarterclayton · 2015-10-15T21:17:42Z

We have to be careful about sharing all labels - because pod autoscalers
must not control the hook or process pods.

On Oct 15, 2015, at 4:51 PM, Dan Mace notifications@github.com wrote:

cc @Kargakis https://github.com/kargakis @smarterclayton
https://github.com/smarterclayton

The use case makes sense, but I'd like to get some wider discussion on the
implementation. Today, the deployer/hook pods will inherit
deploymentConfig.spec.template.spec.nodeSelector to allow the deployment
logic to run in a pod scheduled similiarly to the application they're
deploying. The label scenario seems similar conceptually: you want the
deployer/hook pods to share properties of the application pods to
facilitate interoperation.

My concern is confusion arising from inconsistent rules: for nodeSelector,
the pod template is used, but for labels, the deploymentConfig itself is
used.

I think at the time we decided on the current behavior, we explicitly
avoided adding new fields to configure the deployer pods, opting instead
for conventions. Does the label requirement warrant a new API field to
provide the user with a precise way to express intent, or can we get away
with more conventions?

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

liggitt · 2015-10-15T21:25:48Z

yeah, no auto-propagation of deploymentConfig labels down to pods. this has been attempted enough times for build pods and deployer pods, we should add tests to make sure this doesn't happen

smarterclayton · 2015-10-15T21:38:16Z

Having some way to let a user explicitly set labels is probably ok. It
would be an explicit set vs getting no labels (or just an Openshift scoped
one).

On Oct 15, 2015, at 5:25 PM, Jordan Liggitt notifications@github.com
wrote:

yeah, no propagation of labels down to pods. this has been attempted enough
times for build pods and deployer pods, we should add tests to make sure
this doesn't happen

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

pedro-r-marques · 2015-10-15T22:18:17Z

@liggitt @smarterclayton Would you consider propagating labels that do not have a prefix of "kubernetes.io" and "openshift.io" ? These are user defined labels and in my opinion it would be reasonable to pass them from the deployment config to the pods... or do you propose introducing a different field in the deployment config ?
I'm happy to restructure the PR in any way you suggest.

liggitt · 2015-10-15T22:32:14Z

Not automatic propagation. Propagation would have to be explicitly indicated by the user, since labels on a pod can have unintentional side effects

pedro-r-marques · 2015-10-21T04:30:32Z

@liggitt I've changed the PR so that the deployer pod and hooks use the labels specified in the RC template. This gives the deployer and deployed pods a consistent set of user specified labels which i believe is simpler for the application developer/operator.

liggitt · 2015-10-21T10:09:55Z

Unfortunately, that will also result in unexpected and incorrect behavior. Those labels are used for two things:

Ensuring the pods created by the RC fall under the RC's selector and are managed by the RC. Putting them on the deployment pod could make the deployment pod itself be "managed" by an RC and killed because it exceeds the desired replica count
Including the pods created by the RC in one or more services. Putting them on the deployment pod could make the deployment pod be included in these services, which would result in broken requests to those services, since the deployment pod is not running the images and exposing the ports the RC's pods are

pedro-r-marques · 2015-10-21T15:37:25Z

@liggitt Good point.
I'll rework the patch to introduce a "DeployerTemplate" field to DeploymentConfig and support control over both Labels and Annotations.

liggitt · 2015-10-21T20:48:04Z

Before you do, I'd like @smarterclayton and @ironcladlou to give feedback on that approach. It is a little odd to let the user specify a template which we would only honor the annotations and labels from.

smarterclayton · 2015-10-22T02:47:22Z

A proposal upstream for similar problems is to have specific fields deployerLabels and deployerAnnotations which have no default values and are optional. However, the more metadata we add the more likely we'll want a grouped field. Both hooks and the deployer pod are owned by the strategy, so I would expect them to be as spec.strategy.labels or spec.strategy.deployerLabels or spec.strategy.podLabels. I'm uncertain on the last two vs the first - my worry on the first is it is too generic, but on the other hand it's a more general statement "apply these labels to the strategy".

pedro-r-marques · 2015-10-22T05:09:38Z

@smarterclayton A simpler solution may be to just add labels/annotations to ExecNewPodHook; re-reading the code it seems to me that the deployer pod itself is a controller that is communicating with the kube/openshift API rather than executing code; if my assumption is correct, it is the hook Pod that i need to influence via labels. Still investigating...

smarterclayton · 2015-10-22T05:14:03Z

That's true for some deployer pods, but not custom deployers which might
need network connectivity to the pops.

On Oct 22, 2015, at 1:09 AM, Pedro Marques notifications@github.com wrote:

@smarterclayton https://github.com/smarterclayton A simpler solution may
be to just add labels/annotations to ExecNewPodHook; re-reading the code it
seems to me that the deployer pod itself is a controller that is
communicating with the kube/openshift API rather than executing code; if my
assumption is correct, it is the hook Pod that i need to influence via
labels. Still investigating...

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

pedro-r-marques · 2015-10-22T16:19:43Z

@smarterclayton CustomDeploymentStrategyParams also has image/env/command. It is very similar to ExecNewPodHook (suggestion for future versions of the API would be to make them the same type: e.g. PodTemplate). I would make sense to add labels/annotations to both of these types (imho). It seems to me that it is a non-intrusive addition to the API... both of these types are specifying a pod spec. It is not adding labels/annotations to the pod metadata seems a good fit.

smarterclayton · 2015-10-22T19:28:59Z

Can we think of a scenario under which we want the deployer pod and the
hook pods to have different labels or annotations?

On Thu, Oct 22, 2015 at 12:19 PM, Pedro Marques notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton
CustomDeploymentStrategyParams also has image/env/command. It is very
similar to ExecNewPodHook (suggestion for future versions of the API would
be to make them the same type: e.g. PodTemplate). I would make sense to add
labels/annotations to both of these types (imho). It seems to me that it is
a non-intrusive addition to the API... both of these types are specifying a
pod spec. It is not adding labels/annotations to the pod metadata seems a
good fit.

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

pedro-r-marques · 2015-10-23T01:45:57Z

@smarterclayton My understanding is that the deployer is typically a state machine that talks to the openshift-master in order to control the settings of the RC that control the application pods. If that is the then it wouldn't need tags (labels/annotations) that have semantics... The hook pods typically tend to have the need to for similar characteristics (e.g. network/storage) as the application.
For instance, if the application pod is using secrets to authenticate itself with the database, the hook pod (e.g. "database schema mirgration") would probably need the same secret...

smarterclayton · 2015-10-28T18:49:40Z

Unless we have a very good reason to have different labels/annotations per strategy, I would prefer them on spec.strategy, rather than deeper level.

smarterclayton · 2015-10-30T21:09:01Z

pkg/deploy/util/util.go

@@ -289,3 +289,15 @@ func (d ByLatestVersionDesc) Swap(i, j int) { d[i], d[j] = d[j], d[i] }
 func (d ByLatestVersionDesc) Less(i, j int) bool {
 	return DeploymentVersionFor(&d[j]) < DeploymentVersionFor(&d[i])
 }
+
+// mergeKeyValueMap adds the k,v pairs of the source map to destination map for all keys that are not present in the destination map.
+func MergeKeyValueMap(dst, src map[string]string) {


I think we have a method for this in pkg/util

Thanks! Replaced with util.MergeInto.

smarterclayton · 2015-10-30T21:10:22Z

Just a few comments [test]. You may have to regenerate some of you items.

smarterclayton · 2015-10-30T21:52:02Z

[test]

smarterclayton · 2015-10-31T00:22:44Z

[test]

On Oct 30, 2015, at 6:57 PM, OpenShift Bot notifications@github.com wrote:

continuous-integration/openshift-jenkins/test FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/6506/)

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

liggitt · 2015-10-31T15:14:45Z

pkg/deploy/controller/deployment/factory.go

+func (factory *DeploymentControllerFactory) setPodAttributes(strategy *deployapi.DeploymentStrategy, pod *kapi.Pod) {
+	switch strategy.Type {
+	case deployapi.DeploymentStrategyTypeCustom:
+		util.MergeInto(pod.Labels, strategy.Labels, 0)


In case of conflict, we expect the strategy to override? Doesn't this make it possible to break the deployer pod label lookup the controller uses to reap the pods?

Good catch, this should use the constant and ignore SRC if dest already exists

@liggitt It is not overwriting existing keys (on purpose). So the answer is no; it is not possible to use these labels to break the labels assigned by the deployer controller.

Got it, hadn't checked the MergeInto impl. Add a comment here noting that we're not overwriting existing labels on the pod?

Is there a constant for 0 in this case? If so, we should use it, if not,
we should add it (so the behavior is obvious.

On Oct 31, 2015, at 3:24 PM, Jordan Liggitt notifications@github.com
wrote:

In pkg/deploy/controller/deployment/factory.go
#5149 (comment):

@@ -122,6 +126,15 @@ func (factory *DeploymentControllerFactory) Create() controller.RunnableControll
}
}

+// setPodAttributes updates the deployer pod attributes based on the deployment strategy configuration
+func (factory *DeploymentControllerFactory) setPodAttributes(strategy *deployapi.DeploymentStrategy, pod *kapi.Pod) {

switch strategy.Type {

case deployapi.DeploymentStrategyTypeCustom:

util.MergeInto(pod.Labels, strategy.Labels, 0)

Got it, hadn't checked the MergeInto impl. Add a comment here noting that
we're not overwriting existing labels on the pod?

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

@smarterclayton MergeInfo has several different flags; 0 is the absence of all of them; I'll add the comment.
@ironcladlou The custom strategy is where the user is supplying the deployer; openshift supplies the deployer in the other strategies and thus (to my knowledge) they do not need custom attributes. I'm happy to change it but i do think it does make sense that as you put it "custom labels/annotations apply to the custom deployer pod".

@pedro-r-marques

The custom strategy is where the user is supplying the deployer; openshift supplies the deployer in the other strategies and thus (to my knowledge) they do not need custom attributes. I'm happy to change it but i do think it does make sense that as you put it "custom labels/annotations apply to the custom deployer pod".

In all cases, openshift makes the pod hosting the deployer logic and provides some manner of pod customization (whether it's using our prefab image or yours). We already support a bit of (indirect) control over the deployer pod generally by allowing the node selector of the pod template to be inherited by the deployer pod... If user added labels/annotations can't break the deployer itself (e.g. by overwriting system keys) then I guess I'm not sure why not just make the new configurations apply to deployers generally. It would be easier to explain and implement.

@ironcladlou Updated diff applies labels to all deployers and adds a comment. Can you please ask the test bot to take another go at it ?

I'd like at least one other person (@smarterclayton @liggitt @Kargakis) to validate my premise (that we can apply labels/annotations to all deployer pods). If all agree, you can remove the setPodAttributes abstraction.

I think if we are going to set labels and annotations on spec.strategy,
they should apply to pods created by the strategy (hooks + process) equally.

On Mon, Nov 2, 2015 at 2:25 PM, Dan Mace notifications@github.com wrote:

In pkg/deploy/controller/deployment/factory.go
#5149 (comment):

@@ -122,6 +126,15 @@ func (factory *DeploymentControllerFactory) Create() controller.RunnableControll
}
}

+// setPodAttributes updates the deployer pod attributes based on the deployment strategy configuration
+func (factory *DeploymentControllerFactory) setPodAttributes(strategy *deployapi.DeploymentStrategy, pod *kapi.Pod) {

switch strategy.Type {

case deployapi.DeploymentStrategyTypeCustom:

util.MergeInto(pod.Labels, strategy.Labels, 0)

I'd like at least one other person (@smarterclayton
https://github.com/smarterclayton @liggitt https://github.com/liggitt
@Kargakis https://github.com/kargakis) to validate my premise (that we
can apply labels/annotations to all deployer pods). If all agree, you can
remove the setPodAttributes abstraction.

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

ironcladlou · 2015-11-02T19:10:04Z

Where did all our recent conversation go?

pedro-r-marques · 2015-11-02T19:14:43Z

@ironcladlou you have to expand "commented on an outdated diff"

0xmichalis · 2015-11-03T10:19:43Z

[test]

mfojtik · 2015-11-03T10:23:04Z

pkg/deploy/strategy/support/lifecycle_test.go

+func assert_MapContains(t *testing.T, super, elements map[string]string) {
+	for k, v := range elements {
+		if value, ok := super[k]; ok {
+			if value != v {


You can get rid of the inner if:

if value, ok := super[k]; ok && value != v { }

ironcladlou · 2015-11-04T17:32:59Z

pkg/deploy/controller/deployment/controller_test.go

+		annotations  map[string]string
+		verifyLabels bool
+	}{
+		{deploytest.OkStrategy(), map[string]string{"label1": "value1"}, map[string]string{"annotation1": "value1"}, true},


Sorry, just one more thing here... we're trying to clean up struct initializers for go vet. Can you please use explicit field names, like:

{ strategy: deploytest.OkStrategy(), labels: map[string]string{"label1": "value1"}, annotations: map[string]string{"annotation1": "value1"}, verifyLabels: true },

ironcladlou · 2015-11-04T20:36:16Z

pkg/deploy/controller/deployment/controller_test.go

+		{name: "no overrride", strategy: deploytest.OkStrategy(), labels: map[string]string{deployapi.DeployerPodForDeploymentLabel: "ignored"}, verifyLabels: false},
+	}
+
+	for _, test := range testCases {


Not required, but for future reference it's simpler to just do something like t.Logf("evaluating test %q", test.name) instead of passing the name around everywhere since the tests are sequential (errors for a test will appear right under that test's name)

ironcladlou · 2015-11-04T20:40:49Z

LGTM, thanks for all the work on this!

pedro-r-marques · 2015-11-04T22:23:11Z

@ironcladlou would you mind asking the bot to retest ? thanks.

ironcladlou · 2015-11-04T22:24:05Z

[test]

Labels and annotations may have semantics that are useful when executing a custom deployer pod and/or hooks. For instance the hooks often require access to the services used by the application (example: database schema migration). This commit allows the user to specify the set of labels/annotations to be used on pods used to deploy the application.

pedro-r-marques · 2015-11-05T02:30:56Z

jenkins failure was due to introduction of hook-image-pull secrets. The test was updated with a secret for the expected pod. Please ask the jenkins bot to rerun the test.

sdodson · 2015-11-05T02:39:27Z

[test]

openshift-bot · 2015-11-05T02:40:30Z

Evaluated for origin test up to f2ba544

openshift-bot · 2015-11-05T03:37:09Z

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

pedro-r-marques · 2015-11-16T16:21:17Z

@smarterclayton Can this PR be merged now that the release is done ? thanks.

smarterclayton · 2015-11-16T17:28:53Z

Yes, thanks [merge]

openshift-bot · 2015-11-16T22:05:22Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4023/) (Image: devenv-rhel7_2720)

ironcladlou · 2015-11-16T22:56:12Z

Looks like a weird connectivity issue flaked it out, [merge] again

openshift-bot · 2015-11-16T23:00:27Z

Evaluated for origin merge up to f2ba544

pedro-r-marques · 2015-11-16T23:03:47Z

The jenkins log seems to indicate a script issue.

origin_merge_pull_requests_origin_4021_terminate -s
There was an error talking to AWS. The error message is shown
below:

Error: EC2 Machine is not available
Build step 'Execute a set of scripts' marked build as failure

Merged by openshift-bot

pedro-r-marques changed the title ~~Propagate deploymentConfig labels to deployer and hooks~~ Use ControllerTemplate labels for deployer and hook pods Oct 21, 2015

openshift-bot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 22, 2015

pedro-r-marques changed the title ~~Use ControllerTemplate labels for deployer and hook pods~~ Add labels/annotations to DeploymentStrategy Oct 28, 2015

smarterclayton reviewed Oct 30, 2015
View reviewed changes

liggitt reviewed Oct 31, 2015
View reviewed changes

mfojtik reviewed Nov 3, 2015
View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 4, 2015

ironcladlou reviewed Nov 4, 2015
View reviewed changes

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 4, 2015

ironcladlou reviewed Nov 4, 2015
View reviewed changes

openshift-bot pushed a commit that referenced this pull request Nov 17, 2015

Merge pull request #5149 from pedro-r-marques/deploy-labels

08471b1

Merged by openshift-bot

openshift-bot merged commit 08471b1 into openshift:master Nov 17, 2015

Add labels/annotations to DeploymentStrategy #5149

Add labels/annotations to DeploymentStrategy #5149

Conversation

pedro-r-marques commented Oct 15, 2015

ironcladlou commented Oct 15, 2015

smarterclayton commented Oct 15, 2015

liggitt commented Oct 15, 2015

smarterclayton commented Oct 15, 2015

pedro-r-marques commented Oct 15, 2015

liggitt commented Oct 15, 2015

pedro-r-marques commented Oct 21, 2015

liggitt commented Oct 21, 2015

pedro-r-marques commented Oct 21, 2015

liggitt commented Oct 21, 2015

smarterclayton commented Oct 22, 2015

pedro-r-marques commented Oct 22, 2015

smarterclayton commented Oct 22, 2015

pedro-r-marques commented Oct 22, 2015

smarterclayton commented Oct 22, 2015

pedro-r-marques commented Oct 23, 2015

smarterclayton commented Oct 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Oct 30, 2015

smarterclayton commented Oct 30, 2015

smarterclayton commented Oct 31, 2015

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

ironcladlou commented Nov 2, 2015

pedro-r-marques commented Nov 2, 2015

0xmichalis commented Nov 3, 2015

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

ironcladlou commented Nov 4, 2015

pedro-r-marques commented Nov 4, 2015

ironcladlou commented Nov 4, 2015

pedro-r-marques commented Nov 5, 2015

sdodson commented Nov 5, 2015

openshift-bot commented Nov 5, 2015

openshift-bot commented Nov 5, 2015

pedro-r-marques commented Nov 16, 2015

smarterclayton commented Nov 16, 2015

openshift-bot commented Nov 16, 2015

ironcladlou commented Nov 16, 2015

openshift-bot commented Nov 16, 2015

pedro-r-marques commented Nov 16, 2015