Remove engine by runcom · Pull Request #12771 · moby/moby

runcom · 2015-04-24T23:23:26Z

close #12255
close #12151
close #12713
close #10389

Related to #12905

Signed-off-by: Antonio Murdaca [email protected]

jessfraz · 2015-04-24T23:28:53Z

🎀

runcom · 2015-04-24T23:30:46Z

I think docs should be modified too because no more test-integration

runcom · 2015-04-25T00:01:46Z

api/server/server.go

maybe Shutdown

I like Close much more

LK4D4 · 2015-04-25T02:42:54Z

Yeah, iptables clean is fun :)
@jfrazelle What job was protected against shutdown for this?

LK4D4 · 2015-04-25T02:55:48Z

@runcom Just noticed that you still have supervisor. Maybe will be simpler to fix bug after moving all to daemon.Shutdown.

LK4D4 · 2015-04-25T03:02:46Z

About cleanup: you probably should create sync.Onces for api.Close() and daemon.Shutdown and just call them in defer of NewDaemon too.

LK4D4 · 2015-04-25T03:08:38Z

@runcom Ok, bug in calling shutdowns from daemon in Trap. I think all will be cool when you'll refactor it to only api.Close in Trap.

runcom · 2015-04-25T13:27:44Z

@LK4D4 there are still issues..
if signal.Trap is only used for api.Close then kill -9 nor ctrl^c don't clean the daemon (nor pidfile) because only api.Close gets executed, instead eng.Shutdown was doing both close api + daemon clean
Cleaning of pidfile in docker/daemon in a defer func could work cause it suffers the same problem of daemon shutdown if only api.Close is trapped
Cleaning of driver and graph will be duplicate now because is called once if daemon could not be initialized and next if error occurs after it's initialized

I'm porting tests to integration-cli in the meantime

LK4D4 · 2015-04-25T17:07:58Z

@runcom Yeah, because Trap is sucks. Cleanup of graph and stuff on daemon initialization fail should be inside NewDaemon.
Makes sense to refactor api.Server to have Wait() to wait until all servers are closed, then cleanup for trap will be:

api.Close()
api.Wait()
daemon.Shutdown()

But for now you can use channel waitAPI or smth like this from mainDaemon.

runcom · 2015-04-25T17:26:04Z

@LK4D4 cool, but then how do you Trap api.Close + daemon.Shutdown? because old behaviour was trap api.Close + d.Shutdown from engine.Shutdown, we now have only a single call to signal.Trap(api.Close), could be that Trap could accepts a slice of funcs to execute also?
Also Trap is just used inside docker/daemon..

LK4D4 · 2015-04-25T17:36:05Z

You can create closure with any functions you want.

runcom · 2015-04-25T17:38:39Z

/me shame (but tests green now)

cpuguy83 · 2015-04-27T19:52:22Z

daemon/daemon.go

~~Only closing graph if there was no error? Don't we want to close no matter what?~~
Nevermind, this isn't the piece of code I thought it was.

LK4D4 · 2015-04-27T22:36:19Z

LGTM

cpuguy83 · 2015-04-28T03:05:12Z

docker/daemon.go

So we're just logging that the daemon needs to be force stopped?

It means that probably not all containers died in 15 seconds and we just brutally kill daemon.

But where are we killing it?

in signal.Trap

I know signal.Trap has it's own bail stuff (3 sigs from the user)... but before engine was exiting after ~10s timeout period, now this timeout could probably be increased, but still seems like we should kill it after the timeout.

@cpuguy83 Who should we kill? :) if mainDaemon function exits, then process just stops, nothing can stand this.

So as is the daemon will just hang until someone sends 2 more kill sigs to it.

cpuguy83 · 2015-04-28T03:08:42Z

Looks really good, just one question, but otherwise LGTM.

LK4D4 · 2015-04-28T22:10:45Z

@runcom Is it ready?

LK4D4 · 2015-04-28T22:39:03Z

@runcom Oh, requires rebase, sorry :)

