Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor agent followup #700

Merged
merged 5 commits into from
Sep 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 21 additions & 14 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions agent/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "agent"
version = "0.13.1"
version = "0.13.2"
license = "Apache-2.0"
authors = ["Kate Goldenring <[email protected]>", "<[email protected]>"]
edition = "2021"
Expand Down Expand Up @@ -35,6 +35,7 @@ serde = "1.0.104"
serde_derive = "1.0.104"
serde_json = "1.0.45"
serde_yaml = { version = "0.8.11", optional = true }
simple-mermaid = "0.1" # used for docs
thiserror = "1.0.50"
tokio = { version = "1.0", features = ["rt-multi-thread", "time", "fs", "macros", "net"] }
tokio-stream = { version = "0.1", features = ["net", "sync"] }
Expand Down Expand Up @@ -65,4 +66,4 @@ default = []
onvif-feat = [ "akri-onvif"]
opcua-feat = ["akri-opcua"]
udev-feat = ["akri-udev"]
agent-full = ["serde_yaml", "akri-debug-echo"]
agent-full = ["serde_yaml", "akri-debug-echo"]
14 changes: 14 additions & 0 deletions agent/src/discovery_handler_manager/diagrams/dh_device.mmd
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
sequenceDiagram
Discovery Handler ->> DiscoveryHandlerRequest: send discovered devices
DiscoveryHandlerRequest ->> DiscoveryHandlerRequest: Updates aggregated list of discovered devices
DiscoveryHandlerRequest -) Device Manager: Notifies and updates list of discovered devices
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does Reconcile Configuration mean? Is this where we are making sure that the appropriate set of Akri Instances exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Configuration Controller ->> DiscoveryHandlerRegistry: get_request()
DiscoveryHandlerRegistry ->> Configuration Controller:
Configuration Controller ->> DiscoveryHandlerRequest: get_instances()
DiscoveryHandlerRequest ->> Configuration Controller: Returns list of discovered devices as bare Instances
Configuration Controller ->> Kubernetes API: Apply Instances
deactivate Configuration Controller
18 changes: 18 additions & 0 deletions agent/src/discovery_handler_manager/diagrams/dh_query.mmd
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
sequenceDiagram
Configuration Controller -) DiscoveryHandlerRegistry: new_request()
alt a Handler exists for the Request
DiscoveryHandlerRegistry ->> DiscoveryHandlerRequest: Creates with filtered list of endpoints
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"endpoints" is a little overloaded here. Is this DH endpoints to device filtering that is passed to the DH as discovery properties?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we reword this, then, to clarify what filtered means. For example "Creates a discover connection with each DH for a Configuration"

DiscoveryHandlerRegistry ->> DiscoveryHandlerRegistry: Add request to tracked request list
loop over DiscoveryHandlerEndpoints with this name
DiscoveryHandlerRequest ->>+ Kubernetes API: Solve discovery properties
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same point as my previous comment about reqording this step

Kubernetes API ->>- DiscoveryHandlerRequest:
DiscoveryHandlerRequest ->>+ Discovery Handler: query discovery handler
loop
Discovery Handler ->> DiscoveryHandlerRequest: send discovered devices
note over DiscoveryHandlerRequest,DiscoveryHandlerRegistry: See other diagram for what happens here
end
deactivate Discovery Handler
end
else
DiscoveryHandlerRegistry -x Configuration Controller: DiscoveryError::NoHandler
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be good to clarify that this the the flow for a single Akri Config / DiscoveryHandlerRequest

Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
sequenceDiagram
Discovery Handler ->> Registration endpoint: Registers
Registration endpoint->> DiscoveryHandlerRegistry: register_endpoint()
DiscoveryHandlerRegistry ->> DiscoveryHandlerRegistry: Add endpoint to registered handlers list
DiscoveryHandlerRegistry ->> DiscoveryHandlerRequest: notify all DiscoveryHandlerRequest
alt Discovery Handler name is the same as in Request
DiscoveryHandlerRequest ->>+ Kubernetes API: Solve discovery properties
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could use some elaboration. Is this the equivalent of "[For each Akri Config for the DH] Get discovery handler properties (filters) from Akri Configuration"?

Suggested change
DiscoveryHandlerRequest ->>+ Kubernetes API: Solve discovery properties
DiscoveryHandlerRequest ->>+ Kubernetes API: Look up discovery properties in Akri Configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here solving discovery properties is looking up for config maps and secrets that are in the discovery_properties field as required, this is handled in details by the discovery_property_solver module

Kubernetes API ->>- DiscoveryHandlerRequest:
DiscoveryHandlerRequest ->>+ Discovery Handler: query discovery handler
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DiscoveryHandlerRequest ->>+ Discovery Handler: query discovery handler
DiscoveryHandlerRequest ->>+ Discovery Handler: query discovery handler passing discovery properties for the Akri Configuration

loop
Discovery Handler ->> DiscoveryHandlerRequest: send discovered devices
note over DiscoveryHandlerRequest,DiscoveryHandlerRegistry: See other diagram for what happens here
end
deactivate Discovery Handler
end
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end
Original file line number Diff line number Diff line change
@@ -1,3 +1,22 @@
//! This module is the heart of the discovery process handled by the agent, it is based around the [DiscoveryHandlerRegistry]
//! and uses several other structure to represent and help handle discovery related operations.
//!
//! The [DiscoveryHandlerRegistry] keeps track of registered discovery handlers. Note, multiple endpoints/instances of a given
//! handler can be registered at the same time.
//!
//! The [DiscoveryHandlerRegistry] also keeps track of ongoing discovery requests against those discovery handlers. There is one discovery request (a [DiscoveryHandlerRequest] object) per Configuration.
//!
//! Here are some simple diagrams showing how the components interact with each other in different situations:
//!
//! A new DiscoverHandler gets registered (after it connects to and registers with the agent registration Unix socket):
#![doc=simple_mermaid::mermaid!("diagrams/dh_registration.mmd")]
//!
//! A new query is made by the Configuration Controller:
#![doc=simple_mermaid::mermaid!("diagrams/dh_query.mmd")]
//!
//! A Discovery Handler's instance/endpoint sends a new list of discovered devices for a Request:
#![doc=simple_mermaid::mermaid!("diagrams/dh_device.mmd")]

use std::collections::HashMap;
use std::sync::Arc;

Expand Down
5 changes: 1 addition & 4 deletions build/containers/Dockerfile.rust
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,7 @@ ARG AKRI_COMPONENT
RUN PROFILE=$(echo "${EXTRA_CARGO_ARGS}" | grep -q -- --release && echo "release" || echo "debug"); \
xx-verify ./target/$(xx-cargo --print-target-triple)/${PROFILE}/${AKRI_COMPONENT}\
&& cp ./target/$(xx-cargo --print-target-triple)/${PROFILE}/${AKRI_COMPONENT} /build/bin/akri
# Prepare crictl for agent
RUN VERSION=v1.25.0; if [ "x${AKRI_COMPONENT}" = "xagent" ]; then wget \
"https://github.com/kubernetes-sigs/cri-tools/releases/download/$VERSION/crictl-$VERSION-linux-$(xx-info arch).tar.gz" -O crictl.tar.gz\
&& tar zxvf crictl.tar.gz -C /build/bin; fi



FROM scratch
Expand Down
2 changes: 1 addition & 1 deletion controller/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "controller"
version = "0.13.1"
version = "0.13.2"
license = "Apache-2.0"
authors = ["<[email protected]>", "<[email protected]>"]
edition = "2021"
Expand Down
4 changes: 2 additions & 2 deletions deployment/helm/Chart.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ type: application
# This is the chart version. This version number should be incremented each time you make changes
# to the chart and its templates, including the app version.
# Versions are expected to follow Semantic Versioning (https://semver.org/)
version: 0.13.1
version: 0.13.2

# This is the version number of the application being deployed. This version number should be
# incremented each time you make changes to the application. Versions are not expected to
# follow Semantic Versioning. They should reflect the version the application is using.
appVersion: 0.13.1
appVersion: 0.13.2
25 changes: 0 additions & 25 deletions deployment/helm/templates/agent.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,6 @@ spec:
- name: DEBUG_ECHO_INSTANCES_SHARED
value: {{ .Values.debugEcho.configuration.shared | quote }}
{{- end }}
- name: HOST_CRICTL_PATH
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woohooo 🎉

value: /usr/local/bin/crictl
- name: HOST_RUNTIME_ENDPOINT
value: unix:///host/run/containerd/containerd.sock
- name: HOST_IMAGE_ENDPOINT
value: unix:///host/run/containerd/containerd.sock
- name: AGENT_NODE_NAME
valueFrom:
fieldRef:
Expand All @@ -86,8 +80,6 @@ spec:
mountPath: /var/lib/kubelet/device-plugins
- name: pod-resources
mountPath: /var/lib/kubelet/pod-resources
- name: var-run-dockershim
mountPath: /host/run/containerd/containerd.sock
{{- if .Values.agent.host.udev }}
- name: devices
mountPath: /run/udev
Expand All @@ -111,23 +103,6 @@ spec:
- name: pod-resources
hostPath:
path: "{{ .Values.agent.host.kubeletPodResources }}"
- name: var-run-dockershim
hostPath:
{{- if ne "" .Values.agent.host.containerRuntimeSocket }}
path: {{.Values.agent.host.containerRuntimeSocket }}
{{- else if eq .Values.kubernetesDistro "microk8s" }}
path: "/var/snap/microk8s/common/run/containerd.sock"
{{- else if eq .Values.kubernetesDistro "k3s" }}
path: "/run/k3s/containerd/containerd.sock"
{{- else if eq .Values.kubernetesDistro "k8s" }}
path: "/run/containerd/containerd.sock"
{{- else }}
# Please set container runtime socket by either selecting the appropriate K8s distro `kubernetesDistro=<k8s|k3s|microk8s>`
# or setting `agent.host.containerRuntimeSocket=/container/runtime.sock`.
# See https://docs.akri.sh/user-guide/cluster-setup for more information.
# Using K8s default "/run/containerd/containerd.sock" for now.
path: "/run/containerd/containerd.sock"
{{- end }}
{{- if .Values.agent.host.udev }}
- name: devices
hostPath:
Expand Down
9 changes: 0 additions & 9 deletions deployment/helm/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,6 @@ useDevelopmentContainers: true
# This can be set from the helm command line using `--set imagePullSecrets[0].name="mysecret"`
imagePullSecrets: []

# kubernetesDistro describes the Kubernetes distro Akri is running on. It is used to conditionally set
# distribution specific values such as container runtime socket. Options: microk8s | k3s | k8s
kubernetesDistro: ""

# generalize references to `apiGroups` and `apiVersion` values for Akri CRDs
crds:
group: akri.sh
Expand Down Expand Up @@ -109,11 +105,6 @@ agent:
kubeletDevicePlugins: /var/lib/kubelet/device-plugins
# kubeletPodResources is the location of the kubelet pod-resources socket
kubeletPodResources: /var/lib/kubelet/pod-resources
# containerRuntimeSocket is the default node path of the container runtime socket.
# For MicroK8s, set to "/var/snap/microk8s/common/run/containerd.sock"
# For K3s, set to "/run/k3s/containerd/containerd.sock"
# For standard K8s, set to "/run/containerd/containerd.sock"
containerRuntimeSocket: ""
# udev is the node path of udev, usually at `/run/udev`
udev:
# allowDebugEcho dictates whether the Akri Agent will allow DebugEcho Configurations
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "debug-echo-discovery-handler"
version = "0.13.1"
version = "0.13.2"
license = "Apache-2.0"
authors = ["Kate Goldenring <[email protected]>"]
edition = "2021"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "onvif-discovery-handler"
version = "0.13.1"
version = "0.13.2"
license = "Apache-2.0"
authors = ["Kate Goldenring <[email protected]>"]
edition = "2021"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "opcua-discovery-handler"
version = "0.13.1"
version = "0.13.2"
license = "Apache-2.0"
authors = ["Kate Goldenring <[email protected]>"]
edition = "2021"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "udev-discovery-handler"
version = "0.13.1"
version = "0.13.2"
license = "Apache-2.0"
authors = ["Kate Goldenring <[email protected]>"]
edition = "2021"
Expand Down
2 changes: 1 addition & 1 deletion discovery-handlers/debug-echo/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "akri-debug-echo"
version = "0.13.1"
version = "0.13.2"
license = "Apache-2.0"
authors = ["Kate Goldenring <[email protected]>"]
edition = "2021"
Expand Down
2 changes: 1 addition & 1 deletion discovery-handlers/onvif/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "akri-onvif"
version = "0.13.1"
version = "0.13.2"
license = "Apache-2.0"
authors = ["Kate Goldenring <[email protected]>"]
edition = "2021"
Expand Down
Loading
Loading