Throw startup error if `TeleportReadyEvent` is not emitted #11725

vitorenesduarte · 2022-04-05T12:26:54Z

Before this PR, the TeleportReadyEvent was only waited for when a process reload occurred. Thus, if a bug exists in the code that emits this event (as it's currently the case since the MetricsReady and WindowsDesktopReady events are never emitted - #11724), such a bug may go unnoticed for a while.

This PR ensures that the TeleportReadyEvent is always waited for on startup, and throws an error if the event is not emitted (after some timeout).

This PR also:

removes the MetricsReady event (as this is not produced by a component that sends heartbeats, which is the case of every other event required by the TeleportReadyEvent event mapping)
ensures that WindowsDesktopReady event is emitted
refactors some of the code in lib/service/supervisor.go
moves the event mapping registration to a new registerTeleportReadyEvent function

Testing

None.

Before this commit, the `TeleportReadyEvent` was only waited for when a process reload occurred. Thus, if a bug exists in the code that emits this event (as it's currently the case since the `MetricsReady` and `WindowsDesktopReady` events are never emitted), such a bug may go unnoticed for a while. This commit ensures that the `TeleportReadyEvent` is always waited for on startup, and throws an error if the event is not emitted (after some timeout). This commit also: - removes the `MetricsReady` event (as this is not produced by a component that sends heartbeats, which is the case of every other event required by the `TeleportReadyEvent` event mapping) - ensures that `WindowsDesktopReady` event is emitted - refactors some of the code in `lib/service/supervisor.go`

espadolini

I tried to find objections to not waiting for the new process to emit TeleportReady before shutting down the old one but I couldn't find any

lib/service/supervisor.go

smallinsky · 2022-04-05T13:01:55Z

lib/service/service.go

+		// Wait for the service to report that it has started.
+		startTimeoutCtx, startCancel := context.WithTimeout(ctx, signalPipeTimeout)
+		defer startCancel()
+		eventC := make(chan Event, 1)
+		srv.WaitForEvent(startTimeoutCtx, TeleportReadyEvent, eventC)
+		select {
+		case <-eventC:
+			cfg.Log.Infof("Service has started successfully.")
+		case <-startTimeoutCtx.Done():
+			warnOnErr(srv.Close(), cfg.Log)
+			return trace.BadParameter("service has failed to start")
+		}


it looks that waitAndReload logic already waits for TeleportReadyEvent so after exiting waitAndReload the logic triggers second wait for TeleportReadyEvent event but it the TeleportReadyEvent event was ready broadcasted.

Ah, good point. In a way this is ok since an already broadcast event will still be emitted if waited on, but there might be a better alternative.

Doesn't this PR remove that WaitForEvent?

(I think that a similar situation would already occur if two process reloads were triggered.)

Doesn't this PR remove that WaitForEvent?

You're right. Sorry @smallinsky @espadolini, I misunderstood what waitAndReload was doing.

I pushed a new change (f73af18) to ensure that we always for the the TeleportReadyEvent after calling LocalSupervisor.Start() (both on the initial startup, and after a process reload), reverting the changes to waitAndReload.

significant changes

vitorenesduarte · 2022-04-06T14:11:49Z

CI seems to be passing now. Could you please take another look @espadolini?

…#11249) Fixes #11065. This commit: - ensures that `TeleportReadyEvent` is only produced when all components that send heartbeats (i.e. call [`process.onHeartbeat`](https://github.com/gravitational/teleport/blob/16bf416556f337b045b66dc9c3f5a3e16f8cc988/lib/service/service.go#L358-L366)) are ready - changes `TeleportProcess.registerTeleportReadyEvent` so that it returns a count of these components (let's call it `componentCount`) - uses `componentCount` to also ensure that `stateOK` is only reported when all the components have sent their heartbeat, thus fixing #11065 Since it seems difficult to know when `TeleportProcess.registerTeleportReadyEvent` should be updated, with the goal of quickly detecting a bug when it's introduced we have that: 1. if `componentCount` is lower than it should, then the service fails to start (due to #11725) 2. if `componentCount` is higher than it should, then an error is logged in function `processState.getStateLocked`.

* Throw startup error if `TeleportReadyEvent` is not emitted Before this commit, the `TeleportReadyEvent` was only waited for when a process reload occurred. Thus, if a bug exists in the code that emits this event (as it's currently the case since the `MetricsReady` and `WindowsDesktopReady` events are never emitted), such a bug may go unnoticed for a while. This commit ensures that the `TeleportReadyEvent` is always waited for on startup, and throws an error if the event is not emitted (after some timeout). This commit also: - removes the `MetricsReady` event (as this is not produced by a component that sends heartbeats, which is the case of every other event required by the `TeleportReadyEvent` event mapping) - ensures that `WindowsDesktopReady` event is emitted - refactors some of the code in `lib/service/supervisor.go` - moves the event mapping registration to a new `registerTeleportReadyEvent` function

…#11249) Fixes #11065. This commit: - ensures that `TeleportReadyEvent` is only produced when all components that send heartbeats (i.e. call [`process.onHeartbeat`](https://github.com/gravitational/teleport/blob/16bf416556f337b045b66dc9c3f5a3e16f8cc988/lib/service/service.go#L358-L366)) are ready - changes `TeleportProcess.registerTeleportReadyEvent` so that it returns a count of these components (let's call it `componentCount`) - uses `componentCount` to also ensure that `stateOK` is only reported when all the components have sent their heartbeat, thus fixing #11065 Since it seems difficult to know when `TeleportProcess.registerTeleportReadyEvent` should be updated, with the goal of quickly detecting a bug when it's introduced we have that: 1. if `componentCount` is lower than it should, then the service fails to start (due to #11725) 2. if `componentCount` is higher than it should, then an error is logged in function `processState.getStateLocked`.

* Throw startup error if `TeleportReadyEvent` is not emitted Before this commit, the `TeleportReadyEvent` was only waited for when a process reload occurred. Thus, if a bug exists in the code that emits this event (as it's currently the case since the `MetricsReady` and `WindowsDesktopReady` events are never emitted), such a bug may go unnoticed for a while. This commit ensures that the `TeleportReadyEvent` is always waited for on startup, and throws an error if the event is not emitted (after some timeout). This commit also: - removes the `MetricsReady` event (as this is not produced by a component that sends heartbeats, which is the case of every other event required by the `TeleportReadyEvent` event mapping) - ensures that `WindowsDesktopReady` event is emitted - refactors some of the code in `lib/service/supervisor.go` - moves the event mapping registration to a new `registerTeleportReadyEvent` function

…#11249) Fixes #11065. This commit: - ensures that `TeleportReadyEvent` is only produced when all components that send heartbeats (i.e. call [`process.onHeartbeat`](https://github.com/gravitational/teleport/blob/16bf416556f337b045b66dc9c3f5a3e16f8cc988/lib/service/service.go#L358-L366)) are ready - changes `TeleportProcess.registerTeleportReadyEvent` so that it returns a count of these components (let's call it `componentCount`) - uses `componentCount` to also ensure that `stateOK` is only reported when all the components have sent their heartbeat, thus fixing #11065 Since it seems difficult to know when `TeleportProcess.registerTeleportReadyEvent` should be updated, with the goal of quickly detecting a bug when it's introduced we have that: 1. if `componentCount` is lower than it should, then the service fails to start (due to #11725) 2. if `componentCount` is higher than it should, then an error is logged in function `processState.getStateLocked`.

* Throw startup error if `TeleportReadyEvent` is not emitted (#11725) * Throw startup error if `TeleportReadyEvent` is not emitted Before this commit, the `TeleportReadyEvent` was only waited for when a process reload occurred. Thus, if a bug exists in the code that emits this event (as it's currently the case since the `MetricsReady` and `WindowsDesktopReady` events are never emitted), such a bug may go unnoticed for a while. This commit ensures that the `TeleportReadyEvent` is always waited for on startup, and throws an error if the event is not emitted (after some timeout). This commit also: - removes the `MetricsReady` event (as this is not produced by a component that sends heartbeats, which is the case of every other event required by the `TeleportReadyEvent` event mapping) - ensures that `WindowsDesktopReady` event is emitted - refactors some of the code in `lib/service/supervisor.go` - moves the event mapping registration to a new `registerTeleportReadyEvent` function * Ensure stateOK is reported only when all components have sent updates (#11249) Fixes #11065. This commit: - ensures that `TeleportReadyEvent` is only produced when all components that send heartbeats (i.e. call [`process.onHeartbeat`](https://github.com/gravitational/teleport/blob/16bf416556f337b045b66dc9c3f5a3e16f8cc988/lib/service/service.go#L358-L366)) are ready - changes `TeleportProcess.registerTeleportReadyEvent` so that it returns a count of these components (let's call it `componentCount`) - uses `componentCount` to also ensure that `stateOK` is only reported when all the components have sent their heartbeat, thus fixing #11065 Since it seems difficult to know when `TeleportProcess.registerTeleportReadyEvent` should be updated, with the goal of quickly detecting a bug when it's introduced we have that: 1. if `componentCount` is lower than it should, then the service fails to start (due to #11725) 2. if `componentCount` is higher than it should, then an error is logged in function `processState.getStateLocked`. * Make `PortList.Pop()` thread-safe (#11799)

This reverts commit ea8ee94.

This reverts commit 48eedcd.

…11725)" This reverts commit 933e247.

…2242) * Revert "Backport #11725 #11249 #11799 to branch/v8 (#11794)" This reverts commit 48eedcd. * Revert "Fix ProxyKube not reporting its readiness (#12153)" This reverts commit b924b40.

* Revert "Make `PortList.Pop()` thread-safe (#11799)" This reverts commit a17337d. * Revert "Ensure stateOK is reported only when all components have sent updates (#11249)" This reverts commit b749302. * Revert "Throw startup error if `TeleportReadyEvent` is not emitted (#11725)" This reverts commit 933e247. * Revert "Fix ProxyKube not reporting its readiness (#12150)" This reverts commit 6cdcfe7.

* Revert "Backport #11725 #11249 #11799 to branch/v9 (#11795)" This reverts commit ea8ee94. * Revert "Fix ProxyKube not reporting its readiness (#12152)" This reverts commit ce301f0.

vitorenesduarte requested review from zmb3, r0mant and smallinsky April 5, 2022 12:27

vitorenesduarte self-assigned this Apr 5, 2022

github-actions bot requested a review from xacrimon April 5, 2022 12:27

vitorenesduarte mentioned this pull request Apr 5, 2022

Ensure stateOK is reported only when all components have sent updates #11249

Merged

vitorenesduarte force-pushed the vitor/teleport-ready-event branch from 72f4452 to 1c76b73 Compare April 5, 2022 13:04

espadolini approved these changes Apr 5, 2022

View reviewed changes

lib/service/supervisor.go Outdated Show resolved Hide resolved

lib/service/supervisor.go Outdated Show resolved Hide resolved

smallinsky reviewed Apr 5, 2022

View reviewed changes

Vitor Enes added 2 commits April 5, 2022 14:12

Remove unnecessary return

3ed15a5

Move waiter.notify function below waiter's definition

e6210d7

smallinsky approved these changes Apr 5, 2022

View reviewed changes

Ensure TeleportReadyEvent is waited on after LocalSupervisor.Start

f73af18

espadolini previously approved these changes Apr 5, 2022

View reviewed changes

xacrimon approved these changes Apr 5, 2022

View reviewed changes

Vitor Enes added 4 commits April 6, 2022 10:43

Merge branch 'master' into vitor/teleport-ready-event

01ed564

Fix failing TestIntegrations/RotateChangeSigningAlg test

f8bddcf

Move event mapping registration to registerTeleportReadyEvent function

c548d38

Simplify TeleInstance.Start

4228881

espadolini self-requested a review April 6, 2022 13:23

Merge branch 'master' into vitor/teleport-ready-event

3f9577e

espadolini approved these changes Apr 6, 2022

View reviewed changes

vitorenesduarte merged commit 933e247 into master Apr 6, 2022

vitorenesduarte deleted the vitor/teleport-ready-event branch April 6, 2022 15:10

This was referenced Apr 7, 2022

Backport #11725 #11249 #11799 to branch/v8 #11794

Merged

Backport #11725 #11249 #11799 to branch/v9 #11795

Merged

webvictim mentioned this pull request Apr 19, 2022

opened in error #12068

Closed

r0mant added a commit that referenced this pull request Apr 26, 2022

Revert "Backport #11725 #11249 #11799 to branch/v9 (#11795)"

c4376d0

This reverts commit ea8ee94.

r0mant added a commit that referenced this pull request Apr 26, 2022

Revert "Backport #11725 #11249 #11799 to branch/v8 (#11794)"

0588b69

This reverts commit 48eedcd.

r0mant added a commit that referenced this pull request Apr 26, 2022

Revert "Throw startup error if TeleportReadyEvent is not emitted (#…

aab4d04

…11725)" This reverts commit 933e247.

r0mant mentioned this pull request Apr 26, 2022

Revert readyz changes #12244

Merged

webvictim mentioned this pull request Jun 8, 2022

Opened in error #13285

Closed

alexatcanva mentioned this pull request Oct 13, 2022

BUGFIX | Fix Teleport ALPN Proxy not being HTTP CONNECT Proxy Aware alexatcanva/teleport#30

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw startup error if `TeleportReadyEvent` is not emitted #11725

Throw startup error if `TeleportReadyEvent` is not emitted #11725

vitorenesduarte commented Apr 5, 2022 •

edited

Loading

espadolini left a comment

smallinsky Apr 5, 2022

vitorenesduarte Apr 5, 2022

espadolini Apr 5, 2022

vitorenesduarte Apr 5, 2022

vitorenesduarte Apr 5, 2022

vitorenesduarte commented Apr 6, 2022

Throw startup error if TeleportReadyEvent is not emitted #11725

Throw startup error if TeleportReadyEvent is not emitted #11725

Conversation

vitorenesduarte commented Apr 5, 2022 • edited Loading

Testing

espadolini left a comment

Choose a reason for hiding this comment

smallinsky Apr 5, 2022

Choose a reason for hiding this comment

vitorenesduarte Apr 5, 2022

Choose a reason for hiding this comment

espadolini Apr 5, 2022

Choose a reason for hiding this comment

vitorenesduarte Apr 5, 2022

Choose a reason for hiding this comment

vitorenesduarte Apr 5, 2022

Choose a reason for hiding this comment

vitorenesduarte commented Apr 6, 2022

Throw startup error if `TeleportReadyEvent` is not emitted #11725

Throw startup error if `TeleportReadyEvent` is not emitted #11725

vitorenesduarte commented Apr 5, 2022 •

edited

Loading