update to heartbeats

Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>

beps/0005-split-backend-discovery/README.md

@@ -99,7 +99,7 @@ A node that serves as the primary discovery point. A gateway node will have all
 
 #### Non-gateway Node
 
-A node that needs to call a Gateway node for routing outside of its own plugins.
+A node that needs to call a Gateway node for routing outside of its own plugins. This node will still fire requests to other instances, but does not know which instances have which plugins.
 
 ### New Discovery Plugin
 
@@ -113,36 +113,51 @@ info:
 paths:
   /register:
     post:
-      summary: Register an instance with the gateway.
-      operationId: registerInstance
+      summary: Register a plugin with the gateway.
+      operationId: registerPlugin
       requestBody:
-        description: Create a new pet in the store
         content:
           application/json:
             schema:
               type: object
               properties:
-                instanceUrl:
+                pluginId:
+                  type: string
+                externalUrl:
+                  type: string
+                internalUrl:
                   type: string
-                plugins:
-                  type: object
-                  # Map of pluginId to external/internal URL.
-                  additionalProperties:
-                    type: object
-                    properties:
-                      externalUrl:
-                        type: string
-                      internalUrl:
-                        type: string
         required: true
       responses:
         '200':
           description: Successful operation
         '400':
           description: Invalid input
+  /unregister:
+    post:
+      summary: Unregister a plugin with the gateway.
+      operationId: unregisterPlugin
+      requestBody:
+        content:
+          application/json:
+            schema:
+              type: object
+              properties:
+                pluginId:
+                  type: string
+                externalUrl:
+                  type: string
+                internalUrl:
+                  type: string
+        required: true
+      responses:
+        '200':
+          description: Successful operation
+        '400':
+          description: Invalid input or already unregistered.
   /registrations:
     get:
-      summary: Get all registered plugins
+      summary: Get all registered pluginIds
       operationId: listRegistrations
       responses:
         '200':
@@ -206,7 +221,7 @@ This new method is expected to call the gateway node's `discovery://registration
 
 On startup, _instances_ will send a request to the gateway node, `discovery://register` with information about the instance and what plugins it owns. This request must be signed using service-to-service auth keys between the two instances to prevent malicious registrations. We may revisit this in the future.
 
-#### New `InstanceMetadataService`
+### New `InstanceMetadataService`
 
 While we could attach the existing information to the `PluginMetadataService`, we propose a new service that handles instance-level information. The existing `PluginMetadataService` should reveal information about the plugin itself, its `pluginId`, dependencies or similar. The new `InstanceMetadataService` should give you information about the entire Backstage instance that you're interrogating. At launch, this should include the list of features installed _on your instance_ that can then be aggregated by the discovery API across the gateway nodes. One could imagine this service also having information about instance URLs, health or gateway status.
 
@@ -242,7 +257,7 @@ export interface PluginRegistrations {
   plugin_id: string;
   internal_url: string;
   external_url: string;
-  last_check_in: timestamp;
+  last_health_check: timestamp;
 }
 ```
 
@@ -250,22 +265,24 @@ As any gateway node _could_ be hit by any given plugin, the implementation shoul
 
 The triplet `plugin_id`, `internal_url`, `external_url` should be unique. We may have multiple plugins on multiple URLs either internal or external. Routing in those cases is not covered in this BEP. Horizontally scaled plugins should use external technologies to route requests and handle load balancing. This should be reflected in their `backend.baseUrl` properties. Instance IP addresses should _not_ be sent or stored in this database.
 
-### Check ins
+### Heartbeats
 
-To prevent stale data, we propose implementing "check ins". Each check in will write to the database's `last_check_in` row, these will be infrequent enough that this isn't a crazy load on the database. These check ins serve 2 purposes,
+To prevent stale data, we propose implementing heartbeats. Each plugin should expose a `/health` endpoint (this will be configurable per plugin). When registering, the plugin will send its internal and external reachable URLs to the gateway node. After registering, every x seconds the gateway node will send a heartbeat request to the health endpoint. That endpoint is expected to return a 200 HTTP response. If it doesn't respond with a 200 or doesn't respond within a reasonable timeout, that plugin will be considered unregistered at its instance URL. If this represents the last instance URL for a given plugin, the entire plugin will be considered unavailable.
 
-1. to verify that the gateway node is reachable by the non-gateway node.
-1. to verify that the gateway node has the most up to date data from the non-gateway node.
+The goal with adopting heartbeats is to leverage the decentralized nature of health checks per plugin to enable horizontal scaling easily. Only a single instance has to respond to a health check for it to pass.
 
-The first is necessary to prevent a case where the non-gateway node loses network access or crashes. In this case, we will use the `last_check_in` timestamp. If an instance hasn't contacted the gateway plugin in x check-ins (or seconds), we remove that instance's plugins. This will most likely be an optimistic update and will happen at read time for other operations. So the database may still have entries with expired `last_check_in` rows if the discovery API is not actively used.
+These would initially be naively implemented, using in memory intervals on each gateway node that a plugin registers with. This brings with it 2 risks that would result in lost traffic.
 
-The second is necessary to prevent a case where an instance may restart with more/less plugins. The gateway needs a way of knowing that the instance data changed. This is why we propose a "check in" approach over just a heartbeat. The non-gateway plugin should send its list of plugins to the gateway plugin to check if anything has changed since it last checked in. This allows us to be much more specific about when the instance will re-register.
+1. The gateway node handling heartbeats crashes.
+1. The instance handling traffic crashes.
 
-In the case of horizontally scaled plugins, we should be able to keep the current check-ins, but we may revisit if the volume is too high.
+Both may cause a temporary outage in plugin traffic.
+
+<!-- TODO: Figure out a good story around error handling and restoring state. -->
 
 ### Unregistering Plugins
 
-While some service discovery implementation have the ability for plugins to unregister themselves on shutdown or error, this proves difficult for horizontally scaled deployments. As such, we propose using the check ins described above to verify that a plugin is operational.
+Each plugin will send a request to the gateway node's `/unregister` endpoint on shutdown. Any responses will not block the shutdown process. This will remove the plugin/instance combination from the database. As we have to consider horizontally scaled deployments, this may be triggered multiple times for a given plugin/instance combination. For duplicates, the endpoint will send a 400.
 
 ## Release Plan
 
@@ -289,6 +306,6 @@ not need to be as detailed as the proposal, but should include enough
 information to express the idea and why it was not acceptable.
 -->
 
-## Per-plugin registration
+## Per-instance registration
 
-Assuming that instances restart when plugins are added and removed -- as is currently the case -- this doesn't give much benefit. However, if plugins are able to be installed dynamically and it becomes difficult to maintain an accurate list at the instance-level, we should revisit this.
+This would involve sending a request on instance startup with a list of the current plugins and information about how to route to them. While this is useful with the instance restarts on every plugin addition, it has issues with unregistering plugins when the instance crashes and doesn't restart and causes extra stress on the gateway node in comparison to a pure health check/heartbeat.