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

Wait functions for state #6232

Merged
merged 2 commits into from Jun 27, 2014

Conversation

Projects
None yet
6 participants
@LK4D4
Contributor

LK4D4 commented Jun 6, 2014

I added waiting functions for state and merged waitLock to state.
I intentionally did State.WaitLock receive timeout always. Because if you want waiting something forever you must suffer and think twice about it.
Also may be makes sense to embed *State to container - this is for discuss.

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jun 6, 2014

Contributor

ping @unclejack @crosbymichael @vieux
I'll reping after dockercon, also)

Contributor

LK4D4 commented Jun 6, 2014

ping @unclejack @crosbymichael @vieux
I'll reping after dockercon, also)

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jun 10, 2014

Contributor

Also, may be consider sync.Cond, but I'm not very excited about this idea.

Contributor

LK4D4 commented Jun 10, 2014

Also, may be consider sync.Cond, but I'm not very excited about this idea.

@LK4D4

This comment has been minimized.

Show comment
Hide comment
Contributor

LK4D4 commented Jun 11, 2014

@vieux

View changes

Show outdated Hide outdated daemon/container.go
// FIXME: why are we serializing running state to disk in the first place?
//log.Printf("%s: Failed to dump configuration to the disk: %s", container.ID, err)
if err := container.ToDisk(); err != nil {
utils.Errorf("Error dumping container state to disk: %s\n", err)
}
}
container.State.SetStopped(exitCode)

This comment has been minimized.

@vieux

vieux Jun 12, 2014

Collaborator

Here you set the state to stop after you write to disk.

I guess it's an issue because the container will be saved with a running state, no ?

@vieux

vieux Jun 12, 2014

Collaborator

Here you set the state to stop after you write to disk.

I guess it's an issue because the container will be saved with a running state, no ?

This comment has been minimized.

@LK4D4

LK4D4 Jun 13, 2014

Contributor

You are right. By this I tried to fix race between two RUN commands in buildfile, because they share same runconfig. So image id can change before we call ToDisk. But it seems that it must be fixed in buildfile.

@LK4D4

LK4D4 Jun 13, 2014

Contributor

You are right. By this I tried to fix race between two RUN commands in buildfile, because they share same runconfig. So image id can change before we call ToDisk. But it seems that it must be fixed in buildfile.

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jun 13, 2014

Contributor

@vieux fixed. I'll try to fix race in buildfile in other PR after discuss with you.

Contributor

LK4D4 commented Jun 13, 2014

@vieux fixed. I'll try to fix race in buildfile in other PR after discuss with you.

@cyphar

View changes

Show outdated Hide outdated daemon/container.go
// to return.
// FIXME: why are we serializing running state to disk in the first place?
//log.Printf("%s: Failed to dump configuration to the disk: %s", container.ID, err)
// FIXME: here is race condition between to RUN instructions in Dockerfile

This comment has been minimized.

@cyphar

cyphar Jun 16, 2014

Contributor

"two" RUN instructions?

@cyphar

cyphar Jun 16, 2014

Contributor

"two" RUN instructions?

This comment has been minimized.

@LK4D4

LK4D4 Jun 16, 2014

Contributor

yup

@LK4D4

LK4D4 Jun 16, 2014

Contributor

yup

@unclejack

This comment has been minimized.

Show comment
Hide comment
Contributor

unclejack commented Jun 16, 2014

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Jun 16, 2014

Collaborator

@anandkumarpatel @ykumar6 can you please test this one ?

Collaborator

vieux commented Jun 16, 2014

@anandkumarpatel @ykumar6 can you please test this one ?

@vieux

View changes

Show outdated Hide outdated daemon/container.go
@@ -499,6 +489,7 @@ func (container *Container) cleanup() {
}
func (container *Container) KillSig(sig int) error {
log.Printf("Send %d to %s", sig, container.ID)

This comment has been minimized.

@vieux

vieux Jun 16, 2014

Collaborator

can you switch to debugf here ?

@vieux

vieux Jun 16, 2014

Collaborator

can you switch to debugf here ?

@anandkumarpatel

This comment has been minimized.

Show comment
Hide comment
@anandkumarpatel

anandkumarpatel Jun 17, 2014

Contributor

Will check it out

Contributor

anandkumarpatel commented Jun 17, 2014

Will check it out

@anandkumarpatel

This comment has been minimized.

Show comment
Hide comment
@anandkumarpatel

anandkumarpatel Jun 17, 2014

Contributor

I still see a lot of these:
out: [error] container.go:482 9756b99bef648af3d5435f12eaa676454e61185a98dd2030bf1d12cbc89ad3d3: Error closing terminal: invalid argument

Contributor

anandkumarpatel commented Jun 17, 2014

I still see a lot of these:
out: [error] container.go:482 9756b99bef648af3d5435f12eaa676454e61185a98dd2030bf1d12cbc89ad3d3: Error closing terminal: invalid argument

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Jun 17, 2014

Collaborator

but do you have any panic ?

On Mon, Jun 16, 2014 at 7:37 PM, anandkumarpatel notifications@github.com
wrote:

I still see a lot of these:
out: [error] container.go:482
9756b99bef648af3d5435f12eaa676454e61185a98dd2030bf1d12cbc89ad3d3: Error
closing terminal: invalid argument


Reply to this email directly or view it on GitHub
#6232 (comment).

Victor VIEUX
http://vvieux.com

Collaborator

vieux commented Jun 17, 2014

but do you have any panic ?

On Mon, Jun 16, 2014 at 7:37 PM, anandkumarpatel notifications@github.com
wrote:

I still see a lot of these:
out: [error] container.go:482
9756b99bef648af3d5435f12eaa676454e61185a98dd2030bf1d12cbc89ad3d3: Error
closing terminal: invalid argument


Reply to this email directly or view it on GitHub
#6232 (comment).

Victor VIEUX
http://vvieux.com

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jun 17, 2014

Contributor

@vieux Done, and rebased.
Also I saw Error closing terminal: invalid argument when using dirty btrfs filesystem. I think this error appears on any container start error.

Contributor

LK4D4 commented Jun 17, 2014

@vieux Done, and rebased.
Also I saw Error closing terminal: invalid argument when using dirty btrfs filesystem. I think this error appears on any container start error.

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jun 17, 2014

Contributor

@LK4D4 I see this error message a tonne, even when running on ext4.

Contributor

cyphar commented Jun 17, 2014

@LK4D4 I see this error message a tonne, even when running on ext4.

@anandkumarpatel

This comment has been minimized.

Show comment
Hide comment
@anandkumarpatel

anandkumarpatel Jun 17, 2014

Contributor

@vieux the panic was fixed with #6065

Contributor

anandkumarpatel commented Jun 17, 2014

@vieux the panic was fixed with #6065

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Jun 17, 2014

Collaborator

@anandkumarpatel yes, but this PR touch the same code, I want to make sure we don't reintroduce the panic :)

Collaborator

vieux commented Jun 17, 2014

@anandkumarpatel yes, but this PR touch the same code, I want to make sure we don't reintroduce the panic :)

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Jun 17, 2014

Collaborator

LGTM

Collaborator

vieux commented Jun 17, 2014

LGTM

@vieux

View changes

Show outdated Hide outdated daemon/state.go
// WaitStop waits until state is stopped. If state already stopped it returns
// immediatly. If you want wait forever you must supply negative timeout.
// Returns exit code, that was passed to SetRunning

This comment has been minimized.

@vieux

vieux Jun 17, 2014

Collaborator

SetRunning takes a pid, you mean SetStop no ?

@vieux

vieux Jun 17, 2014

Collaborator

SetRunning takes a pid, you mean SetStop no ?

This comment has been minimized.

@LK4D4

LK4D4 Jun 17, 2014

Contributor

Yeah, you are right.

@LK4D4

LK4D4 Jun 17, 2014

Contributor

Yeah, you are right.

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jun 17, 2014

Contributor

Fixed docstring

Contributor

LK4D4 commented Jun 17, 2014

Fixed docstring

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jun 17, 2014

Contributor

@vieux Tried to fix #6259. Just moved SetStopped before cleanup.

Contributor

LK4D4 commented Jun 17, 2014

@vieux Tried to fix #6259. Just moved SetStopped before cleanup.

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Jun 17, 2014

Collaborator

sorry to bother you @anandkumarpatel
can you please retry to see if you get the panic back or not ?

Collaborator

vieux commented Jun 17, 2014

sorry to bother you @anandkumarpatel
can you please retry to see if you get the panic back or not ?

// WaitRunning waits until state is running. If state already running it returns
// immediatly. If you want wait forever you must supply negative timeout.
// Returns pid, that was passed to SetRunning
func (s *State) WaitRunning(timeout time.Duration) (int, error) {

This comment has been minimized.

@crosbymichael

crosbymichael Jun 19, 2014

Contributor

This seems kinda weird that we have WaitRunning and WaitStopped methods.

@crosbymichael

crosbymichael Jun 19, 2014

Contributor

This seems kinda weird that we have WaitRunning and WaitStopped methods.

This comment has been minimized.

@LK4D4

LK4D4 Jun 19, 2014

Contributor

I didn't get :) It is about naming or about existing?

@LK4D4

LK4D4 Jun 19, 2014

Contributor

I didn't get :) It is about naming or about existing?

This comment has been minimized.

@crosbymichael

crosbymichael Jun 20, 2014

Contributor

Why don't we just have one wait method?

@crosbymichael

crosbymichael Jun 20, 2014

Contributor

Why don't we just have one wait method?

This comment has been minimized.

@LK4D4

LK4D4 Jun 21, 2014

Contributor

Hmm, for example:

s := NewState()

go func() {
    SomethingBeforeStart()
    s.SetRunning(1)
}()
go func() {
    pid := s.WaitRunning()
    SomethingAfterStart(pid)
}()

And same for stop.
How can I achieve this with one wait method?

@LK4D4

LK4D4 Jun 21, 2014

Contributor

Hmm, for example:

s := NewState()

go func() {
    SomethingBeforeStart()
    s.SetRunning(1)
}()
go func() {
    pid := s.WaitRunning()
    SomethingAfterStart(pid)
}()

And same for stop.
How can I achieve this with one wait method?

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jun 20, 2014

Contributor
Contributor

LK4D4 commented Jun 20, 2014

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jun 27, 2014

Contributor

ping @crosbymichael , I have some race fixes, which depend on this

Contributor

LK4D4 commented Jun 27, 2014

ping @crosbymichael , I have some race fixes, which depend on this

LK4D4 added some commits Jun 6, 2014

State refactoring and add waiting functions
Docker-DCO-1.1-Signed-off-by: Alexandr Morozov <lk4d4math@gmail.com> (github: LK4D4)
Use State waiting functions
Docker-DCO-1.1-Signed-off-by: Alexandr Morozov <lk4d4math@gmail.com> (github: LK4D4)
@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jun 27, 2014

Contributor

rebased

Contributor

LK4D4 commented Jun 27, 2014

rebased

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Jun 27, 2014

Contributor

LGTM

Contributor

crosbymichael commented Jun 27, 2014

LGTM

crosbymichael added a commit that referenced this pull request Jun 27, 2014

@crosbymichael crosbymichael merged commit 680adb9 into moby:master Jun 27, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@LK4D4 LK4D4 deleted the LK4D4:wait_functions_for_state branch Jun 28, 2014

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