template: support templating of task runtime parameters by stevvooe · Pull Request #1650 · moby/swarmkit

stevvooe · 2016-10-14T23:48:06Z

To better support common use cases that require injected service and
task information, we now treat ContainerSpec fields as templates. A
common, simplified context is defined that contains commonly needed
values, with access to names, identifiers and labels.

This initial PR focuses on supporting mounts and environment variables.
By supporting these, users will be able to locate mounts based on task
context or service name, unlocking many use cases that were previously
challenging or impossible. Environment variables open up further
possibilities.

Signed-off-by: Stephen J Day [email protected]

cc @aluzzardi @mgoelzer @ehazlett @NathanMcCauley @diogomonica

aaronlehmann · 2016-10-14T23:51:08Z

This is pretty cool.

aaronlehmann · 2016-10-15T01:56:15Z

There are some golint issues around copying a structure with a lock inside it.

I don't have a strong opinion about the template cache. I would think that templates are relatively cheap to parse (sort of like a printf format string), and if we didn't cache them, we would only need to reparse when handling a new task. This is not exactly something we'd be doing in an inner loop, so the caching probably doesn't buy us much. But maybe template parsing has more overhead than I naively expect.

aluzzardi · 2016-10-15T03:32:00Z

I don't have a strong opinion about the template cache

Yeah, template compilation is dirt cheap compared to prepping a container

stevvooe · 2016-10-17T16:51:11Z

I'll remove the template cache. I was breaking my own rules about premature optimization and felt dirty while doing it. If we prove a bottleneck here, we'll take care of it later. The actual answer might be to "de-feature" the template system, since complex templating isn't really needed.

diogomonica · 2016-10-17T17:49:00Z

Looks pretty cool. Will this bear any relation with config management?

stevvooe · 2016-10-17T18:02:41Z

@diogomonica This is the engine for configuration management:

Imagine a new mount type that carries a file that can be evaluated as a template.
Imagine having a secret template function to access to secrets within a template
Imagine wiring this context to the evaluated resources to have the template re-expanded on resource change
Imagine hooking in new sources for populating template variables

tlvenn · 2016-10-24T10:31:49Z

That is awesome @stevvooe !

My usecase would be to create a dir mounted as tmpfs and create a file in it containing credentials pulled from Vault that my service can access transparently.

Can't wait for this to land.

diogomonica · 2016-10-24T18:09:21Z

@tlvenn for this release all the secrets will be stored on the swarm managers, and will have to be added/removed/managed directly from the docker secret command. Later on we would love to support adding externally managed secrets that are imported from external secret stores and distributed throughout all the nodes.

dnephin · 2016-10-24T18:40:28Z

This looks great.

What do you think about supporting ContainerSpec.Name and ContainerSpec.Hostname ?

stevvooe · 2016-10-24T20:50:35Z

@dnephin There is no ContainerSpec.Name field. We'd have to integrate Annotations and work to fix the backwards compatibility issues.

ContainerSpec.Hostname is not yet merged, but I think we can go ahead and merge it and add it without issue. The main problem with it, as I found out the other day, is that Hostname isn't just the hostname. It is parsed into both Hostname and DomainName, so we have to honor that model.

thaJeztah · 2016-10-24T21:19:05Z

Based on all the "ooooh" and "ahhhh" above, shall I move this to "code review"?

aluzzardi · 2016-10-24T22:49:32Z

LGTM

Please rebase

stevvooe · 2016-10-25T00:57:39Z

Added support for parameterizing hostname.

codecov-io · 2016-10-25T01:33:14Z

Current coverage is 55.89% (diff: 54.34%)

Merging #1650 into master will decrease coverage by 0.08%

@@             master      #1650   diff @@
==========================================
  Files            93         96     +3   
  Lines         14796      14886    +90   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           8283       8320    +37   
- Misses         5425       5466    +41   
- Partials       1088       1100    +12

Powered by Codecov. Last update 328d703...21cdd63

tlvenn · 2016-10-25T01:50:04Z

@diogomonica Can you link the Issue/PR/Proposal if any that introduce docker secret command ? I would love to learn more about it. Thanks !

stevvooe · 2016-10-25T02:16:07Z

A few more changes:

removed support for Task.Labels. These aren't really of interest to the user. They can set container labels manually or template with task labels. We may allow templating of container labels in the future and this limitation preserves that choice.
Added some pedantic units tests ensuring that labels from the task.annotations don't pollute the templates.

thaJeztah · 2016-10-25T04:42:50Z

ping @aaronlehmann @diogomonica PTAL, let's see if we can get this into 1.13 ❤️

aaronlehmann · 2016-10-25T05:33:19Z

template/context.go

+	ctx.Node.ID = t.NodeID
+
+	ctx.Task.ID = t.ID
+	ctx.Task.Name = fmt.Sprintf("%s.%d.%s", t.ServiceAnnotations.Name, t.Slot, t.ID)


I know it probably sounds like I'm trolling, but I remember you felt very strongly that task name derivation should not be duplicated throughout the code, and this seems to add another copy of it.

I think in the current code, t.Annotations.Name stores a precomputed task name? Although @aluzzardi said he wanted to revert this by 1.13.

Do as I say, not as I do? 🐹

We still need to centralize the naming. Is this okay for now or do want me to introduce the naming package?

A function in api/naming would do

@aluzzardi Seems outside the scope of this PR. I've asked for this in 6 other PRs and I'm not gating this feature on that addition if no one else is willing to.

Naming package: #1694.

aaronlehmann · 2016-10-25T05:36:17Z

template/expand.go

+func ExpandContainerSpec(t *api.Task) (*api.ContainerSpec, error) {
+	container := t.Spec.GetContainer()
+	if container == nil {
+		return nil, nil


Should this return an error?

For now, yes.

aaronlehmann · 2016-10-25T05:38:10Z

template/expand.go

+				return values, err
+			}
+
+			entry = fmt.Sprintf("%s=%s", entry, expanded)


Personal preference - feel free to ignore - but I find entry + "=" + expanded easier to read and I like the type safety benefits.

Doesn't that require two intermediates?

aaronlehmann · 2016-10-25T05:38:57Z

template/expand.go

+		}
+
+		n[k] = v
+


nit: blank line looks out of place

aaronlehmann · 2016-10-25T05:43:00Z

template/expand.go

+		return container, err
+	}
+
+	container.Hostname, err = ctx.Expand(container.Hostname)


In this and all the other calls to ctx.Expand, it seems like a good idea to wrap the error with an indication of which field caused the error.

aluzzardi · 2016-10-31T21:54:39Z

LGTM

Needs rebase

To better support common use cases that require injected service and task information, we now treat ContainerSpec fields as templates. A common, simplified context is defined that contains commonly needed values, with access to names, identifiers and labels. This initial PR focuses on supporting mounts, hostname and environment variables. By supporting these, users will be able to locate mounts based on task context or service name, unlocking many use cases that were previously challenging or impossible. Environment variables open up further possibilities. Hostname is mostly harmless but useful in certain applications. Signed-off-by: Stephen J Day <[email protected]>

GordonTheTurtle added the status/0-triage label Oct 14, 2016

justincormack mentioned this pull request Oct 15, 2016

Named volumes don't work with services moby/moby#27379

Closed

stevvooe force-pushed the template-expansion branch from 8af1afe to fcc7968 Compare October 17, 2016 16:52

This was referenced Oct 19, 2016

[EXPERIMENTAL] new mount type: introspection moby/moby#26331

Closed

Added option hostname to docker service create #1318

Closed

ghost mentioned this pull request Oct 21, 2016

Pass swarm task information as environment variables into each task moby/moby#24110

Closed

thaJeztah added status/2-code-review and removed status/0-triage labels Oct 24, 2016

stevvooe force-pushed the template-expansion branch 2 times, most recently from ed66a05 to 7e18ee1 Compare October 25, 2016 00:55

stevvooe force-pushed the template-expansion branch 2 times, most recently from 34a8aad to c70ff24 Compare October 25, 2016 01:19

stevvooe force-pushed the template-expansion branch from c70ff24 to 5ccd351 Compare October 25, 2016 02:14

aaronlehmann reviewed Oct 25, 2016

View reviewed changes

template/expand.go Outdated

}

n[k] = v

Copy link

Collaborator

aaronlehmann Oct 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: blank line looks out of place

aaronlehmann reviewed Oct 25, 2016

View reviewed changes

stevvooe force-pushed the template-expansion branch 8 times, most recently from a9ac2ae to 2b15064 Compare October 27, 2016 22:19

stevvooe force-pushed the template-expansion branch from 2b15064 to 21cdd63 Compare October 31, 2016 22:00

aaronlehmann merged commit b693657 into moby:master Nov 1, 2016

stevvooe deleted the template-expansion branch November 1, 2016 01:37

AkihiroSuda mentioned this pull request Nov 10, 2016

[doc] update description about the task slot #1741

Merged

Comments

Conversation

stevvooe commented Oct 14, 2016

Uh oh!

aaronlehmann commented Oct 14, 2016

Uh oh!

aaronlehmann commented Oct 15, 2016

Uh oh!

aluzzardi commented Oct 15, 2016

Uh oh!

stevvooe commented Oct 17, 2016

Uh oh!

diogomonica commented Oct 17, 2016

Uh oh!

stevvooe commented Oct 17, 2016

Uh oh!

tlvenn commented Oct 24, 2016

Uh oh!

diogomonica commented Oct 24, 2016

Uh oh!

dnephin commented Oct 24, 2016

Uh oh!

stevvooe commented Oct 24, 2016

Uh oh!

thaJeztah commented Oct 24, 2016

Uh oh!

aluzzardi commented Oct 24, 2016

Uh oh!

stevvooe commented Oct 25, 2016

Uh oh!

codecov-io commented Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 55.89% (diff: 54.34%)

Uh oh!

tlvenn commented Oct 25, 2016

Uh oh!

stevvooe commented Oct 25, 2016

Uh oh!

thaJeztah commented Oct 25, 2016

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!

aluzzardi commented Oct 31, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

codecov-io commented Oct 25, 2016 •

edited

Loading