Output network attachment task information #35246

dperny · 2017-10-18T21:49:48Z

- What I did
Network attachment tasks are phony tasks created in swarmkit to deal with unmanaged containers attached to swarmkit. Before this change, attempting docker inspect on the task id of a network attachment task would result in an empty task object. After this change, a full task object is returned

Fixes #26548 the correct way.
Fixes #36498

- How I did it
Adds functionality to parse and return network attachment spec information.

- How to verify it

Create a network like docker network create --driver overlay --attachable testnet
Attach a container like docker run -d --network testnet nginx
Try to delete the network docker network rm testnet. Note that it will return an error because the network is in use, and that error will include a task id.
docker inspect the task id from the previous spec.

Before this change, the output of the inspect will be an empty task.
After this change, the output will be a full task object, including the ID of the container that this task represents.

- Description for the changelog

Running docker inspect on network attachment tasks now returns a full task object

dperny · 2017-10-23T22:50:33Z

Failure are unrelated to the PR.

dperny · 2017-10-24T17:14:05Z

🎉 It's green 🎉

cpuguy83 · 2017-10-25T19:05:58Z

api/types/swarm/task.go

+	// AttachmentSpec will be used when the `Runtime` field is set to `attachment`
+	ContainerSpec         *ContainerSpec         `json:",omitempty"`
+	PluginSpec            *runtime.PluginSpec    `json:",omitempty"`
+	NetworkAttachmentSpec *NetworkAttachmentSpec `json:",omitempty"`


I think we may need to error out on swarm create/update if it includes a NetworkAttachmentSpec?

Should we have a separate type for inspect vs create? I know we're reusing some types at various locations (sometimes resulting in information being shown that happens to be there, but is not needed)

I don't think a separate type is such a good idea. This is the task spec, having the task spec in the inspect output I think is hugely important.

cpuguy83 · 2017-10-25T19:06:32Z

Putting this in design review as it's an API change with a few potential implications.

thaJeztah · 2017-10-26T18:33:57Z

@dperny would it be possible to have the error message show the container id instead of the task if it's a container that's attached to the network?

thaJeztah · 2017-10-26T18:35:58Z

Discussing with @cpuguy83 and @vieux, and moving to code review again

thaJeztah · 2017-10-26T18:36:24Z

Also needs an update to the swagger.yml

dperny · 2017-10-30T18:31:51Z

Does not need an update for creation and deletion, because it will already fail with an appropriate error, see: https://github.com/moby/moby/blob/master/daemon/cluster/convert/service.go#L202-L203

dperny · 2017-10-30T20:15:56Z

@cpuguy83 updated to prevent write operations with network attachment runtime specs. Also fixed some edge cases in runtime type conversion in the process.

thaJeztah · 2017-10-30T20:46:13Z

CI failing on a godoc;

20:38:37 daemon/cluster/convert/service.go:20:2:warning: comment on exported var ErrMismatchedRuntime should be of the form "ErrMismatchedRuntime ..." (golint)

dperny · 2017-11-06T21:34:13Z

rebased.

thaJeztah · 2017-11-07T10:22:03Z

daemon/cluster/convert/service.go

+		}
+	case types.RuntimeNetworkAttachment:
+		// agree to parse this runtime type, but it's not actually valid to
+		// create a service with this runtime type. but that logic is someone


whose responsibility is it? do we check for this situation? (because this looks to be the code you referred to to produce a proper error in #35246 (comment))

ping @dperny ^^

dperny · 2017-11-22T19:44:08Z

Thanks for the reminder @thaJeztah. I've fixed your comment and rebased. Sorry about the poor attention.

thaJeztah

LGTM, but can you also add a note in the API version history; https://github.com/moby/moby/blob/master/docs/api/version-history.md ?

Adds functionality to parse and return network attachment spec information. Network attachment tasks are phony tasks created in swarmkit to deal with unmanaged containers attached to swarmkit. Before this change, attempting `docker inspect` on the task id of a network attachment task would result in an empty task object. After this change, a full task object is returned Fixes moby#26548 the correct way. Signed-off-by: Drew Erny <drew.erny@docker.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah

Saw some other nits; let me rebase this one, and make those changes

thaJeztah · 2018-05-22T21:34:31Z

api/types/swarm/task.go

@@ -56,10 +56,12 @@ type Task struct {

 // TaskSpec represents the spec of a task.
 type TaskSpec struct {
-	// ContainerSpec and PluginSpec are mutually exclusive.
+	// ContainerSpec, AttachmentSpec and PluginSpec are mutually exclusive.


s/AttachmentSpec/NetworkAttachmentSpec/`

thaJeztah · 2018-05-22T21:34:35Z

api/types/swarm/task.go

 	// PluginSpec will only be used when the `Runtime` field is set to `plugin`
-	ContainerSpec *ContainerSpec      `json:",omitempty"`
-	PluginSpec    *runtime.PluginSpec `json:",omitempty"`
+	// AttachmentSpec will be used when the `Runtime` field is set to `attachment`


s/AttachmentSpec/NetworkAttachmentSpec/`

thaJeztah · 2018-05-22T21:35:43Z

api/swagger.yaml

@@ -2683,6 +2683,13 @@ definitions:
              - "default"
              - "process"
              - "hyperv"
+      NetworkAttachmentSpec:
+        description: "Read-only spec type for non-swarm containers attached to swarm overlay networks"


Think we can use a common description for these being mutually exclusive (let me add a commit to address that)

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2018-05-22T21:40:17Z

api/swagger.yaml

+          > **Note**: ContainerSpec, NetworkAttachmentSpec, and PluginSpec are
+          > mutually exclusive. PluginSpec is only used when the Runtime field
+          > is set to `plugin`. NetworkAttachmentSpec is used when the Runtime
+          > field is set to `attachment`.


@dperny I took this description from the type godoc; let me know if you think these look correct

thaJeztah · 2018-05-23T08:00:52Z

Arf... Windows cleanup script before CI started hung

21:40:19 INFO: Nuke-Everything...
21:40:19 INFO: Container count on control daemon to delete is 0
01:00:14 Build timed out (after 200 minutes). Marking the build as failed.

boaz0

Code LGTM

codecov · 2018-05-23T08:44:39Z

Codecov Report

Merging #35246 into master will increase coverage by <.01%.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##           master   #35246      +/-   ##
==========================================
+ Coverage   35.04%   35.05%   +<.01%     
==========================================
  Files         615      615              
  Lines       45813    45829      +16     
==========================================
+ Hits        16057    16066       +9     
- Misses      27646    27651       +5     
- Partials     2110     2112       +2

thaJeztah · 2018-05-23T09:00:51Z

oh... hm, just realised the API version itself wasn't bumped yet 😓 - there's a couple PR's adding new things to the API (#36914, #37043, #36720), so it would be a race which one got merged first 😂 - given that this one's green now, I can also do it in a follow-up

thaJeztah

LGTM'ing because I addressed my earlier comments, but I'm obviously biased now, so need other reviewers 😂

vdemeester

SGTM 🐯
On the API update, we're probably gonna bump the API number on an external PR 👼

cpuguy83

LGTM

GordonTheTurtle added the status/0-triage label Oct 18, 2017

dperny force-pushed the attachment-inspect branch 4 times, most recently from 82d615f to d03df03 Compare October 23, 2017 17:39

yongtang added status/2-code-review rebuild/experimental and removed status/0-triage labels Oct 24, 2017

GordonTheTurtle removed rebuild/experimental labels Oct 24, 2017

cpuguy83 reviewed Oct 25, 2017

View reviewed changes

cpuguy83 added status/1-design-review and removed status/2-code-review labels Oct 25, 2017

thaJeztah added status/2-code-review and removed status/1-design-review labels Oct 26, 2017

dperny force-pushed the attachment-inspect branch from d03df03 to f0710f2 Compare October 30, 2017 20:14

dperny force-pushed the attachment-inspect branch from f0710f2 to 8a9f230 Compare October 30, 2017 20:29

dperny force-pushed the attachment-inspect branch 2 times, most recently from 9f220c7 to bce0158 Compare November 1, 2017 18:10

GordonTheTurtle assigned aluzzardi Nov 2, 2017

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Nov 6, 2017

thaJeztah reviewed Nov 7, 2017

View reviewed changes

dperny force-pushed the attachment-inspect branch 2 times, most recently from 31369fd to 6718b44 Compare November 22, 2017 20:12

thaJeztah added impact/api impact/changelog labels Jan 4, 2018

thaJeztah approved these changes Jan 4, 2018

View reviewed changes

thaJeztah mentioned this pull request Mar 6, 2018

Overlay network is in use by a task that does not exist #36498

Closed

cuericlee mentioned this pull request Mar 8, 2018

Cannot remove network due to task #31068

Open

thaJeztah requested changes May 22, 2018

View reviewed changes

Update swagger and API history

3682703

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the attachment-inspect branch from 6718b44 to 3682703 Compare May 22, 2018 21:39

thaJeztah reviewed May 22, 2018

View reviewed changes

thaJeztah added the rebuild/windowsRS1 label May 23, 2018

GordonTheTurtle removed the rebuild/windowsRS1 label May 23, 2018

boaz0 approved these changes May 23, 2018

View reviewed changes

thaJeztah approved these changes May 23, 2018

View reviewed changes

thaJeztah added area/api area/networking area/swarm labels May 23, 2018

vdemeester approved these changes May 23, 2018

View reviewed changes

cpuguy83 approved these changes May 23, 2018

View reviewed changes

cpuguy83 merged commit 5a68e26 into moby:master May 23, 2018

thaJeztah mentioned this pull request May 24, 2018

Update API version to v1 38 #37141

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Output network attachment task information #35246

Output network attachment task information #35246

dperny commented Oct 18, 2017 •

edited by thaJeztah

Loading

dperny commented Oct 23, 2017

dperny commented Oct 24, 2017

cpuguy83 Oct 25, 2017

thaJeztah Oct 25, 2017

cpuguy83 Oct 26, 2017

cpuguy83 commented Oct 25, 2017

thaJeztah commented Oct 26, 2017

thaJeztah commented Oct 26, 2017

thaJeztah commented Oct 26, 2017

dperny commented Oct 30, 2017

dperny commented Oct 30, 2017

thaJeztah commented Oct 30, 2017

dperny commented Nov 6, 2017

thaJeztah Nov 7, 2017

thaJeztah Nov 22, 2017

dperny commented Nov 22, 2017

thaJeztah left a comment

thaJeztah left a comment

thaJeztah May 22, 2018

thaJeztah May 22, 2018

thaJeztah May 22, 2018

thaJeztah May 22, 2018

thaJeztah commented May 23, 2018

boaz0 left a comment

codecov bot commented May 23, 2018

thaJeztah commented May 23, 2018

thaJeztah left a comment

vdemeester left a comment

cpuguy83 left a comment

Output network attachment task information #35246

Output network attachment task information #35246

Conversation

dperny commented Oct 18, 2017 • edited by thaJeztah Loading

dperny commented Oct 23, 2017

dperny commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpuguy83 commented Oct 25, 2017

thaJeztah commented Oct 26, 2017

thaJeztah commented Oct 26, 2017

thaJeztah commented Oct 26, 2017

dperny commented Oct 30, 2017

dperny commented Oct 30, 2017

thaJeztah commented Oct 30, 2017

dperny commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dperny commented Nov 22, 2017

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah left a 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

thaJeztah commented May 23, 2018

boaz0 left a comment

Choose a reason for hiding this comment

codecov bot commented May 23, 2018

Codecov Report

thaJeztah commented May 23, 2018

thaJeztah left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

cpuguy83 left a comment

Choose a reason for hiding this comment

dperny commented Oct 18, 2017 •

edited by thaJeztah

Loading