Support the init for container spec

[denverdino]

Add the optional init flag in container spec for init feature since Docker v1.13

Signed-off-by: Li Yi [email protected]

[GordonTheTurtle]

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 "init-support" [email protected]:AliyunContainerService/swarmkit.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

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

[codecov]

Codecov Report

Merging #2350 into master will increase coverage by 0.12%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2350      +/-   ##
==========================================
+ Coverage   59.96%   60.08%   +0.12%     
==========================================
  Files         128      128              
  Lines       26183    26185       +2     
==========================================
+ Hits        15701    15734      +33     
+ Misses       9092     9061      -31     
  Partials     1390     1390

[nishanttotla]

@denverdino how will this be used by Docker?

[denverdino]

Still need some change int the docker/cli and moby/moby, the user interface should be in the

docker service create --init ...

It is similar to docker run --init

Thnaks

[nishanttotla]

Sounds like a reasonable feature to me, and it contributes to parity with the Docker API. It would be useful to add a tracker issue for this in moby/moby so that we don't miss out on different parts of the implementation @denverdino

cc @aluzzardi @thaJeztah WDYT?

[denverdino]

Sure, will do. And this PR is related to #2173

[aaronlehmann]

Add a space between init and {`.

[denverdino]

Fixed and thanks

[thaJeztah]

Sounds like a reasonable feature to me, and it contributes to parity with the Docker API.

Yes, I have no problem adding this for services 👍

It would be useful to add a tracker issue for this in moby/moby so that we don't miss out on different parts of the implementation

Yes; I linked this PR already to the "epic", moby/moby#25303, but it's best to have an issue in moby/moby tracking it as well

[nishanttotla]

LGTM

@denverdino can you squash your two commits into one, and open a tracking issue in moby/moby and link back to this PR? We can merge this PR then.

[denverdino]

Thanks, the issue is opened in moby/moby#34529, and commits are rebased and squashed.

[stevvooe]

Why is this field a oneof?

[nishanttotla]

@denverdino can you please reply to @stevvooe's concern above? In particular, there should be a comment about what the field does, as well as why this can't be a regular boolean. Since this is a feature in progress, we should make these changes sooner rather than later.

cc @thaJeztah

[denverdino]

@stevvooe,

From https://github.com/moby/moby/blob/master/api/types/container/host_config.go
// Run a custom init inside the container, if null, use the daemon's configured settings
Init *bool json:",omitempty"

So, we need to tell if the int is set or not. That is why we need oneof. Your thought?

Thanks

[stevvooe]

@denverdino That is not the correct use of oneof. A oneof is used to implement option types. If you need a pointer to a concrete type, you can use BoolValue from the well known types.

To tell you the trust, this doesn't need to be a pointer. The default of false is sufficient here, since true on daemon or config overrides the value. Just use a bool on this.

[denverdino]

@stevvooe my understanding is if the daemon flag is set, the default value will be the changed.

Then if the init flag is not set in the container/service, it use the daemon's configured settings

E.g. when the init flag in daemon is true, if the init flag of container/service is null, the real value will be true. if the init flag of container/service is false, the real value will be false.

Your thought?

[stevvooe]

@denverdino This is an incorrect use of oneof. Please remove this field and resubmit the PR.

[thaJeztah]

@stevvooe this was vendored already in moby, and (I think) part of the 17.09 release/rc's is that a problem? (the option itself is not exposed yet in the Docker API)

[stevvooe]

@thaJeztah This needs to be removed immediately. This is not the correct use of oneof and we'll have to remove it.

If our focus is on quality, we need to commit to it and ensure stuff like this doesn't slip through review. I brought this up over two weeks ago. If this can't be done by a current contributor, I'll submit a PR to revert this commit so we can avoid releasing with a broken use of oneof.

[thaJeztah]

I think that's the best short term solution

ping @andrewhsu FYI ^^

[thaJeztah]

A PR to revert the PR for now was opened here: #2384 #2383, we can work on re-adding this feature after that (as the feature is definitely useful)

[denverdino]

@stevvooe @thaJeztah That is fine, I adjust that with new PR #2386, pls take a look.