From f9ac03d6e3a6bcd33e6169dae1e695af5789e6c9 Mon Sep 17 00:00:00 2001 From: Christian Kreibich Date: Mon, 31 Jan 2022 17:40:23 -0800 Subject: [PATCH] Simplify ClusterController::API::set_configuration_request/response It's easier to track outstanding controller/agent requests via a simple set of pending agent names, and we can remove all of the result aggregation logic since we can simply re-use the results reported by the agents. This can serve as a template for request-response patterns where a client's request triggers a request to all agents, followed by a response to the client once all agents have responded. Once we have a few more of those, it'll become clearer how to abstract this further. --- .../frameworks/cluster/controller/main.zeek | 70 +++++-------------- .../cluster/controller/request.zeek | 2 +- 2 files changed, 20 insertions(+), 52 deletions(-) diff --git a/scripts/policy/frameworks/cluster/controller/main.zeek b/scripts/policy/frameworks/cluster/controller/main.zeek index 33e0456049..d96b49d58f 100644 --- a/scripts/policy/frameworks/cluster/controller/main.zeek +++ b/scripts/policy/frameworks/cluster/controller/main.zeek @@ -70,9 +70,9 @@ function send_config_to_agents(req: ClusterController::Request::Request, areq$parent_id = req$id; # We track the requests sent off to each agent. As the - # responses come in, we can check them off as completed, - # and once all are, we respond back to the client. - req$set_configuration_state$requests += areq; + # responses come in, we delete them. Once the requests + # set is empty, we respond back to the client. + add req$set_configuration_state$requests[areq$id]; # We could also broadcast just once on the agent prefix, but # explicit request/response pairs for each agent seems cleaner. @@ -283,62 +283,30 @@ event ClusterAgent::API::set_configuration_response(reqid: string, result: Clust if ( ClusterController::Request::is_null(areq) ) return; - # Record the result and mark the request as done. This also - # marks the request as done in the parent-level request, since - # these records are stored by reference. - areq$results[0] = result; # We only have a single result here atm - areq$finished = T; + # Release the request, which is now done. + ClusterController::Request::finish(areq$id); - # Update the original request from the client: + # Find the original request from the client local req = ClusterController::Request::lookup(areq$parent_id); if ( ClusterController::Request::is_null(req) ) return; - # If there are any requests to the agents still unfinished, - # we're not done yet. - for ( i in req$set_configuration_state$requests ) - if ( ! req$set_configuration_state$requests[i]$finished ) - return; + # Add this result to the overall response + req$results[|req$results|] = result; - # All set_configuration requests to instances are done, so respond - # back to client. We need to compose the result, aggregating - # the results we got from the requests to the agents. In the - # end we have one Result per instance requested in the - # original set_configuration_request. - # - # XXX we can likely generalize result aggregation in the request module. - for ( i in req$set_configuration_state$requests ) - { - local r = req$set_configuration_state$requests[i]; + # Mark this request as done by removing it from the table of pending + # ones. The following if-check should always be true. + if ( areq$id in req$set_configuration_state$requests ) + delete req$set_configuration_state$requests[areq$id]; - local success = T; - local errors: string_vec; - local instance = ""; + # If there are any pending requests to the agents, we're + # done: we respond once every agent has responed (or we time out). + if ( |req$set_configuration_state$requests| > 0 ) + return; - for ( j in r$results ) - { - local res = r$results[j]; - instance = res$instance; - - if ( res$success ) - next; - - success = F; - errors += fmt("node %s failed: %s", res$node, res$error); - } - - req$results += ClusterController::Types::Result( - $reqid = req$id, - $instance = instance, - $success = success, - $error = join_string_vec(errors, ", ") - ); - - ClusterController::Request::finish(r$id); - } - - # We're now done with the original set_configuration request. - # Adopt the configuration as the current one. + # All set_configuration requests to instances are done, so adopt the + # client's requested configuration as the new one and respond back to + # client. g_config_current = req$set_configuration_state$config; g_config_reqid_pending = ""; diff --git a/scripts/policy/frameworks/cluster/controller/request.zeek b/scripts/policy/frameworks/cluster/controller/request.zeek index 202a615e6b..987dc9338c 100644 --- a/scripts/policy/frameworks/cluster/controller/request.zeek +++ b/scripts/policy/frameworks/cluster/controller/request.zeek @@ -28,7 +28,7 @@ export { # State specific to the set_configuration request/response events type SetConfigurationState: record { config: ClusterController::Types::Configuration; - requests: vector of Request &default=vector(); + requests: set[string] &default=set(); }; # State specific to supervisor interactions