plugins, runtime: Add visibility for server init #3706
Conversation
|
I confirmed this change resolves our problem. |
|
Thank you! I'll have another look tomorrow. |
| @@ -441,6 +441,7 @@ func (rt *Runtime) Serve(ctx context.Context) error { | |||
| rt.serverInitMtx.Lock() | |||
| rt.serverInitialized = true | |||
| rt.serverInitMtx.Unlock() | |||
| rt.Manager.ServerInitialized() | |||
There was a problem hiding this comment.
I think there's a race condition here because there's no guarantee that the socket will have been opened.
The serverLoop() call on L430 (above) eventually calls down into net.Listen here and at that's what actually calls socket() and listen() at the OS level (so requests sent in the meantime will get network error.) That said, the window is extremely small; so perhaps that's fine. The solution would be to have this code wait until the server was actually listening for connections but that would require some more invasive changes to the listener interface (e.g., the serverLoop() function could return a channel or something to do indicate the listener was opened...)
@gshively11 if you agree w/ the above and are okay with the potential race condition, just include a comment here.
There was a problem hiding this comment.
Ah, good catch. I think we're probably OK with this race condition, I'll add the comment. If it does end up being an issue, a modification as you described shouldn't be a breaking change, so it should be easy to implement if we need it.
| @@ -340,6 +340,34 @@ func TestPluginManagerConsoleLogger(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestPluginManagerServerInitialized(t *testing.T) { | |||
There was a problem hiding this comment.
Nit: It would be great if you could add a comment as to what you're trying to test in the 2 test cases below. It's not clear to me at least.
There was a problem hiding this comment.
Thanks, I somehow missed you weren't calling ServerInitialized() in the second case.
There was a problem hiding this comment.
Oh, I definitely was calling it lol, and it was passing because I was erroneously using return above it instead of break. So good catch 😁
There was a problem hiding this comment.
LGTM. Just noticed one small future-proofing improvement on the manager change. @gshively11 if you add that in, we can merge this.
plugins/plugins.go
Outdated
| // ServerInitialized signals a channel indicating that the OPA | ||
| // server has finished initialization. | ||
| func (m *Manager) ServerInitialized() { | ||
| close(m.serverInitialized) |
There was a problem hiding this comment.
nit: Just realized that it would be easy to make this function idempotent. It's only ever being called once from inside of the runtime package but if someone else was instantiating the manager and somehow called this function twice, they would get a panic due to the channel already being closed. We could use a sync.Once primitive here?
- Plugins can now access a channel which receives a message when the OPA server is fully initialized and ready to receive traffic. - Added ServerInitialized() and ServerInitializedChannel() to plugin manager. - Runtime now calls ServerInitialized() when server listeners are initialized. Fixes #3701 Signed-off-by: Grant Shively <gshively@godaddy.com>
server is fully initialized and ready to receive traffic.
manager.
initialized.
Fixes #3701
(Tests will be implemented after review of implementation)
Signed-off-by: Grant Shively gshively@godaddy.com