Refactor agent followup #700
Conversation
diconico07
requested review from
bfjelds,
kate-goldenring,
jiria,
Britel,
romoh,
adithyaj and
johnsonshih
as code owners
agent/src/discovery_handler_manager/discovery_handler_registry.rs
Outdated
Show resolved
Hide resolved
agent/src/discovery_handler_manager/discovery_handler_registry.rs
Outdated
Show resolved
Hide resolved
agent/src/discovery_handler_manager/discovery_handler_registry.rs
Outdated
Show resolved
Hide resolved
agent/src/discovery_handler_manager/discovery_handler_registry.rs
Outdated
Show resolved
Hide resolved
| break on Discovery Handler connection error | ||
| Registration endpoint -x DiscoveryHandlerRegistry: close endpoint | ||
| DiscoveryHandlerRegistry ->> DiscoveryHandlerRegistry: Remove endpoint from registered handlers list | ||
| note over DiscoveryHandlerRequest,Discovery Handler: The DiscoveryHandlerRequest request will handle termination by itself |
There was a problem hiding this comment.
What does this mean?
There was a problem hiding this comment.
it means that we do not propagate the fact the discovery handler is not reachable as every active request will notice that by themselves and handle this.
| sequenceDiagram | ||
| Configuration Controller -) DiscoveryHandlerRegistry: new_request() | ||
| alt a Handler exists for the Request | ||
| DiscoveryHandlerRegistry ->> DiscoveryHandlerRequest: Creates with filtered list of endpoints |
There was a problem hiding this comment.
"endpoints" is a little overloaded here. Is this DH endpoints to device filtering that is passed to the DH as discovery properties?
There was a problem hiding this comment.
I looked at the DHRequestImpl structure which defines endpoints as discovered devices but i dont think that is what this diagram is referring to by "endpoints"
There was a problem hiding this comment.
Well, there is a bit of type passing here, the DHRequestImpl defines an endpoint as a receiver for discovered devices, that is the output stream from a discovery handler's endpoint connection.
So endpoint for DHRequestImpl is an open connection to a discovery handler, and in here to create that we pass the list of discovery handlers endpoints that have the right name (hence filtered list)
There was a problem hiding this comment.
Can we reword this, then, to clarify what filtered means. For example "Creates a discover connection with each DH for a Configuration"
| DiscoveryHandlerRegistry ->> DiscoveryHandlerRequest: Creates with filtered list of endpoints | ||
| DiscoveryHandlerRegistry ->> DiscoveryHandlerRegistry: Add request to tracked request list | ||
| loop over DiscoveryHandlerEndpoints with this name | ||
| DiscoveryHandlerRequest ->>+ Kubernetes API: Solve discovery properties |
There was a problem hiding this comment.
same point as my previous comment about reqording this step
| DiscoveryHandlerRequest -) Configuration Controller: Requests reconciliation of Configuration linked to Request | ||
| note over Configuration Controller: The following is Configuration Controller behavior | ||
| activate Configuration Controller | ||
| Configuration Controller ->> Configuration Controller: Reconcile Configuration |
There was a problem hiding this comment.
What does Reconcile Configuration mean? Is this where we are making sure that the appropriate set of Akri Instances exists?
There was a problem hiding this comment.
This is just what the configuration controller do, so ensuring there is a request for the configuration and that the appropriate set of Instances exists.
At first I didn't want to put it in that diagram as it is outside of the discovery handler manager, but did it in the end to highlight the get_request() and get_instances() use.
| @@ -67,12 +67,6 @@ spec: | |||
| - name: DEBUG_ECHO_INSTANCES_SHARED | |||
| value: {{ .Values.debugEcho.configuration.shared | quote }} | |||
| {{- end }} | |||
| - name: HOST_CRICTL_PATH | |||
diconico07
force-pushed
the
refactor-agent-followup
branch
from
1aff5b6
to
51855d1
Compare
Signed-off-by: Nicolas Belouin <[email protected]>
New slot reconciliation system doesn't need crictl anymore, removing everything related to that Signed-off-by: Nicolas Belouin <[email protected]>
Signed-off-by: Kate Goldenring <[email protected]> Signed-off-by: Nicolas Belouin <[email protected]> Co-authored-by: Kate Goldenring <[email protected]>
Signed-off-by: Nicolas Belouin <[email protected]>
diconico07
force-pushed
the
refactor-agent-followup
branch
from
51855d1
to
d8e50ad
Compare
Signed-off-by: Nicolas Belouin <[email protected]>
What this PR does / why we need it:
This PR adds some documentation for the agent's discovery handler as asked in #684 .
It also does a lot of cleanup related to the new slot reclaiming system: as the new slot reclaiming process doesn't rely on crictl nor the containerd socket, we can remove those from the agent's definition, it also removes the need for specifying the kubernetes distribution during installation.
Special notes for your reviewer:
Documentation PR: project-akri/akri-docs#113
If applicable:
cargo fmt)cargo build)cargo clippy)cargo test)cargo doc)