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

feature: add wait client command for pouch #1333

Merged
merged 1 commit into from
May 30, 2018

Conversation

xiechengsheng
Copy link
Contributor

@xiechengsheng xiechengsheng commented May 16, 2018

Ⅰ. Describe what this PR did

Add wait client command for pouch

Ⅱ. Does this pull request fix one issue?

fixes little part of #238

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

$ pouch ps
Name   ID       Status         Created         Image                                            Runtime
foo    f6717e   Up 2 seconds   3 seconds ago   registry.hub.docker.com/library/busybox:latest   runc
$ pouch stop foo
$ pouch ps -a
Name   ID       Status                 Created         Image                                            Runtime
foo    f6717e   Stopped (0) 1 minute   2 minutes ago   registry.hub.docker.com/library/busybox:latest   runc
$ pouch wait foo
0

Add more test codes are in test/cli_wait_test.go and test/api_container_wait_test.go

Ⅴ. Special notes for reviews

@HusterWan
Copy link
Contributor

fmt
/usr/local/go/bin/gofmt
test/cli_wait_test.go
please format Go code with 'gofmt -s -w'
Makefile:42: recipe for target 'fmt' failed
make: *** [fmt] Error 1
Exited with code 2

CI failed, Please resolve it first, Thanks a lot!!

@xiechengsheng
Copy link
Contributor Author

@HusterWan Oh, I forget to check the code format, thanks for your reply.

@codecov-io
Copy link

codecov-io commented May 16, 2018

Codecov Report

Merging #1333 into master will increase coverage by 0.06%.
The diff coverage is 47.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1333      +/-   ##
==========================================
+ Coverage   38.72%   38.79%   +0.06%     
==========================================
  Files         254      256       +2     
  Lines       16724    16808      +84     
==========================================
+ Hits         6477     6520      +43     
- Misses       9421     9459      +38     
- Partials      826      829       +3
Impacted Files Coverage Δ
cli/wait.go 0% <0%> (ø)
cli/main.go 0% <0%> (ø) ⬆️
apis/server/router.go 91.3% <100%> (+0.06%) ⬆️
apis/server/container_bridge.go 82.8% <100%> (+0.47%) ⬆️
client/container_wait.go 100% <100%> (ø)
daemon/mgr/container_types.go 76.81% <100%> (+0.69%) ⬆️
ctrd/container.go 50.9% <68.42%> (+2.84%) ⬆️
daemon/mgr/container.go 49.7% <71.42%> (ø) ⬆️
daemon/containerio/container_io.go 59.74% <0%> (-1.3%) ⬇️
... and 5 more

apiClient := wait.cli.Client()