runcom · 2015-04-29T11:58:45Z

@LK4D4 almost, there are many tests inside integration/ that have to be ported to integration-cli/, trying to do my best and as fast as I can 😄

estesp · 2015-04-29T22:03:09Z

is this ready to go? @LK4D4 ?

Signed-off-by: Antonio Murdaca <[email protected]>

LK4D4 · 2015-04-30T03:29:43Z

Fails looks like registry down.

runcom · 2015-04-30T13:12:03Z

@LK4D4 yup, need a rebuild
Now tests green

tiborvass · 2015-04-30T16:16:57Z

LGTM

Remove engine

runcom · 2015-04-30T16:19:11Z

🎉

tiborvass · 2015-04-30T16:19:34Z

Huge thanks to @runcom and everyone involved in #12151 !!

RIP engine

GordonTheTurtle added the status/0-triage label Apr 24, 2015

jessfraz added status/2-code-review and removed status/0-triage labels Apr 24, 2015

runcom reviewed Apr 25, 2015
View reviewed changes

runcom force-pushed the say-bye-to-engine branch 4 times, most recently from 9360fb3 to 9ea81f2 Compare April 25, 2015 17:07

runcom force-pushed the say-bye-to-engine branch 8 times, most recently from 890ba7e to 42aad7a Compare April 25, 2015 20:40

cpuguy83 reviewed Apr 27, 2015
View reviewed changes

runcom force-pushed the say-bye-to-engine branch 4 times, most recently from 1661c14 to 1820616 Compare April 27, 2015 22:04

cpuguy83 reviewed Apr 28, 2015
View reviewed changes

runcom force-pushed the say-bye-to-engine branch 3 times, most recently from 3c6198c to 9f21dab Compare April 28, 2015 20:58

runcom force-pushed the say-bye-to-engine branch 3 times, most recently from 958f68c to c0c2c7b Compare April 29, 2015 11:56

runcom force-pushed the say-bye-to-engine branch from c0c2c7b to 4b8cff6 Compare April 29, 2015 14:19

Antonio Murdaca added 2 commits April 30, 2015 01:35

Remove engine mechanism

531f412

Signed-off-by: Antonio Murdaca <[email protected]>

Remove integration tests and port them to integration-cli

f7e417e

Signed-off-by: Antonio Murdaca <[email protected]>

runcom force-pushed the say-bye-to-engine branch from 4b8cff6 to f7e417e Compare April 29, 2015 23:45

tiborvass added a commit that referenced this pull request Apr 30, 2015

Merge pull request #12771 from runcom/say-bye-to-engine

0d0b425

Remove engine

tiborvass merged commit 0d0b425 into moby:master Apr 30, 2015

runcom deleted the say-bye-to-engine branch April 30, 2015 16:18

Comments

Conversation

runcom commented Apr 24, 2015

Uh oh!

jessfraz commented Apr 24, 2015

Uh oh!

runcom commented Apr 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Apr 25, 2015

Uh oh!

LK4D4 commented Apr 25, 2015

Uh oh!

LK4D4 commented Apr 25, 2015

Uh oh!

LK4D4 commented Apr 25, 2015

Uh oh!

runcom commented Apr 25, 2015

Uh oh!

LK4D4 commented Apr 25, 2015

Uh oh!

runcom commented Apr 25, 2015

Uh oh!

LK4D4 commented Apr 25, 2015

Uh oh!

runcom commented Apr 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Apr 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Apr 28, 2015

Uh oh!

LK4D4 commented Apr 28, 2015

Uh oh!

LK4D4 commented Apr 28, 2015

Uh oh!

runcom commented Apr 29, 2015

Uh oh!

estesp commented Apr 29, 2015

Uh oh!

LK4D4 commented Apr 30, 2015

Uh oh!

runcom commented Apr 30, 2015

Uh oh!

tiborvass commented Apr 30, 2015

Uh oh!

runcom commented Apr 30, 2015

Uh oh!

tiborvass commented Apr 30, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants