Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add --init option to docker service create #479

Merged
merged 1 commit into from
Jun 14, 2018

Conversation

alexkreidler
Copy link

@alexkreidler alexkreidler commented Aug 28, 2017

Helps #51, but requires more work to add stack/compose v3 file support.
See moby/swarmkit#2350, and moby/moby#34529. We've got it all set in swarmkit, this should finish the CLI, and we're waiting on the final pieces in moby.

- What I did
Added a --init option to docker service create
- How I did it
Added an options string (flagInit), the swarm api field in opts.ToService(), and a boolean flag in newCreateCommand()

I also changed a line in the vendor code to add the Init option to the swarm container api. I understand I'm not supposed to do this, but I'll wait till they get a PR into moby/moby, and then do the vendoring properly.

- How to verify it
Run the tests for the service subcommand. Since the daemon doesn't support init for swarm yet, it won't actually do anything.
- Description for the changelog

Add --init option to docker service create

Waiting for the following PR to get merged

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

I gave this a spin against a docker 17.09-rc2 daemon (which should have the SwarmKit changes), but it didn't seem to work;

Creating a service with --init looks to create the right request:

docker service create --init --name initservice nginx:alpine
Sep 20 10:29:30 moby root: time="2017-09-20T10:29:30.328450671Z" level=debug msg="Calling POST /v1.31/services/create"
Sep 20 10:29:30 moby root: time="2017-09-20T10:29:30.328573170Z" level=debug msg="form data: {\"EndpointSpec\":{\"Mode\":\"vip\"},\"Labels\":{},\"Mode\":{\"Replicated\":{}},\"Name\":\"initservice\",\"TaskTemplate\":{\"ContainerSpec\":{\"DNSConfig\":{},\"Image\":\"nginx:alpine@sha256:83f10f82722087e6944e0348b2e64a95baf247135de7c237f4dec7729a386d7f\",\"Init\":true},\"ForceUpdate\":0,\"Placement\":{\"Platforms\":[{\"Architecture\":\"amd64\",\"OS\":\"linux\"}]},\"Resources\":{\"Limits\":{},\"Reservations\":{}}}}"

(formatted for readability): I can see the Init to be passed to the daemon;

{
  "EndpointSpec": {
    "Mode": "vip"
  },
  "Labels": {
    
  },
  "Mode": {
    "Replicated": {
      
    }
  },
  "Name": "initservice",
  "TaskTemplate": {
    "ContainerSpec": {
      "DNSConfig": {
        
      },
      "Image": "nginx:alpine@sha256:83f10f82722087e6944e0348b2e64a95baf247135de7c237f4dec7729a386d7f",
      "Init": true
    },
    "ForceUpdate": 0,
    "Placement": {
      "Platforms": [
        {
          "Architecture": "amd64",
          "OS": "linux"
        }
      ]
    },
    "Resources": {
      "Limits": {
        
      },
      "Reservations": {
        
      }
    }
  }
}

However, I don't see the Init option set for the task backing the container:

docker container inspect  initservice.1.xsbsrewdxj0eq9sg2sa51p3im | grep Init
(no response)

Inspecting the service also does not show this information, which may be either because there's an issue there, or because docker service inspect does not show all information;

[
    {
        "ID": "3kju7dy0b7gz0hl8mf5yy93ub",
        "Version": {
            "Index": 90211
        },
        "CreatedAt": "2017-09-20T10:29:30.329124642Z",
        "UpdatedAt": "2017-09-20T10:29:30.329124642Z",
        "Spec": {
            "Name": "initservice",
            "Labels": {},
            "TaskTemplate": {
                "ContainerSpec": {
                    "Image": "nginx:alpine@sha256:83f10f82722087e6944e0348b2e64a95baf247135de7c237f4dec7729a386d7f",
                    "StopGracePeriod": 10000000000,
                    "DNSConfig": {}
                },
                "Resources": {
                    "Limits": {},
                    "Reservations": {}
                },
                "RestartPolicy": {
                    "Condition": "any",
                    "Delay": 5000000000,
                    "MaxAttempts": 0
                },
                "Placement": {
                    "Platforms": [
                        {
                            "Architecture": "amd64",
                            "OS": "linux"
                        }
                    ]
                },
                "ForceUpdate": 0,
                "Runtime": "container"
            },
            "Mode": {
                "Replicated": {
                    "Replicas": 1
                }
            },
            "UpdateConfig": {
                "Parallelism": 1,
                "FailureAction": "pause",
                "Monitor": 5000000000,
                "MaxFailureRatio": 0,
                "Order": "stop-first"
            },
            "RollbackConfig": {
                "Parallelism": 1,
                "FailureAction": "pause",
                "Monitor": 5000000000,
                "MaxFailureRatio": 0,
                "Order": "stop-first"
            },
            "EndpointSpec": {
                "Mode": "vip"
            }
        },
        "Endpoint": {
            "Spec": {}
        }
    }
]

