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

Shutdown plugins during daemon shutdown. #24229

Merged
merged 1 commit into from
Jul 12, 2016

Conversation

anusha-ragunathan
Copy link
Contributor

Signed-off-by: Anusha Ragunathan anusha@docker.com

@cpuguy83
Copy link
Member

cpuguy83 commented Jul 1, 2016

But we shouldn't do this if live-restore is enabled, right?

@cpuguy83
Copy link
Member

cpuguy83 commented Jul 1, 2016

daemon/daemon.go:687: undefined: plugin.GetManager

@anusha-ragunathan
Copy link
Contributor Author

@cpuguy83 : The live-restore case bails out much early in the function.

@@ -128,3 +128,21 @@ func (pm *Manager) disable(p *plugin) error {
pm.save()
return nil
}

// Shutdown stops all plugins and called during daemon shutdown.
func (pm *Manager) Shutdown() error {
Copy link
Member

Choose a reason for hiding this comment

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

We never return an error, correct? Should we fail if a plugin fails to stop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We return the error now. But its purely for logging purposes. The daemon shutdown should not fail, if any of the sub components fail to stop. It should continue its best-effort shutdown, as it does today.

@thaJeztah
Copy link
Member

@anusha-ragunathan is this for 1.12?

@anusha-ragunathan anusha-ragunathan force-pushed the shutdown-plugins branch 2 times, most recently from 6ff4422 to 552e64a Compare July 5, 2016 15:09
@anusha-ragunathan anusha-ragunathan added this to the 1.12.0 milestone Jul 5, 2016
@anusha-ragunathan
Copy link
Contributor Author

@thaJeztah : Yes, we should take this for 1.12

@LK4D4
Copy link
Contributor

LK4D4 commented Jul 5, 2016

@anusha-ragunathan Is it possible to write test?

@anusha-ragunathan
Copy link
Contributor Author

@LK4D4 : Done!

@LK4D4
Copy link
Contributor

LK4D4 commented Jul 5, 2016

@anusha-ragunathan great, thanks!

@LK4D4
Copy link
Contributor

LK4D4 commented Jul 5, 2016

I think experimental failure is unrelated.
LGTM

@cpuguy83
Copy link
Member

cpuguy83 commented Jul 6, 2016

daemon/daemon_experimental.go:15: plugin.GetManager().Shutdown undefined (type *plugin.Manager has no field or method Shutdown)

@anusha-ragunathan anusha-ragunathan force-pushed the shutdown-plugins branch 2 times, most recently from 4eeaeb4 to 8e1a065 Compare July 6, 2016 16:59
@anusha-ragunathan
Copy link
Contributor Author

@cpuguy83 : Fixed the experimental windows cross compile issue. PTAL.
Experimental and Windows native are failing across the board.

}
}
if pm.containerdClient != nil {
if err := pm.containerdClient.Signal(p.P.ID, int(syscall.SIGKILL)); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

We should give the plugin a chance to shutdown gracefully with SIGTERM

Copy link
Member

Choose a reason for hiding this comment

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

Or rather STOPSIGNAL if that was brought over from the image format.

Signed-off-by: Anusha Ragunathan <anusha@docker.com>
@anusha-ragunathan
Copy link
Contributor Author

@cpuguy83 : Graceful shutdown added. PTAL.

@cpuguy83
Copy link
Member

LGTM

@cpuguy83 cpuguy83 merged commit b91e2dd into moby:master Jul 12, 2016
@anusha-ragunathan anusha-ragunathan deleted the shutdown-plugins branch July 19, 2016 01:30
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.

None yet

6 participants