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

Use sdnotify for systemd #768

Merged
merged 2 commits into from Oct 25, 2016
Merged

Use sdnotify for systemd #768

merged 2 commits into from Oct 25, 2016

Conversation

guilhem
Copy link
Contributor

@guilhem guilhem commented Oct 24, 2016

This is useful if a configuration is long to load.
Systemd will continue dependency chain only when server have finish to start.

https://www.freedesktop.org/software/systemd/man/systemd.service.html#Type=

This is useful if a configuration is long to load.
Systemd will continue dependency chain only when server have finish to start.

https://www.freedesktop.org/software/systemd/man/systemd.service.html#Type=
Copy link
Contributor

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

design LGTM 😻

@guilhem
Copy link
Contributor Author

guilhem commented Oct 25, 2016

After a night of reflection... I think we can do a better design :)
More to come

@guilhem
Copy link
Contributor Author

guilhem commented Oct 25, 2016

Main problem is server.Start() is a blocking function. I can't evaluate when it finish to listen on socket etc.
With current patch it's working but it will send ready signal some little time before socket init...

@emilevauge emilevauge changed the title Use sdnotify for systemd [WIP] Use sdnotify for systemd Oct 25, 2016
@guilhem guilhem changed the title [WIP] Use sdnotify for systemd Use sdnotify for systemd Oct 25, 2016
@guilhem
Copy link
Contributor Author

guilhem commented Oct 25, 2016

Changes can be squashed and pushed with 1-clic

@guilhem
Copy link
Contributor Author

guilhem commented Oct 25, 2016

Many thanks to @nikkau !

defer server.Close()
sent, err := daemon.SdNotify("READY=1")
Copy link
Member

Choose a reason for hiding this comment

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

Don't you need to call server.Start() before?

Choose a reason for hiding this comment

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

Seems a good idea :) .
Sorry, bad C/C.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hopefully tests was there to protect us, but @emilevauge eyes are quite faster ;)

Copy link
Contributor

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

code LGTM \o/

@vdemeester
Copy link
Contributor

Let's squash the commit too if possible 👼

@guilhem
Copy link
Contributor Author

guilhem commented Oct 25, 2016

@vdemeester this can be done via GH button ;)

@vdemeester
Copy link
Contributor

@guilhem that's true, but I am not a huge fan of that 😸 — but let's do it 😝

@vdemeester vdemeester merged commit 649cb54 into traefik:v1.1 Oct 25, 2016
@guilhem guilhem deleted the sdnotify branch October 25, 2016 16:16
@guilhem
Copy link
Contributor Author

guilhem commented Oct 25, 2016

@vdemeester this have 1 advantage: keeping history in PR.
But I'm not really sure if good or not :/
Just experiment this pattern for now ^^

@vdemeester
Copy link
Contributor

@guilhem hum yeah but sometimes you want to actually have a PR with multiples commit merged as is. Anyway, this is no big deal 😝

emilevauge pushed a commit that referenced this pull request Dec 5, 2016
* Use sdnotify for systemd

This is useful if a configuration is long to load.
Systemd will continue dependency chain only when server have finish to start.

https://www.freedesktop.org/software/systemd/man/systemd.service.html#Type=

* Extract the waiting behavior from Start()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants