Add support for templating Node.Hostname in docker executor by mion00 · Pull Request #2266 · moby/swarmkit

mion00 · 2017-06-17T08:42:42Z

At the moment we can only use NodeID as a template variable. The hostname is better for some kind of application and it's also more flexible and human readable. See #1951 or moby/moby#32561 for related proposals.

This PR is built on the previous work done by @gianarb

codecov · 2017-06-17T08:57:53Z

Codecov Report

Merging #2266 into master will increase coverage by 0.01%.
The diff coverage is 80.85%.

@@            Coverage Diff             @@
##           master    #2266      +/-   ##
==========================================
+ Coverage   60.35%   60.36%   +0.01%     
==========================================
  Files         125      125              
  Lines       20394    20412      +18     
==========================================
+ Hits        12308    12322      +14     
  Misses       6701     6701              
- Partials     1385     1389       +4

aaronlehmann · 2017-06-19T21:50:15Z

agent/exec/dockerapi/docker_client_stub.go

 }

+// NodeInspectWithRaw is part of the APIClient interface
+func (sa *StubAPIClient) NodeInspectWithRaw(ctx context.Context, nodeID string) (swarm.Node, []byte, error) {


What is this for? I don't see anything that uses it.

It was a leftover from my attempts at using NodeInspect with the Docker client.. I'll remove this change.

aaronlehmann · 2017-06-19T21:57:19Z

agent/exec/dockerapi/executor.go

-	ctlr, err := newController(e.client, t, secrets.Restrict(e.secrets, t))
+	ctx := context.Background()
+	// Get node information
+	nodeDescription, err := e.Describe(ctx)


If I remember correctly, Describe is a relatively expensive operation. I'm not sure we should call Describe for every new controller. It might be better to store the latest nodeDescription returned by Describe in the executor struct, and use that (with any necessary locking).

aaronlehmann · 2017-06-19T22:01:52Z

template/context.go

 	Node struct {
-		ID string
+		ID       string
+		Hostname string


While we're at it, would it make sense to add other properties from NodeDescription, like Architecture and OS?

aaronlehmann · 2017-06-19T22:09:04Z

template/context.go

 // task. The provided context can then be used to populate runtime values in a
 // ContainerSpec.
-func NewContextFromTask(t *api.Task) (ctx Context) {
+func NewContextFromTask(t *api.Task, n *api.NodeDescription) (ctx Context) {


If we make this dependent on NodeDescription, we should rename the function to NewContext.

Another option would be to keep NewContextFromTask as-is, and add a WithNodeDescription method that fills in the node-dependent values. This would keep the node description optional and avoid needing to pass it through code paths that don't need it (for example, tests exercising other parts of the templating functionality). However, I'm not sure there's much value in NodeDescription being optional if we expect all executors to provide it.

@stevvooe WDYT?

aaronlehmann · 2017-06-19T22:10:18Z

template/getter_test.go

 		dependencyManager.Configs().Add(*referencedConfig)

-		templatedDependencies := NewTemplatedDependencyGetter(agent.Restrict(dependencyManager, testCase.task), testCase.task)
+		templatedDependencies := NewTemplatedDependencyGetter(agent.Restrict(dependencyManager, testCase.task), testCase.task, &api.NodeDescription{})


Can you please add test cases that expand Hostname in a template?

GordonTheTurtle · 2017-06-20T11:22:08Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "feature/template-node-hostname" git@github.com:mion00/swarmkit.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842353841144
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

aaronlehmann · 2017-06-20T20:01:35Z

manager/controlapi/service.go

 // - Returns an error if the creation fails.
 func (s *Server) CreateService(ctx context.Context, request *api.CreateServiceRequest) (*api.CreateServiceResponse, error) {
+	// TODO(gianarb) not sure about how to get here the nodeID or something
+	// similar. but the service has already a host at this point?


The service is not associated with any particular nodes when it is first created.

aaronlehmann · 2017-06-20T20:02:05Z

template/context.go

 )

+// Platform holds information about the underlying platform of the node
+type Platform struct {


You can use api.Platform instead of redefining the struct here.

Forget I said that... realized this is going into a struct that will be accessable by templates, so no methods should be defined on the type.

That's exactly what I thought 😉

aaronlehmann · 2017-06-20T20:08:04Z

agent/exec/dockerapi/executor.go

 		genericResources: genericResources,
 	}
+	// Get the node information from the newly created executor
+	var ctx = context.Background()


Using context.Background here is problematic. It meanst NewExecutor can potentially block forever.

I would make Describe set executor.node. Describe is called when the agent first starts up, before anything else happens. It is also called periodically to refresh the information.

We should protect executor.node with a mutex so it isn't accessed during an update.

I added a mutex to the executor field. Sorry but it is my first time using Golang. I hope the code makes sense.

stevvooe · 2017-06-20T20:24:53Z

template/context.go

-		ID string
+		ID       string
+		Hostname string
+		Platform


Why is this embedded?

stevvooe · 2017-06-20T20:25:15Z

template/context.go

+// The provided context can then be used to populate runtime values in a
 // ContainerSpec.
-func NewContextFromTask(t *api.Task) (ctx Context) {
+func NewContext(t *api.Task, n *api.NodeDescription) (ctx Context) {


Argument should be in order of context, so node, then task.

aaronlehmann

Thanks for making the changes. I think this is getting close.

aaronlehmann · 2017-06-21T18:20:44Z

agent/exec/dockerapi/executor.go

 	client           engineapi.APIClient
 	secrets          exec.SecretsManager
 	genericResources []*api.GenericResource
+	mutex            *sync.Mutex // This mutex protects the following node field


The field doesn't have to be a pointer. This can be something like:

mu sync.Mutex

and then it doesn't need to be initialized.

aaronlehmann · 2017-06-21T18:27:32Z

template/context.go

+	ctx.Node.Platform = Platform{
+		Architecture: n.Platform.Architecture,
+		OS:           n.Platform.OS,
+	}


Let's make this tolerate a nil n, just in case Describe failed before storing the node description. In this case, we can just skip filling in the ctx.Node fields.

aaronlehmann · 2017-06-21T20:13:04Z

LGTM

Please squash the commits before merging

…er executor. Add unit testing. Rename NewContextFromTask in NewContext. Populate node description in executor in Describe() method, protected using mutex. Change order of parameters using node first and task second. Check if node is null in template context before populating fields. Signed-off-by: Carlo Mion <cmion@fbk.eu>

stevvooe · 2017-06-22T23:18:46Z

LGTM

- moby/swarmkit#2266 (support for templating Node.Hostname in docker executor) - moby/swarmkit#2281 (change restore action on objects to be update, not delete/create) - moby/swarmkit#2285 (extend watch queue with timeout and size limit) - moby/swarmkit#2253 (version-aware failure tracking in the scheduler) - moby/swarmkit#2275 (update containerd and port executor to container client library) - moby/swarmkit#2292 (rename some generic resources) - moby/swarmkit#2300 (limit the size of the external CA response) - moby/swarmkit#2301 (delete global tasks when the node running them is deleted) Minor cleanups, dependency bumps, and vendoring: - moby/swarmkit#2271 - moby/swarmkit#2279 - moby/swarmkit#2283 - moby/swarmkit#2282 - moby/swarmkit#2274 - moby/swarmkit#2296 (dependency bump of etcd, go-winio) Signed-off-by: Ying Li <ying.li@docker.com> Upstream-commit: 4509a00 Component: engine

mion00 force-pushed the feature/template-node-hostname branch from 3bab7af to 12dfa91 Compare June 17, 2017 08:47

aaronlehmann reviewed Jun 19, 2017

View reviewed changes

GordonTheTurtle added the dco/no label Jun 20, 2017

mion00 force-pushed the feature/template-node-hostname branch from 0684538 to da63b8b Compare June 20, 2017 11:24

GordonTheTurtle removed the dco/no label Jun 20, 2017

mion00 force-pushed the feature/template-node-hostname branch from da63b8b to 064c801 Compare June 20, 2017 11:34

aaronlehmann reviewed Jun 20, 2017

View reviewed changes

stevvooe reviewed Jun 20, 2017

View reviewed changes

template/context.go Outdated

ID string

ID string

Hostname string

Platform

Copy link
Copy Markdown

Contributor

stevvooe Jun 20, 2017

Choose a reason for hiding this comment

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

Why is this embedded?

stevvooe reviewed Jun 20, 2017

View reviewed changes

aaronlehmann reviewed Jun 21, 2017

View reviewed changes

mion00 force-pushed the feature/template-node-hostname branch from 06c398f to 69602b4 Compare June 21, 2017 20:25

aaronlehmann merged commit 9cbc2cc into moby:master Jun 22, 2017

cyli mentioned this pull request Jul 11, 2017

Re-vendor swarmkit. moby/moby#34061

Merged

gianarb mentioned this pull request Aug 2, 2017

[Proposal] docker service create template support for {{ .Node.Hostname }} #1951

Closed

Conversation

mion00 commented Jun 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mion00 Jun 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

GordonTheTurtle commented Jun 20, 2017

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!

aaronlehmann left a comment

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!

aaronlehmann commented Jun 21, 2017

Uh oh!

stevvooe commented Jun 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mion00 commented Jun 17, 2017 •

edited

Loading

codecov bot commented Jun 17, 2017 •

edited

Loading

mion00 Jun 20, 2017 •

edited

Loading