Skip to content
This repository has been archived by the owner on Dec 13, 2021. It is now read-only.

add support for both attach mode and detach mode #158

Merged
merged 2 commits into from
Jun 27, 2016
Merged

add support for both attach mode and detach mode #158

merged 2 commits into from
Jun 27, 2016

Conversation

v01dstar
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Jun 24, 2016

Coverage Status

Changes Unknown when pulling d445e72 on v01dstar:detach-mode-support into * on osrg:master*.

@v01dstar v01dstar closed this Jun 24, 2016
@AkihiroSuda
Copy link
Member

@v01dstar

Thank you a lot for working on this 😃 .

I confirmed -p and -privileged are working perfectly with both proc inspector and ethernet inspector 😆 .
I didn't do test -net and -volumes-from, but basically they LGTM.

However I could not get about -d. It seems just waiting for a container and does not seem doing "detach".

if detach {
    err = client.StartContainer(container.ID, opt.HostConfig)
    if err == nil {
        _, err = client.WaitContainer(container.ID)
    }
    exitCh <- err
}

Actually I tried nmz container run -p 80 -d nginx and nmz container run -itd busybox sh, but it did not work for me.
(Please compare the result of docker run)

Could you please look into this?

If you can split -d into other PR, I think we can merge this PR immediately.

Thank you again

@v01dstar
Copy link
Contributor Author

@AkihiroSuda
I am sorry for this. I mistakenly open this PR. I suppose to merge this with my fork and have it tested.
But here is my idea about WaitContainer. I should call WaitContainer after StartNamazuRoutines to ensure the parent process being blocked.

@v01dstar v01dstar reopened this Jun 24, 2016
@v01dstar
Copy link
Contributor Author

v01dstar commented Jun 24, 2016

PTAL @AkihiroSuda @mitake.
nmz will run in foreground and docker container will run in background in detach mode.

@coveralls
Copy link

coveralls commented Jun 24, 2016

Coverage Status

Changes Unknown when pulling 1555275 on v01dstar:detach-mode-support into * on osrg:master*.

@coveralls
Copy link

coveralls commented Jun 24, 2016

Coverage Status

Changes Unknown when pulling 1555275 on v01dstar:detach-mode-support into * on osrg:master*.

@AkihiroSuda
Copy link
Member

Thank you a lot 😄
LGTM

@AkihiroSuda
Copy link
Member

(the following discussion can be done in separate PR)

I still fear a user may be surprised that nmz container run -d keeps running in foreground.

So I think the nmz should daemonize itself in such a case.
But I'm not sure how we can safely daemonize golang program..
(golang/go#227, Chinese article: http://studygolang.com/articles/6190, Japanese article: http://qiita.com/k0kubun/items/d6d332697ef7a369e6e2, could not find other useful information in English 😢 )

What do you think about this?
@v01dstar @mitake

@AkihiroSuda
Copy link
Member

copying from gitter https://gitter.im/osrg/namazu?at=576e07bb74f25dd571bea9e2

v01dstar @v01dstar Jun 25 13:25
I am not sure. Namazu is not in client–server model like docker(docker client <-> docker daemon).
So, if we damonize nmz process, it would also surprise some users who may expect nmz will run in background by providing a & flag (eg. nmz container run -d nginx &).
Probably, we should inform users the behavior of using -d in our document or through nmz container run --help rather than daemonize nmz.

@AkihiroSuda
Copy link
Member

OK, merging, thanks!
Cc @mitake

@mitake
Copy link
Contributor

mitake commented Jun 27, 2016

@AkihiroSuda lgtm too, thanks a lot @v01dstar !

AkihiroSuda added a commit that referenced this pull request Sep 5, 2016
Release Note: http://osrg.github.io/namazu/post/release-0-2-1/

Changes from v0.2.0:

 * #167, #168, #169, #170: doc: miscellaneous improvements
 * #166: vendor go packages
 * #163: *: bump up Go to 1.7
 * #162: container: add support for inspectors/fs
 * #160: inspectors/fs: implement Fsync
 * #158, #159: inspectors/fs: improved CLI (thank you @v01dstar !)
 * #156: inspectors/proc: improved error handling
 * #154, #155: inspectors/proc: improved CLI

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
@AkihiroSuda AkihiroSuda mentioned this pull request Sep 5, 2016
AkihiroSuda added a commit that referenced this pull request Sep 5, 2016
Release Note: http://osrg.github.io/namazu/post/release-0-2-1/

Changes from v0.2.0:

 * #167, #168, #169, #170: doc: miscellaneous improvements
 * #166: vendor go packages
 * #163: *: bump up Go to 1.7
 * #162: container: add support for inspectors/fs
 * #160: inspectors/fs: implement Fsync
 * #158, #159: inspectors/fs: improved CLI (thank you @v01dstar !)
 * #156: inspectors/proc: improved error handling
 * #154, #155: inspectors/proc: improved CLI

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants