Allow specifying environment variables when installing an engine plugin as a Swarm service by sirlatrom · Pull Request #38441 · moby/moby

sirlatrom · 2018-12-27T17:15:15Z

- What I did
I enabled specifying environment variables when installing a plugin through a Swarm service.

- How I did it

I added an Env []string field to the PluginSpec type by modifying its protobuf file, allowing environment variables (and values) to be specified when a plugin is to be installed as a Swarm service.
I added logic to transfer the environment variables to the resulting plugin task.

- How to verify it
a) Specify Env: []string{"MY_ENV_VAR=my-env-value"} in the TaskTemplate.PluginSpec object of the swarm.ServiceSpec when calling the service/create endpoint.
Example:

...
Env: []string{
    "policy-template={{ .ServiceName }},{{ .TaskImage }},{{ ServiceLabel \"com.docker.ucp.access.label\" }}",
...
},

b) The test TestServicePlugin in integration/service/plugin_test.go now covers the changed code.

- Description for the changelog

Allow specifying environment variables when installing an engine plugin as a Swarm service.

- A picture of a cute animal (not mandatory but encouraged)

codecov · 2018-12-27T19:59:45Z

Codecov Report

Merging #38441 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #38441      +/-   ##
==========================================
- Coverage   36.89%   36.87%   -0.03%     
==========================================
  Files         613      613              
  Lines       45421    45435      +14     
==========================================
- Hits        16760    16754       -6     
- Misses      26370    26389      +19     
- Partials     2291     2292       +1

plugin/defs.go

sirlatrom · 2019-01-09T14:10:38Z

@thaJeztah I've incorporated your suggested change. I could not find any existing tests covering the WithSwarmService adjacent to my WithEnv function.

cpuguy83 · 2019-01-09T18:14:25Z

Can you add a test for this (or modify the swarm plugin create test?)

sirlatrom · 2019-01-09T20:11:42Z

@cpuguy Sure thing, thanks for the pointer, I'll see if I can add it to the right place in that test.

sirlatrom · 2019-01-10T16:42:57Z

@cpuguy83 Currently there's no way for the Docker engine to tell what tasks exist for a plugin Swarm service, since they are in a different namespace. Please advise on how to continue writing a test in integration/service/plugin_test.go when the goal should be to verify the task/container for the plugin has the correct environment variables set.

cpuguy83 · 2019-01-10T17:18:44Z

@sirlatrom You should be able to query the tasks API with the service ID as the filter.

sirlatrom · 2019-01-11T09:53:51Z

@cpuguy83 That works for regular Docker tasks, but the /tasks endpoint only returns tasks that have a ContainerSpec. So I can inspect the plugin service, since that endpoint does not hide plugin services, but I cannot get its tasks, because the tasks endpoint hides those.

sirlatrom · 2019-01-14T14:39:06Z

More specifically, it is this bit of code that restricts the runtime of the returned tasks:

moby/daemon/cluster/tasks.go

Lines 42 to 46 in 489b8ed

    
           if !filter.Contains("runtime") { 
        
           	// default to only showing container tasks 
        
           	filter.Add("runtime", "container") 
        
           	filter.Add("runtime", "") 
        
           }

I'll extend the test helper for task listing to allow additional filters, including the "runtime" filter.

sirlatrom · 2019-01-14T22:02:57Z

Flaky test is flaky. DockerHubPullSuite.TestPullFromCentralRegistry passed in experimental, powerpc and z, but failed on janky. DockerSuite.TestRunStoppedLoggingDriverNoLeak failed on windowsRS1.

sirlatrom · 2019-01-15T13:35:51Z

I force pushed to get checks to pass, but windowsRS1 still fails. @cpuguy83 Is that test unrelated? I feel like it is.

olljanat · 2019-01-15T17:23:24Z

(reserved for my derek commands)

plugin/defs.go

sirlatrom · 2019-03-27T11:09:18Z

Rebased on moby/moby master.

cpuguy83

LGTM

sirlatrom · 2019-03-27T23:08:04Z

Looks like experimental build fails because of this CRIU test, which I hope is unrelated:

Running /go/src/github.com/docker/docker/integration/container
INFO: Testing against a local daemon
=== RUN   TestCheckpoint
--- FAIL: TestCheckpoint (1.55s)
    checkpoint_test.go:40: Error (criu/util.c:816): exited, status=3
        Warn  (criu/net.c:2840): Unable to get socket network namespace
        Warn  (criu/net.c:2840): Unable to get tun network namespace
        Warn  (criu/sk-unix.c:229): unix: Unable to open a socket file: Bad address
        Warn  (criu/net.c:2840): Unable to get socket network namespace
        Warn  (criu/kerndat.c:881): Can't keep kdat cache on non-tempfs
        Looks good.
    checkpoint_test.go:51: Start a container
    checkpoint_test.go:69: ++ type -P true
        ++ type -P ip6tables-restore
        + mount --bind /bin/true /sbin/ip6tables-restore
        ++ type -P true
        ++ type -P ip6tables-save
        + mount --bind /bin/true /sbin/ip6tables-save
    checkpoint_test.go:81: Do a checkpoint and leave the container running
    checkpoint_test.go:24: Exec: [touch /tmp/test-file]
    checkpoint_test.go:28: 
    checkpoint_test.go:115: Do a checkpoint and stop the container
    checkpoint_test.go:117: assertion failed: error is not nil: Error response from daemon: Cannot checkpoint container 1239a021c72845fc2ed944e75b6e41bf691bb2542b7f329aa9e09a76976cbf13: cannot checkpoint a stopped container: unknown
    checkpoint_test.go:77: ++ type -P ip6tables-restore
        + umount -c -i -l /sbin/ip6tables-restore
        ++ type -P ip6tables-save
        + umount -c -i -l /sbin/ip6tables-save
    main_test.go:32: assertion failed: error is not nil: Error response from daemon: Container 1239a021c72845fc2ed944e75b6e41bf691bb2542b7f329aa9e09a76976cbf13 is not paused: failed to unpause container 1239a021c72845fc2ed944e75b6e41bf691bb2542b7f329aa9e09a76976cbf13

The tests failing for z are TestCreateWithDuplicateNetworkNames and TestDockerNetworkConnectAlias, which I would also argue are unrelated.

I don't even know why the Windows tests are failing, but I'm hoping it's also unrelated.

Allow specifying environment variables when installing an engine plugin as a Swarm service. Invalid environment variable entries (without an equals (`=`) char) will be ignored. Signed-off-by: Sune Keller <[email protected]>

sirlatrom · 2019-04-12T16:32:16Z

@thaJeztah Any chance the "experimental" build could be retried?

thaJeztah · 2019-04-12T19:27:18Z

Restarted 👍

sirlatrom · 2019-04-13T10:43:28Z

Thanks @thaJeztah, looks like it passed the flaky tests this time 😅

Any chance the appropriate maintainers will give this their final judgement?

crosbymichael · 2019-07-08T19:26:44Z

LGTM

sirlatrom requested a review from cpuguy83 as a code owner December 27, 2018 17:15

GordonTheTurtle added the status/0-triage label Dec 27, 2018

sirlatrom force-pushed the swarm_plugin_env branch 3 times, most recently from b3df338 to 37c3828 Compare December 27, 2018 17:35

sirlatrom changed the title ~~WIP: Swarm plugin environment variables passing~~ WIP: Allow specifying environment variables when installing an engine plugin as a Swarm service Dec 27, 2018

thaJeztah added the area/plugins label Jan 9, 2019

thaJeztah reviewed Jan 9, 2019

View reviewed changes

plugin/defs.go Outdated Show resolved Hide resolved

sirlatrom force-pushed the swarm_plugin_env branch from ae17882 to cf4c4df Compare January 9, 2019 14:05

sirlatrom changed the title ~~WIP: Allow specifying environment variables when installing an engine plugin as a Swarm service~~ Allow specifying environment variables when installing an engine plugin as a Swarm service Jan 9, 2019

sirlatrom requested a review from vdemeester as a code owner January 9, 2019 23:32

GordonTheTurtle assigned crosbymichael Jan 11, 2019

sirlatrom force-pushed the swarm_plugin_env branch 3 times, most recently from e16a0de to 11fb7d1 Compare January 14, 2019 16:48

sirlatrom force-pushed the swarm_plugin_env branch from 11fb7d1 to 7f6a6d3 Compare January 15, 2019 09:48

sirlatrom force-pushed the swarm_plugin_env branch from 7f6a6d3 to ab32aec Compare January 15, 2019 14:36

derek bot added the rebuild/z label Jan 15, 2019

GordonTheTurtle removed the rebuild/z label Jan 15, 2019

cpuguy83 requested changes Jan 15, 2019

View reviewed changes

plugin/defs.go Outdated Show resolved Hide resolved

sirlatrom force-pushed the swarm_plugin_env branch 2 times, most recently from 6c8266f to 8908c31 Compare January 15, 2019 19:52

sirlatrom force-pushed the swarm_plugin_env branch 2 times, most recently from 399a198 to 7ac049a Compare March 27, 2019 11:09

cpuguy83 approved these changes Mar 27, 2019

View reviewed changes

cpuguy83 added status/2-code-review rebuild/experimental and removed status/0-triage labels Mar 27, 2019

GordonTheTurtle removed the rebuild/experimental label Mar 27, 2019

thaJeztah mentioned this pull request Mar 28, 2019

Flaky test: TestCheckpoint #38963

Open

sirlatrom force-pushed the swarm_plugin_env branch 4 times, most recently from 9f37ce8 to 32bf388 Compare April 5, 2019 09:59

Support environment vars in Swarm plugins services

fca5ee3

Allow specifying environment variables when installing an engine plugin as a Swarm service. Invalid environment variable entries (without an equals (`=`) char) will be ignored. Signed-off-by: Sune Keller <[email protected]>

sirlatrom force-pushed the swarm_plugin_env branch from 32bf388 to fca5ee3 Compare April 7, 2019 08:08

thaJeztah removed rebuild/windowsRS5-process labels Apr 13, 2019

crosbymichael merged commit fb459f6 into moby:master Jul 8, 2019

sirlatrom deleted the swarm_plugin_env branch July 9, 2019 19:17

thaJeztah added this to the 20.03.0 milestone Apr 2, 2020

Conversation

sirlatrom commented Dec 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

sirlatrom commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 commented Jan 9, 2019

Uh oh!

sirlatrom commented Jan 9, 2019

Uh oh!

sirlatrom commented Jan 10, 2019

Uh oh!

cpuguy83 commented Jan 10, 2019

Uh oh!

sirlatrom commented Jan 11, 2019

Uh oh!

sirlatrom commented Jan 14, 2019

Uh oh!

sirlatrom commented Jan 14, 2019

Uh oh!

sirlatrom commented Jan 15, 2019

Uh oh!

olljanat commented Jan 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sirlatrom commented Mar 27, 2019

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

sirlatrom commented Mar 27, 2019

Uh oh!

sirlatrom commented Apr 12, 2019

Uh oh!

thaJeztah commented Apr 12, 2019

Uh oh!

sirlatrom commented Apr 13, 2019

Uh oh!

crosbymichael commented Jul 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sirlatrom commented Dec 27, 2018 •

edited

Loading

codecov bot commented Dec 27, 2018 •

edited

Loading

sirlatrom commented Jan 9, 2019 •

edited

Loading

olljanat commented Jan 15, 2019 •

edited

Loading