bakaid commented on a change in pull request #734: MINIFICPP-1157 Implement
lightweight C2 heartbeat.
URL: https://github.com/apache/nifi-minifi-cpp/pull/734#discussion_r379520331
##########
File path: libminifi/src/c2/C2Agent.cpp
##########
@@ -315,16 +317,35 @@ void C2Agent::performHeartBeat() {
payload.addPayload(std::move(deviceInfo));
}
- if (!root_response_nodes_.empty()) {
- for (auto metric : root_response_nodes_) {
- C2Payload child_metric_payload(Operation::HEARTBEAT);
- child_metric_payload.setLabel(metric.first);
- if (metric.second->isArray()) {
- child_metric_payload.setContainer(true);
- }
- serializeMetrics(child_metric_payload, metric.first,
metric.second->serialize(), metric.second->isArray());
- payload.addPayload(std::move(child_metric_payload));
+ for (auto metric : root_response_nodes_) {
+ C2Payload child_metric_payload(Operation::HEARTBEAT);
+ bool isArray{false};
+ std::string metricName;
+ std::vector<state::response::SerializedResponseNode> metrics;
+ std::shared_ptr<state::response::NodeReporter> reporter;
+ std::shared_ptr<state::response::ResponseNode> agentInfoManifest;
+
+ //Send agent manifest in first heartbeat
Review comment:
> May be the function "getAgentInformationWithManifest" name is confusing to
you. If you look at this function, it instantiates "AgentInformation" class.
Will change the name to "getAgentInformation" instead.
Yep, it did confuse me, thanks for the clarification.
Let me unpack the other parts of why this does not feel right (while
describing my understanding of the processes behind our C2 implementation).
The classes we want to use for C2 responses are defined in the
`nifi.c2.root.classes` property.
These are then instantiated using our classloader architecture by
`FlowController::initializeC2`, static casted into
`state::response::ResponseNode`s and stored in root_response_nodes_ .
3 checks are made on these objects using dynamic casts:
- if they implement `state::response::AgentIdentifier`, the agent
identifier and class are set on them
- if they implement `state::response::AgentMonitor`, the provenance and
flowfile repos and the `FlowController` itself are made known to them
- if they implement `state::response::FlowMonitor` flow-related information
and the `FlowController` itself is made known to them
We don't assume anything about these classes other than based on what
interfaces they implement and we only interact with them through these
interfaces.
These response nodes are exposed through `FlowController::getResponseNodes`
(which overrides `StateManager::getResponseNodes`), and get fed into the
C2Agent through a convoluted scheme involving registering the C2Agent as an
`UpdateController` via `registerUpdateListener` and a TreeUpdateListener
through which the `FlowController::getResponseNodes()` is looped back into
`FlowController::setResponseNodes` (which in reality is
`StateManager::setResponseNodes`, because `FlowController` does not override
that function), which will call `setResponseNodes` on all registered
UpdateControllers, thereby calling `C2Agent::setResponseNodes`, completing a
section of the Rube Goldberg machine that we call our C2 implementation.
The only thing `C2Agent` did so far was to call `serializeMetrics` on these
when needed by `performHeartBeat`. `serializeMetrics` depends only on the
interfaces implemented by the defined response classes: all behaviour is
defined by them.
What your change does, is if the CoreComponent name of the one of these
classes match `agentInfo` (which is the name of both
`AgentInformationWithoutManifest` and `AgentInformation`) overrides the
behaviour of an class defined by the user in `nifi.c2.root.classes` and
instantiated by `FlowController` by creating a new instance of
`AgentInformation` via `FlowController::getAgentInformation` and using that for
the first heartbeat irrespective of what the configured class is.
This, instead of depending on interfaces and letting the behaviour stay at
level of the response classes, changes behaviour at the level of the `C2Agent`,
based on the name of the response node instance, something which we did not
depend on so far. It also makes us expose a hardcoded object factory on
`FlowController` for the `AgentInformation` class
(`FlowController::getAgentInformation()`); something that was not needed so
far, as everything was done dynamically through our classloader.
I do not have a perfect solution for this, and I do not know if a perfect
solution exists in the current architecture.
We could change`AgentInformationWithoutManifest` to include the manifest on
its first `serialize` call, but that would not be a solution: we cannot be sure
and we should not depend on whether that call was done by the C2Agent and
whether the resulting heartbeat was successfully delivered, thereby really
negating the need to include the manifest in further serializations.
I see two main root causes for this:
- C2Agent sometimes needs a different set of information, but the
information is only accessible through a composite object (`AgentInformation`
or `AgentInformationWithoutManifest`) which contains a hardcoded set of
information
- we have no method of communicating the needs of the `C2Agent` to
`AgentInformation`, therefore we can't request a particular set of information
(in this case `C2Agent` would specify the set of information needed), nor do we
have the ability to give feedback about the need for including a particular set
of information (in this case `AgentInformation` would be able to decide what
information to include based on the state of `C2Agent`, namely whether the
first heartbeat has been successfully sent)
If we can find no better solution I can live with this until we change the
architecture, but we will have to consider how to refactor the C2 architecture
to better fit our needs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
