Add ready plugin #2616

miekg · 2019-02-25T16:15:59Z

Add a ready plugin that allows plugin to signal when they are ready.
This uses simple functions defined in the ready plugin; RegisterPlugin
and Signal.

Add README.md and tests moduled on the health plugin; which will be
relegated to just providing process health. In similar vein to health
this is a process wide setting meaning it will need to include all
plugins for all server block when signalling readiness.

During startup it block forever, while displaying the status every
second:

2019-02-25T16:09:02.168Z [INFO] plugin/ready: Waiting on 2/2: erratic
2019-02-25T16:09:03.168Z [INFO] plugin/ready: All plugins signal readiness
.:1053
bla.:1053
2019-02-25T16:09:03.168Z [INFO] CoreDNS-1.3.1
2019-02-25T16:09:03.168Z [INFO] linux/amd64, go1.11.4,
CoreDNS-1.3.1
linux/amd64, go1.11.4,
^C[INFO] SIGINT: Shutting down

This was started with this Corefile

. {
    erratic
    whoami
    ready
}

bla {
    erratic
    whoami
}

Both erratic plugins are reporting readiness.

Signed-off-by: Miek Gieben miek@miek.nl

See #2611 and #2547

corbot · 2019-02-25T16:16:07Z

Thank you for your contribution. I've just checked the OWNERS files to find a suitable reviewer. This search was successful and I've asked greenpau (via /OWNERS) for a review.

If you have questions or suggestions for this bot, please file an issue against the miekg/dreck repository.

The bot understands the commands that are listed here.

fturib · 2019-02-25T18:21:17Z

plugin/ready/setup.go

+	c.OnStartup(r.wait)
+	c.OnRestart(r.onRestart)
+	c.OnFinalShutdown(r.onFinalShutdown)
+


Thanks to the new "OnRestartFailed" event that was introduced few months ago in Caddy.
We can handle the failed restart (when the Corefile is invalid).

i guess we could have:

c.OnStartup(r.onStartup) c.OnStartup(r.wait) c.OnRestart(r.onRestart) c.OnRestartFailed(r.onStartup) c.OnRestartFailed(r.wait) c.OnFinalShutdown(r.onFinalShutdown)

miekg · 2019-02-25T18:58:46Z

[ Quoting <notifications@github.com> in "Re: [coredns/coredns] Add *ready* p..." ]

fturib commented on this pull request. > + }) +} + +func setup(c *caddy.Controller) error { + addr, err := parse(c) + if err != nil { + return plugin.Error("ready", err) + } + + r := new(addr) + + c.OnStartup(r.onStartup) + c.OnStartup(r.wait) + c.OnRestart(r.onRestart) + c.OnFinalShutdown(r.onFinalShutdown) + Thanks to the new "OnRestartFailed" event that was introduced few months ago in Caddy. We can handle the failed restart (when the Corefile is invalid). i guess we could have: ``` c.OnStartup(r.onStartup) c.OnStartup(r.wait) c.OnRestart(r.onRestart) c.OnRestartFailed(r.onStartup) c.OnRestartFailed(r.wait) c.OnFinalShutdown(r.onFinalShutdown) ```

oh, that's awesome!

codecov-io · 2019-02-26T11:41:20Z

Codecov Report

Merging #2616 into master will increase coverage by 0.1%.
The diff coverage is 66.33%.

@@            Coverage Diff            @@
##           master    #2616     +/-   ##
=========================================
+ Coverage   55.56%   55.66%   +0.1%     
=========================================
  Files         203      207      +4     
  Lines       10307    10408    +101     
=========================================
+ Hits         5727     5794     +67     
- Misses       4150     4180     +30     
- Partials      430      434      +4

Impacted Files	Coverage Δ
plugin/erratic/ready.go	`0% <0%> (ø)`
plugin/ready/list.go	`100% <100%> (ø)`
plugin/ready/setup.go	`47.61% <47.61%> (ø)`
plugin/ready/ready.go	`78.12% <78.12%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ffa4f1...30e326c. Read the comment docs.

chrisohaver · 2019-02-26T13:46:16Z

Thanks for adding this, although, I was thinking we'd just need report readiness (and not actually block serving until all plugins are ready).

miekg · 2019-02-26T18:59:34Z

fair point.

It's also global in that it crosses server block boundaries in the current impl. That's not good and counter intuitive; meaning should look even more like health

miekg · 2019-02-26T21:05:06Z

ok, reworked it to;
a) Address Chris' comment
b) make it local to a server block

johnbelamaric · 2019-02-26T21:26:08Z

plugin/ready/ready.go

+			io.WriteString(w, "OK")
+			return
+		}
+		log.Infof("Still waiting on: %q", todo)


Is there a reason not to also write this to the body of the response? A 500 error can still have a body. Something like {"plugin-name": false, ...etc...}

I guess it exposes some internals we might now want to.

+1 to make it json per your example (i.e. not just for curling humans).

It does expose internals, so might want to allow users to disable it.

miekg · 2019-02-26T21:27:28Z

Might be nice to add for when a human runs a curl

…

On Tue, 26 Feb 2019, 21:26 John Belamaric, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In plugin/ready/ready.go <#2616 (comment)>: > + } + + rd.Lock() + rd.ln = ln + rd.mux = http.NewServeMux() + rd.done = true + rd.Unlock() + + rd.mux.HandleFunc("/ready", func(w http.ResponseWriter, _ *http.Request) { + ok, todo := plugs.Ready() + if ok { + w.WriteHeader(http.StatusOK) + io.WriteString(w, "OK") + return + } + log.Infof("Still waiting on: %q", todo) Is there a reason not to also write this to the body of the response? A 500 error can still have a body. Something like {"plugin-name": false, ...etc...} I guess it exposes some internals we might now want to. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2616 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVkW5sydASxn3iO3rQXnu7XqDP5e2hKks5vRaZygaJpZM4bQPIM> .

miekg · 2019-02-27T21:01:25Z

[ Quoting <notifications@github.com> in "Re: [coredns/coredns] Add *ready* p..." ]

+1 to make it json per your example (i.e. not just for curling humans). It does expose internals, so might want to allow users to disable it.

Thanks, but I think both suggestions are taking this a bit too far.

miekg · 2019-03-01T14:39:37Z

This is now fully ready. PTAL

The proposed changes from @fturib will be done in a seperate PR for: health, prometheus and this plugin.

johnbelamaric · 2019-03-01T20:39:41Z

/lgtm

corbot

LGTM by johnbelamaric

@localhost

Add a ready plugin that allows plugin to signal when they are ready. Once a plugin is ready it is not queried again. This uses same mechanism as the health plugin: each plugin needs to implement an interface. Implement readines for the *erratic* plugin to aid in testing. Add README.md and tests moduled after the health plugin; which will be relegated to just providing process health. In similar vein to health this is a process wide setting. With this Corefile: ~~~ . { erratic whoami ready } bla { erratic whoami } ~~~ ready will lead to: ~~~ sh % curl localhost:8181/ready % dig @localhost -p 1053 mx example.org % curl localhost:8181/ready OK% ~~~ Meanwhile CoreDNS logs: ~~~ .:1053 bla.:1053 2019-02-26T20:59:07.137Z [INFO] CoreDNS-1.3.1 2019-02-26T20:59:07.137Z [INFO] linux/amd64, go1.11.4, CoreDNS-1.3.1 linux/amd64, go1.11.4, 2019-02-26T20:59:11.415Z [INFO] plugin/ready: Still waiting on: "erratic" 2019-02-26T20:59:13.510Z [INFO] plugin/ready: Still waiting on: "erratic" ~~~ *ready* can be used in multiple server blocks and will do the right thing; query all those plugins from all server blocks for readiness. This does a similar thing to the prometheus plugin. Signed-off-by: Miek Gieben <miek@miek.nl>

miekg · 2019-03-07T20:35:11Z

quay.io is broken sigh

@localhost

Add a ready plugin that allows plugin to signal when they are ready. Once a plugin is ready it is not queried again. This uses same mechanism as the health plugin: each plugin needs to implement an interface. Implement readines for the *erratic* plugin to aid in testing. Add README.md and tests moduled after the health plugin; which will be relegated to just providing process health. In similar vein to health this is a process wide setting. With this Corefile: ~~~ . { erratic whoami ready } bla { erratic whoami } ~~~ ready will lead to: ~~~ sh % curl localhost:8181/ready % dig @localhost -p 1053 mx example.org % curl localhost:8181/ready OK% ~~~ Meanwhile CoreDNS logs: ~~~ .:1053 bla.:1053 2019-02-26T20:59:07.137Z [INFO] CoreDNS-1.3.1 2019-02-26T20:59:07.137Z [INFO] linux/amd64, go1.11.4, CoreDNS-1.3.1 linux/amd64, go1.11.4, 2019-02-26T20:59:11.415Z [INFO] plugin/ready: Still waiting on: "erratic" 2019-02-26T20:59:13.510Z [INFO] plugin/ready: Still waiting on: "erratic" ~~~ *ready* can be used in multiple server blocks and will do the right thing; query all those plugins from all server blocks for readiness. This does a similar thing to the prometheus plugin. Signed-off-by: Miek Gieben <miek@miek.nl>

@localhost

Add a ready plugin that allows plugin to signal when they are ready. Once a plugin is ready it is not queried again. This uses same mechanism as the health plugin: each plugin needs to implement an interface. Implement readines for the *erratic* plugin to aid in testing. Add README.md and tests moduled after the health plugin; which will be relegated to just providing process health. In similar vein to health this is a process wide setting. With this Corefile: ~~~ . { erratic whoami ready } bla { erratic whoami } ~~~ ready will lead to: ~~~ sh % curl localhost:8181/ready % dig @localhost -p 1053 mx example.org % curl localhost:8181/ready OK% ~~~ Meanwhile CoreDNS logs: ~~~ .:1053 bla.:1053 2019-02-26T20:59:07.137Z [INFO] CoreDNS-1.3.1 2019-02-26T20:59:07.137Z [INFO] linux/amd64, go1.11.4, CoreDNS-1.3.1 linux/amd64, go1.11.4, 2019-02-26T20:59:11.415Z [INFO] plugin/ready: Still waiting on: "erratic" 2019-02-26T20:59:13.510Z [INFO] plugin/ready: Still waiting on: "erratic" ~~~ *ready* can be used in multiple server blocks and will do the right thing; query all those plugins from all server blocks for readiness. This does a similar thing to the prometheus plugin. Signed-off-by: Miek Gieben <miek@miek.nl>

corbot bot requested a review from greenpau February 25, 2019 16:16

fturib reviewed Feb 25, 2019

View reviewed changes

miekg force-pushed the ready branch from 52b75aa to 5a3af45 Compare February 26, 2019 21:01

greenpau approved these changes Feb 26, 2019

View reviewed changes

chrisohaver approved these changes Feb 26, 2019

View reviewed changes

johnbelamaric reviewed Feb 26, 2019

View reviewed changes

miekg force-pushed the ready branch from d6c6f40 to 63d27a1 Compare February 27, 2019 07:55

miekg force-pushed the ready branch from 63d27a1 to 30e326c Compare February 28, 2019 16:46

corbot bot approved these changes Mar 1, 2019

View reviewed changes

miekg force-pushed the ready branch from 30e326c to 4506b56 Compare March 7, 2019 20:22

miekg merged commit db0b16b into master Mar 7, 2019

corbot bot deleted the ready branch March 7, 2019 20:35

miekg mentioned this pull request Mar 7, 2019

add ready plugin #2611

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ready plugin #2616

Add ready plugin #2616

miekg commented Feb 25, 2019

corbot bot commented Feb 25, 2019

fturib Feb 25, 2019

miekg commented Feb 25, 2019 via email

codecov-io commented Feb 26, 2019 •

edited

Loading

chrisohaver commented Feb 26, 2019

miekg commented Feb 26, 2019

miekg commented Feb 26, 2019

johnbelamaric Feb 26, 2019

chrisohaver Feb 27, 2019

miekg commented Feb 26, 2019 via email

miekg commented Feb 27, 2019 via email

miekg commented Mar 1, 2019

johnbelamaric commented Mar 1, 2019

corbot bot left a comment

miekg commented Mar 7, 2019

Add *ready* plugin #2616

Add *ready* plugin #2616

Conversation

miekg commented Feb 25, 2019

corbot bot commented Feb 25, 2019

fturib Feb 25, 2019

Choose a reason for hiding this comment

miekg commented Feb 25, 2019 via email

codecov-io commented Feb 26, 2019 • edited Loading

Codecov Report

chrisohaver commented Feb 26, 2019

miekg commented Feb 26, 2019

miekg commented Feb 26, 2019

johnbelamaric Feb 26, 2019

Choose a reason for hiding this comment

chrisohaver Feb 27, 2019

Choose a reason for hiding this comment

miekg commented Feb 26, 2019 via email

miekg commented Feb 27, 2019 via email

miekg commented Mar 1, 2019

johnbelamaric commented Mar 1, 2019

corbot bot left a comment

Choose a reason for hiding this comment

miekg commented Mar 7, 2019

Add ready plugin #2616

Add ready plugin #2616

codecov-io commented Feb 26, 2019 •

edited

Loading