var errs []string
for _, name := range args {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also do some check for the args

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I use errs = append(errs, err.Error()) to show client error messages.

command.PouchRun("run", "-d", "--name", name, busyboxImage, "sh", "-c", "true").Assert(c, icmd.Success)
defer DelContainerForceMultyTime(c, name)

output := command.PouchRun("wait", name).Stdout()
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer change this code to:

res := command.PouchRun("wait", name)
res. Assert(c, icmd.Success)
output := res.Stdout()

In this way, we could get more info if pouch wait failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, Thanks for your apply~

@xiechengsheng
Copy link
Contributor Author

Could anyone else help to review this pr? Thanks a lot. 😆

Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

Hi @xiechengsheng

I have commented for your change. Please take a look. Thanks

apis/swagger.yml Outdated
@@ -3428,3 +3451,4 @@ responses:
description: An unexpected server error occurred.
schema:
$ref: "#/definitions/Error"

Copy link
Contributor

Choose a reason for hiding this comment

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

Please Remove the blank line at the ending of file

@@ -500,3 +500,27 @@ func (c *Client) ResizeContainer(ctx context.Context, id string, opts types.Resi

return pack.task.Resize(ctx, uint32(opts.Height), uint32(opts.Width))
}

// WaitContainer waits until container's status is stopped.
func (c *Client) WaitContainer(ctx context.Context, id string, timeout time.Duration) (int64, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The waitContainer API doesn't require the timeout param as you mentioned in the swagger.yml.

I think we can remove the timeout here so that it can make wait word clear. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we could do this, the timeout is not used in the next codes.

StatusCode:
description: "Exit code of the container"
type: "integer"
x-nullable: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the ProbeContainer maybe return the unexpected error, I think we should add the error in the response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fuweid Emmm, IMO, I don't think ProbeContainer error should appear in HTTP response. The codes in ctrd/container.go WaitContainer() function show that if ProbeContainer error occurs, the function will return -1(wrong exit code) and show client the ProbeContainer error message as many other error cases do; And function apis/server/container_bridge.go waitContainer returns the error directly so that the client can see the error message. If the code can run to EncodeResponse(...), the container must have exited successfully. So what's your opinion?

Copy link
Contributor

@fuweid fuweid May 21, 2018

Choose a reason for hiding this comment

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

So, the error means 500? The error is caused by the container, not the server. The waitContainer API doesn't meets any error during the waiting. That is why that I suggest to use the error in the response. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll add it.


CreateBusyboxContainerOk(c, cname)
StartContainerOk(c, cname)
StopContainerOk(c, cname)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add some cases about running container? I think the common case is using for the running container

Copy link
Contributor

Choose a reason for hiding this comment

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

Solved

case status := <-chWait:
c.Assert(status, check.Equals, fmt.Sprintf("%s\n", "0"))
case <-time.After(2 * time.Second):
c.Errorf("timeout waiting for `docker wait` to exit")
Copy link
Contributor

Choose a reason for hiding this comment

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

No docker wait, is pouch wait

Copy link
Contributor Author

@xiechengsheng xiechengsheng May 21, 2018

Choose a reason for hiding this comment

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

I feel so sorry, this test file is locally referred to docker tests. A little bit embarrassed. 😂

case status := <-chWait:
c.Assert(status, check.Equals, fmt.Sprintf("%s\n", "99"))
case <-time.After(2 * time.Second):
c.Errorf("timeout waiting for `docker wait` to exit")
Copy link
Contributor

Choose a reason for hiding this comment

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

No docker wait, is pouch wait

@fuweid fuweid self-assigned this May 21, 2018
@@ -53,6 +53,7 @@ func initRoute(s *Server) http.Handler {
s.addRoute(r, http.MethodGet, "/containers/{name:.*}/logs", withCancelHandler(s.logsContainer))
s.addRoute(r, http.MethodPost, "/containers/{name:.*}/resize", s.resizeContainer)
s.addRoute(r, http.MethodPost, "/containers/{name:.*}/restart", s.restartContainer)
s.addRoute(r, http.MethodPost, "/containers/{name:.*}/wait", s.waitContainer)
Copy link
Contributor

Choose a reason for hiding this comment

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

One more thing, we should add the withCancelHandler to wrap the s.waitContainer, just in case that the client closes the connection without patience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right~

@xiechengsheng xiechengsheng force-pushed the add-wait branch 2 times, most recently from 70e58f1 to e2b16c3 Compare May 21, 2018 15:12
@xiechengsheng
Copy link
Contributor Author

xiechengsheng commented May 21, 2018

@fuweid Thanks for your review, I have modified many codes according to your suggestions but I don't know whether the latest pr meets with your thought or not. So could you help to recheck the function ctrd/container.go WaitContainer() please? In my eyes this function codes seems to be a little bit redundant. Do you have a better way to solve this problem? Thanks in advance! 😆

@@ -39,6 +39,7 @@ type ContainerAPIClient interface {
ContainerTop(ctx context.Context, name string, arguments []string) (types.ContainerProcessList, error)
ContainerLogs(ctx context.Context, name string, options types.ContainerLogsOptions) (io.ReadCloser, error)
ContainerResize(ctx context.Context, name, height, width string) error
ContainerWait(ctx context.Context, name string) (int64, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can use ContainerWaitOKBody as return value instead of int64, since the error is one of kind error, like 404, 505 and so on. It can make your code clean. Make senses to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll try it.

ctx = leases.WithLease(ctx, wrapperCli.lease.ID())

waitExit := func() *Message {
return c.ProbeContainer(ctx, id, -1*time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

I still have concerns about this function ProbeContainer, because it consumes the channel receiver's data. If you consumes the data here, some function will loss the data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fuweid You're right. But this channel ch seems only to be consumed in function ProbeContainer. And this function is called in function DestroyContainer in order to wait container exits normally. And if you don't mind, I can add additional channel waitMeg chan *Message in struct containerPack for WaitContainer function to consume specially. I don't have a more elegant way to solve it out...

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @xiechengsheng , please go ahead and show your code. We can find the way out. 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

@xiechengsheng @fuweid ProbeContainer consumes the channel data, and it will push the data back again. So we will not loss the data

Copy link
Contributor

Choose a reason for hiding this comment

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

Solved

@fuweid
Copy link
Contributor

fuweid commented May 22, 2018

Hi @xiechengsheng , thanks for contribution. But I still have some comments. Please take a look. Thanks

cli/wait.go Outdated
baseCommand
}

// Init initialize wait command.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/initialize/initializes/

@fuweid
Copy link
Contributor

fuweid commented May 23, 2018

@xiechengsheng good job. Please update the typo and I will merge it

if err != nil {
errMsg = err.Error()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fuweid I am still concerned about the necessity of Error member in ContainerWaitOKBody structure. One one thing, this Error member value comes directly from the function Wait, and if an error occurs in function Wait, the err value in sequence status, err := s.ContainerMgr.Wait(ctx, name) is not nil, then this function waitContainer will directly return err and show error messages to client so that user can see this error; If err in sequence status, err := s.ContainerMgr.Wait(ctx, name) is nil, then member Error of structure ContainerWaitOKBody is also "" so that user won't see the error message. So in sequence Error: errMsg the errMsg must be "" if program can reach to this sequence; On the other hand, traceback to the internal function WaitContainer, we can find each path of function call will return err if an err occurs in internal function WaitContainer, and user will see the error message. So in my eyes this variable errMsg is useless. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

@xiechengsheng , when you run command and get wrong something from shell, not only you get the error code, but also the error message, like command not found: xxx. errMsg is used to tell user that hey, you got something wrong here. Does it make senses to you?

In your commit, we can define the struct to wrap error code and error message. The error message is different from the error which means the server got something.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thx.

}

errMsg := ""
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the err is not nil, the following statements are unreachable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fuweid If err is not nil, the HTTP response API should return this err directly. If I don't return this err and just return errMsg, the HTTP response code would be 200, not 404/500.

Copy link
Contributor

Choose a reason for hiding this comment

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

if err != nil {
       return err
}

// if the err is not nil, the above statement will return and the following will not execute.
errMsg := ""
if err != nil {
...
}

In my options, the s.ContainerMgr.Wait should return two vars.

One is

type ErrStatus {
    err error
    errMsg string
}

Other one is error.

Make senses? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll add this.

if err != nil {
errMsg = err.Error()
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@xiechengsheng , when you run command and get wrong something from shell, not only you get the error code, but also the error message, like command not found: xxx. errMsg is used to tell user that hey, you got something wrong here. Does it make senses to you?

In your commit, we can define the struct to wrap error code and error message. The error message is different from the error which means the server got something.

WDYT?

@xiechengsheng xiechengsheng force-pushed the add-wait branch 2 times, most recently from b0d78d1 to d53c13b Compare May 24, 2018 05:54
Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

@xiechengsheng , please take a look at my comments.

Error: err.Error(),
StatusCode: -1,
}, fmt.Errorf("failed to get a containerd grpc client: %v", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If we got error from c.Get(ctx), we can keep the ContainerWaitOKBody simple and return types.ContainerWaitOKBody{}, fmt.Errorf("failed to get a containerd grpc client: %v", err).

In golang, basically we don't consume the data if error != nil

// wait for the task to exit.
msg = waitExit()

if err := msg.RawError(); err != nil && errtypes.IsTimeout(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the errtypes.IsTimeout(err) is true, we can return types.ContainerWaitOKBody{}, err like my last comment. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll fix all.

Copy link
Contributor

Choose a reason for hiding this comment

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

Solved

Copy link
Contributor

Choose a reason for hiding this comment

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

emmm... could we remove this duplicate line, since you have done it in the following statement?

cli/wait.go Outdated
for _, name := range args {
response, err := apiClient.ContainerWait(ctx, name)
if err != nil {
errs = append(errs, "container exitcode: "+strconv.FormatInt(response.StatusCode, 10)+
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just show the error without response, because in golang we don't consume the data if err != nil.

resp, err := client.post(ctx, "/containers/"+name+"/wait", nil, nil, nil)

if err != nil {
return types.ContainerWaitOKBody{
Copy link
Contributor

Choose a reason for hiding this comment

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

Just return the empty types.ContainerWaitOKBody is ok

func (mgr *ContainerManager) Wait(ctx context.Context, name string) (types.ContainerWaitOKBody, error) {
c, err := mgr.container(name)
if err != nil {
return types.ContainerWaitOKBody{
Copy link
Contributor

Choose a reason for hiding this comment

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

please return the error and empty types.ContainerWaitOKBody directly.

// If a container status is exited or stopped, return exit code immediately.
if c.IsExited() || c.IsStopped() {
return types.ContainerWaitOKBody{
Error: "",
Copy link
Contributor

Choose a reason for hiding this comment

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

please set the c.State.Error for Error field.

resp *http.Response
)

chWait := make(chan *http.Response)
Copy link
Contributor

Choose a reason for hiding this comment

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

tip: if we uses the wait channel for notify, we can use empty struct in golang 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Emmm, I think this channel is not only used for notify but also transferring data to the variable resp in the next. So I think we can't use empty struct here. WDYT?

Copy link
Contributor

@fuweid fuweid May 24, 2018

Choose a reason for hiding this comment

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

Sorry. I misunderstand your point.

Both resp and err vars are used as closure and it seems that chWait is useless here.

But If we put the StopContainerOK in goroutine and use request.WithContext(timeoutCtx), it seems that it can be clean. The chWait is used to do notify.

WDYT?

"while true; do usleep 10; done").Assert(c, icmd.Success)
defer DelContainerForceMultyTime(c, name)

chWait := make(chan string)
Copy link
Contributor

Choose a reason for hiding this comment

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

tip: please try empty struct for the wait channel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This channel type is string, can this be converted to struct{}? And this string channel's data will be transferred to variable status in the next.

Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

@xiechengsheng Last round review. Please take a look. Last one!

ctx = leases.WithLease(ctx, wrapperCli.lease.ID())

waitExit := func() *Message {
return c.ProbeContainer(ctx, id, -1*time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

Solved

// wait for the task to exit.
msg = waitExit()

if err := msg.RawError(); err != nil && errtypes.IsTimeout(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Solved


CreateBusyboxContainerOk(c, cname)
StartContainerOk(c, cname)
StopContainerOk(c, cname)
Copy link
Contributor

Choose a reason for hiding this comment

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

Solved

}

return types.ContainerWaitOKBody{
Error: "",
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry @xiechengsheng , I miss this one. Please put the msg.RawError() to the Error field

@xiechengsheng xiechengsheng force-pushed the add-wait branch 4 times, most recently from 13a29b0 to b6e3027 Compare May 26, 2018 04:34
@xiechengsheng
Copy link
Contributor Author

It seems that Travis CI build always fails, the error message is:

...
No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

How to solve this?

@fuweid
Copy link
Contributor

fuweid commented May 28, 2018

@xiechengsheng , could you please rebase your branch with latest master branch, since the case has updated by other PR? Thanks

@xiechengsheng
Copy link
Contributor Author

@fuweid Emmmm. I update the branch and repush codes, but it is useless... Seems a testing bug?

@fuweid
Copy link
Contributor

fuweid commented May 28, 2018

Hi @xiechengsheng , could you please to update the -check.v to -check.vv in hack/make.sh#271L? Therefore, we can see which case takes so long.

@xiechengsheng
Copy link
Contributor Author

I'm sorry it's my code's bug......

@HusterWan
Copy link
Contributor

Woo!! what a long discuss ! :)
@xiechengsheng @fuweid what the progress, can we finish this feature now ?

// We should notice that container's meta data shouldn't be locked in wait process, otherwise waiting for
// a running container to stop would make other client commands which manage this container are blocked.
// If a container status is exited or stopped, return exit code immediately.
if c.IsExited() || c.IsStopped() {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about the created container? What is the behaviour?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the created case is very special, this concern is very considerate. So I dive into the code and find that when a new container is created and running, this map watch.containers would record container's pack info. If a container state is created, this code won't go to if statement codeblock and will run to WaitContainer function in ctrd/container.go, so this WaitContainer function will call ProbeContainer function, and ProbeContainer function will look up container pack info from the above map watch.containers. This function watch.notify() will look up map watch.containers, so when the container is just created, the look up process will fail and just return the Message{} to channel ch, so the ProbeContainer function will get Message{} channel struct and this struct's member uint 32 ExitCode would be initialized with default number 0. So when waiting a created container, the client will see the code 0, which is the same as moby.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, can we add the case for this? Thanks

}

errMsg := ""
if err := msg.RawError(); err != nil && errtypes.IsTimeout(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@xiechengsheng , could we use the following statement to make the logic clear? WDYT

errMsg := ""
err := msg.RawError()
if err != nil {
       if errtypes.IsTimeout(err) {
		return types.ContainerWaitOKBody{}, err
       }
       errMsg = err.Error()
}
....

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's ok, only if the code is clear in logic

// We should notice that container's meta data shouldn't be locked in wait process, otherwise waiting for
// a running container to stop would make other client commands which manage this container are blocked.
// If a container status is exited or stopped, return exit code immediately.
if c.IsExited() || c.IsStopped() {
Copy link
Contributor

Choose a reason for hiding this comment

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

So, can we add the case for this? Thanks

resp, err = request.Post("/containers/" + cname + "/wait")
close(chWait)
}()
time.Sleep(100 * time.Millisecond)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we have problem for this case.

<-chWait return which means that the channel chWait has been closed. In the next statement select , the <-chWait always return.

package main

import (
        "fmt"
        "time"
)

func main() {
        ch := make(chan struct{})

        go func() {
                time.Sleep(2 * time.Second)
                close(ch)
        }()
        fmt.Println(<-ch)
        fmt.Println(<-ch)
}

So I think we still need to update the case like, WDYT?

chWait := make(chan struct{})
go func() {
        chWait <- struct{}{}
        resp, err = request.Post("/containers/" + cname + "/wait")
        close(chWait)
}()
<-chWait // wait for the goroutine activate
time.Sleep(100 * time.Millisecond)
stopContainerOk(c, cname)

select {
...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

since we have call <-chWait before, why we wait chWait in select again ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@HusterWan we need to make sure that the Stop command can let the wait quit.


// TestRunningContainer tests waiting a running container to stop, then returns 200.
func (suite *APIContainerWaitSuite) TestRunningContainer(c *check.C) {
cname := "TestRunningContainer"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the TestWaitForRunningContainer so that the case doesn't impact other case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree! @xiechengsheng please change the function name, because TestRunningContainer may has confusion, thanks a lot !

@xiechengsheng
Copy link
Contributor Author

@fuweid OK, I'll fix all ASAP.

// wait for the task to exit.
msg = waitExit()

if err := msg.RawError(); err != nil && errtypes.IsTimeout(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

emmm... could we remove this duplicate line, since you have done it in the following statement?

// If a container status is created, return 0 as status code.
if c.IsCreated() {
return types.ContainerWaitOKBody{
Error: c.State.Error,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just left the Error field empty. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll fix all.

@fuweid
Copy link
Contributor

fuweid commented May 30, 2018

LGTM. I will merge it after CI is green.

Thanks @xiechengsheng ! Great job.

@fuweid fuweid merged commit f3f72cf into AliyunContainerService:master May 30, 2018
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