@@ -57,6 +57,8 @@ func newCreateCommand(dockerCli command.Cli) *cobra.Command {
flags.SetAnnotation(flagDNSSearch, "version", []string{"1.25"})
flags.Var(&opts.hosts, flagHost, "Set one or more custom host-to-IP mappings (host:ip)")
flags.SetAnnotation(flagHost, "version", []string{"1.25"})
flags.BoolVar(&opts.init, flagInit, false, "Use an init inside each service container to forward signals and reap processes")
flags.SetAnnotation(flagInit, "version", []string{"1.30"})
Copy link
Member

Choose a reason for hiding this comment

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

This will probably need to be bumped to 1.33 (1.32 has shipped in a release)

@@ -55,6 +55,7 @@ type ContainerSpec struct {
User string `json:",omitempty"`
Groups []string `json:",omitempty"`
Privileges *Privileges `json:",omitempty"`
Init bool `json:",omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

As commented in moby/moby#34529 (comment) - SwarmKit changes are merged in moby/moby, so can you open a pull request there to make the required changes in moby/moby?

Copy link
Member

Choose a reason for hiding this comment

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

(you, or @denverdino ) 😇

@thaJeztah
Copy link
Member

Actually, nevermind, the daemon API of course doesn't yet put this property forward, so that's expected that it's non functional yet. 😅

@thaJeztah
Copy link
Member

Bumping moby/moby in #679, which should bring in the required changes

@leshik
Copy link

leshik commented Mar 23, 2018

Any chances we'll see this feature in near future?

@codecov-io
Copy link

codecov-io commented May 31, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@722d08f). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master     #479   +/-   ##
=========================================
  Coverage          ?   48.52%           
=========================================
  Files             ?      199           
  Lines             ?    16409           
  Branches          ?        0           
=========================================
  Hits              ?     7962           
  Misses            ?     8033           
  Partials          ?      414

@vdemeester
Copy link
Collaborator

I am carrying this PR, opened upstream PR on moby and swarmkit

@vdemeester
Copy link
Collaborator

btw, design LGTM 👼

@thaJeztah
Copy link
Member

Just so that I don't forget;

  • This option needs to be on docker service update as well
  • We need a test to verify that running docker service update without passing the --init flag preserves the existing value
  • We need a test (probably in moby/moby) to verify that the daemon option (which can set --init so that it's added by default) is used if the --init flag is not passed, on docker service create

@vdemeester vdemeester force-pushed the init-option branch 2 times, most recently from b4d89ba to cbaf7d9 Compare May 31, 2018 14:38
@vdemeester
Copy link
Collaborator

service update is taken care of, with a unit test (to validate the spec update or not)
I'll add a test in moby for the daemon conf 😉

@thaJeztah
Copy link
Member

Oh, last two bullets;

  • Add the new option to the --pretty output for docker service inspect;
    const serviceInspectPrettyTemplate Format = `
    ID: {{.ID}}
    Name: {{.Name}}
    {{- if .Labels }}
    Labels:
    {{- range $k, $v := .Labels }}
    {{ $k }}{{if $v }}={{ $v }}{{ end }}
    {{- end }}{{ end }}
    Service Mode:
    {{- if .IsModeGlobal }} Global
    {{- else if .IsModeReplicated }} Replicated
    {{- if .ModeReplicatedReplicas }}
    Replicas: {{ .ModeReplicatedReplicas }}
    {{- end }}{{ end }}
    {{- if .HasUpdateStatus }}
    UpdateStatus:
    State: {{ .UpdateStatusState }}
    {{- if .HasUpdateStatusStarted }}
    Started: {{ .UpdateStatusStarted }}
    {{- end }}
    {{- if .UpdateIsCompleted }}
    Completed: {{ .UpdateStatusCompleted }}
    {{- end }}
    Message: {{ .UpdateStatusMessage }}
    {{- end }}
    Placement:
    {{- if .TaskPlacementConstraints }}
    Constraints: {{ .TaskPlacementConstraints }}
    {{- end }}
    {{- if .TaskPlacementPreferences }}
    Preferences: {{ .TaskPlacementPreferences }}
    {{- end }}
    {{- if .HasUpdateConfig }}
    UpdateConfig:
    Parallelism: {{ .UpdateParallelism }}
    {{- if .HasUpdateDelay}}
    Delay: {{ .UpdateDelay }}
    {{- end }}
    On failure: {{ .UpdateOnFailure }}
    {{- if .HasUpdateMonitor}}
    Monitoring Period: {{ .UpdateMonitor }}
    {{- end }}
    Max failure ratio: {{ .UpdateMaxFailureRatio }}
    Update order: {{ .UpdateOrder }}
    {{- end }}
    {{- if .HasRollbackConfig }}
    RollbackConfig:
    Parallelism: {{ .RollbackParallelism }}
    {{- if .HasRollbackDelay}}
    Delay: {{ .RollbackDelay }}
    {{- end }}
    On failure: {{ .RollbackOnFailure }}
    {{- if .HasRollbackMonitor}}
    Monitoring Period: {{ .RollbackMonitor }}
    {{- end }}
    Max failure ratio: {{ .RollbackMaxFailureRatio }}
    Rollback order: {{ .RollbackOrder }}
    {{- end }}
    ContainerSpec:
    Image: {{ .ContainerImage }}
    {{- if .ContainerArgs }}
    Args: {{ range $arg := .ContainerArgs }}{{ $arg }} {{ end }}
    {{- end -}}
    {{- if .ContainerEnv }}
    Env: {{ range $env := .ContainerEnv }}{{ $env }} {{ end }}
    {{- end -}}
    {{- if .ContainerWorkDir }}
    Dir: {{ .ContainerWorkDir }}
    {{- end -}}
    {{- if .ContainerUser }}
    User: {{ .ContainerUser }}
    {{- end }}
    {{- if .ContainerMounts }}
    Mounts:
    {{- end }}
    {{- range $mount := .ContainerMounts }}
    Target: {{ $mount.Target }}
    Source: {{ $mount.Source }}
    ReadOnly: {{ $mount.ReadOnly }}
    Type: {{ $mount.Type }}
    {{- end -}}
    {{- if .Configs}}
    Configs:
    {{- range $config := .Configs }}
    Target: {{$config.File.Name}}
    Source: {{$config.ConfigName}}
    {{- end }}{{ end }}
    {{- if .Secrets }}
    Secrets:
    {{- range $secret := .Secrets }}
    Target: {{$secret.File.Name}}
    Source: {{$secret.SecretName}}
    {{- end }}{{ end }}
    {{- if .HasResources }}
    Resources:
    {{- if .HasResourceReservations }}
    Reservations:
    {{- if gt .ResourceReservationNanoCPUs 0.0 }}
    CPU: {{ .ResourceReservationNanoCPUs }}
    {{- end }}
    {{- if .ResourceReservationMemory }}
    Memory: {{ .ResourceReservationMemory }}
    {{- end }}{{ end }}
    {{- if .HasResourceLimits }}
    Limits:
    {{- if gt .ResourceLimitsNanoCPUs 0.0 }}
    CPU: {{ .ResourceLimitsNanoCPUs }}
    {{- end }}
    {{- if .ResourceLimitMemory }}
    Memory: {{ .ResourceLimitMemory }}
    {{- end }}{{ end }}{{ end }}
    {{- if .Networks }}
    Networks:
    {{- range $network := .Networks }} {{ $network }}{{ end }} {{ end }}
    Endpoint Mode: {{ .EndpointMode }}
    {{- if .Ports }}
    Ports:
    {{- range $port := .Ports }}
    PublishedPort = {{ $port.PublishedPort }}
    Protocol = {{ $port.Protocol }}
    TargetPort = {{ $port.TargetPort }}
    PublishMode = {{ $port.PublishMode }}
    {{- end }} {{ end -}}
    `
  • Updates to the docs 😅

}
cspec := spec.TaskTemplate.ContainerSpec

// Update with --stop-signal=SIGUSR1
Copy link
Member

Choose a reason for hiding this comment

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

s/--stop-signal=SIGUSR1/--init=true/

updateService(nil, nil, flags, spec)
assert.Check(t, is.Equal(true, cspec.Init))

// Update without --stop-signal, no change
Copy link
Member

Choose a reason for hiding this comment

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

s/--stop-signal/--init/

updateService(nil, nil, flags, spec)
assert.Check(t, is.Equal(true, cspec.Init))

// Update with --stop-signal=SIGWINCH
Copy link
Member

Choose a reason for hiding this comment

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

s/--stop-signal= SIGWINCH/--init=false/

@silvin-lubecki
Copy link
Contributor

Validate is complaining:

These files were changed:
 M vendor/github.com/docker/docker/api/types/swarm/container.go

@vdemeester
Copy link
Collaborator

Add the new option to the --pretty output for docker service inspect;

@thaJeztah actually there is quite some field from the ContainerSpec that are not showed in the --pretty (like TTY, …). I wonder if we should add all of them too…

@thaJeztah
Copy link
Member

We should probably add more of those yes; but it's a good start to make sure that new fields are not missed

Moby vendoring was updated in another PR, so looks like we can continue on this one 👍

@vdemeester vdemeester changed the title [WIP] Add --init option to docker service create Add --init option to docker service create Jun 11, 2018
Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 😝

@silvin-lubecki
Copy link
Contributor

test is complaining 😂

--- FAIL: TestPrettyPrintWithNoUpdateConfig (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xbaceb4]

goroutine 8 [running]:
testing.tRunner.func1(0xc4203fc1e0)
	/usr/local/go/src/testing/testing.go:742 +0x29d
panic(0xd2d540, 0x13dd220)
	/usr/local/go/src/runtime/panic.go:502 +0x229
text/template.errRecover(0xc420175490)
	/usr/local/go/src/text/template/exec.go:137 +0x1d4
panic(0xd2d540, 0x13dd220)
	/usr/local/go/src/runtime/panic.go:502 +0x229
github.com/docker/cli/cli/command/formatter.(*serviceInspectContext).ContainerInit(0xc42008fa00, 0x0)
	/go/src/github.com/docker/cli/cli/command/formatter/service.go:381 +0x14
...

--container-label-add list Add or update a container label
--container-label-rm list Remove a container label by its key
--credential-spec credential-spec Credential spec for managed service account (Windows only)
--args command Service command args
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this whole file changed from using spaces to tabs; can you revert that change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

😱

@silvin-lubecki
Copy link
Contributor

A test is failing:

--- FAIL: TestUpdateInit (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
    panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xc36156]

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM after the test is fixed

Signed-off-by: Timothy Higinbottom <[email protected]>
Signed-off-by: Vincent Demeester <[email protected]>
@thaJeztah
Copy link
Member

Follow up for the compose-file format; #1129

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants