-
Notifications
You must be signed in to change notification settings - Fork 150
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
CoAP discovery handler #346
Conversation
@@ -6,7 +6,19 @@ metadata: | |||
spec: | |||
discoveryHandler: | |||
name: {{ required "A custom.configuration.discoveryHandlerName is required." .Values.custom.configuration.discoveryHandlerName }} | |||
discoveryDetails: {{ .Values.custom.configuration.discoveryDetails }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for custom-configuration, i think the intent is that .Values.custom.configuration.discoveryDetails be yaml that can implicitly be what you are describing explicitly below. if we make the explicit changes you have here, custom-configuration becomes coap-configuration (and can no longer be other configs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems more like the start of a deployment/helm/templates/coap-configuration.yaml :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the custom.configuration
values should be added to a coap
section of the values file: https://github.com/deislabs/akri/blob/main/deployment/helm/values.yaml. A coap-discovery-handler.yaml
file should also be added instead of using the custom one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this great work @jiayihu! See my comments for my main questions around multicast discovery and async coap client. Also, can you add a coap-configuration.md document under the docs folder to explain installation and the discoveryDetails
parameters provided for filtering? For example, here is the one for ONVIF. Finally, are you interested in CoAP being able to be compiled into the agent? If so, I can point out where those changes need to happen in a subsequent PR.
|
||
The Broker acts as an HTTP-to-CoAP Proxy. It translates RESTful HTTP requests into RESTful CoAP requests and vice versa for the response. "Cross-Protocol Proxying between CoAP and HTTP" is defined in section 10 of RFC 7252. Currently, the Broker forwards only GET requests. | ||
|
||
The Broker is also in an excellent position to cache CoAP responses. "Unlike HTTP, the cacheability of CoAP responses does not depend on the request method, but it depends on the Response Code." Currently, the Broker only caches CoAP responses with status code equal to `2.05 Content`, which is returned if the device is not reachable during the connection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove extra � character here: 2.05 Content
�
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
- Is there a better way to store the discovered resources as Configuration in the cluster? | ||
|
||
The current implementation would need a controller to accept queries about available resources and return the name of the Broker's service which can communicate to the device. The device is listed as a generic `akri.sh/coap-021dd7` resource on the node, which is too generic to be useful by any application. A better label would be `akri.sh/oic.r.temperature-021dd7`, the discovered resource. This would allow using the K8s controller for scheduling pods that need the resource. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this, someone could deploy a configuration named oir.r.tempurature and filter for specifically temperature devices right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is definitely an idea but the number of resource types is quite huge. Besides, as cluster administrator, who deploys the configurations, you may not know them in advance. Imagine you tasked to set up the clusters of a company with several geographical locations. It also means that you need two separate configurations, thus duplicate discovery handlers, for a same device that exposes both temperature and brightness as resources.
With that being said, I like the idea. It should also already be supported by using the queryFilter
discoveryDetail like this:
queryFilter:
name: "rt" // resource type
value: "oic.r.temperature"
It's part of the CoAP standard that the devices can supported filtering by using a query filter. Moreover, my understanding of the RFC is that it supports only one filter. Even though I've implemented it, I never actually thought about using it for my use case 😄 It remains a workaround local deployments though, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you mean by ", thus duplicate discovery handlers"? There would still only be one coap DH running on the node but it would be called twice, once for each config
|
||
### With Akri | ||
|
||
The Akri Configuration defines a list of IP addresses to use for resource discovery, implementing thus unicast discovery. Likewise, a multicast IP address can be used for multicast discovery. Both methods can be used at the same time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a multicast example? Does an ip address always have to be provided or can a network scan be performed? Also static ip address are more of an aliveness check and resources supported by that device check rather than discovery, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A multicast discovery is just a normal CoAP request sent to the special IP address 224.0.1.187
(in the case of IPv4) and with the broadcast
flag on the IP packet. CoAP devices should be instructed to reply if they receive that kind of broadcast packet. The discovery handler only supports IPv4 multicast for now.
Static IP addresses are more for debugging or local clusters where you know which devices are available. I don't expect them to be used for real-world cases. That's why the default values.yml
use multicast. For instance I use the static IP of my laptop during development, the IP of my local embedded device when actually testing the whole system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds great!
discovery-handler-modules/coap-discovery-handler/Dockerfile.discovery-handler
Outdated
Show resolved
Hide resolved
@jiayihu can you add some documentation on how to test your discovery handler? Do you happen to have a way you are mocking the coap devices? |
Sure, I'll add more documentation. To mock the CoAP devices you can use coap-rs (I personally use node-coap because it's quicker to use an interpreted language for testing). Should I just provide the instructions in a |
Are you referring to only documentation on mocking the devices? What would go in the "project" you're suggesting? A |
@jiayihu give us a bump when you have addressed the comments and think this is ready for another round of reviews. |
# Conflicts: # Cargo.lock
@kate-goldenring I think this is finally ready for a second review 🎉 |
Hi @jiayihu, this is great to see! I've been on vacation the past few days, but I'll start taking a look this week! Another PR with docs on the docs site is perfect. |
let query_filter = query_filter.clone(); | ||
|
||
tokio::task::spawn_blocking(move || { | ||
let mut shared_devices = shared_devices.lock().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe wait to lock shared_devices
after making the call to discovery_endpoint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding in the configuration file, dockerfiles, and deployment yaml! Added some questions and comments about settings and docs. Haven't looked into the implementation yet. I'll look through that next.
memoryRequest: 10Mi | ||
# cpuRequest defines the minimum amount of CPU that must be available to this Pod | ||
# for it to be scheduled by the Kubernetes Scheduler | ||
cpuRequest: 10m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably do an analysis using the vertical pod autoscaler as we did with other Akri components to determine good estimates here. Or we could leave out requests and limits for now and add an issue to do that.
capacity: 1 | ||
discoveryDetails: | ||
multicast: true | ||
multicastIpAddress: 224.0.1.187 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the default for coap as explained in the configuration doc, should it ever be configurable?
{{- end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this some auto-linting? Can you undo to this and other irrelevant changes to other files.
spec: | ||
containers: | ||
- name: akri-coap-discovery | ||
image: {{ printf "%s:%s" (required "A coap.discovery.image.repository is required." .Values.coap.discovery.image.repository) .Values.coap.discovery.image.tag | quote }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Akri will host the image. set the value as done in reference udev-discovery-handler.yaml for an example
{{- else }} | ||
queryFilter: null | ||
{{- end }} | ||
discoveryTimeoutSeconds: {{ .Values.coap.configuration.discoveryDetails.discoveryTimeoutSeconds }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this used?
# Copy over container legal notice | ||
COPY ./build/container-images-legal-notice.md . | ||
|
||
RUN apt-get update && apt-get install -y --no-install-recommends libssl-dev openssl && apt-get clean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with broker: are libssl-dev and openssl needed? I dont remember why they were necessary in all of the other containers
|
||
RUN apt-get update && apt-get install -y --no-install-recommends libssl-dev openssl && apt-get clean | ||
COPY ./target/${CROSS_BUILD_TARGET}/${BUILD_TYPE}/coap-broker /coap-broker | ||
ENV RUST_LOG=coap_broker,api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the api
package?
|
||
## Deploying the CoAP Discovery Handler | ||
|
||
In order for the Agent to know how to discover CoAP servers an CoAP Discovery Handler must exist. Akri supports an Agent image that includes all supported Discovery Handlers. This Agent will be used if `agent.full=true`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation doesn't support embedding coap in the Agent. A subsequent PR can add that support. For now, this section can be simplified to "The CoAP Discovery Handlers are deployed by specifying coap.discovery.enabled=true
when installing Akri."
|
||
Discovery Handlers are passed discovery details that are set in a Configuration to determine what to discover, filter out of discovery, and so on. | ||
|
||
By default, the CoAP Discovery Handler doesn't require any additional information as it implements the multicast CoAP discovery (described in [Section 8 of RFC 7252](https://datatracker.ietf.org/doc/html/rfc7252#section-8)). The IPv4 address reserved for "All CoAP Nodes" is `224.0.1.187`. Devices must support the default port `5683` as specified by the standard. At the time of writing, IPv6 multicast is not supported in the CoAP Discovery Handler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this IPv4 address ever be configurable? When would it change?
|---|---|---|---| | ||
| coap.configuration.discoveryDetails.multicast | boolean | true | Enable IPv4 multicast discovery | | ||
| coap.configuration.discoveryDetails.multicastIpAddress | string | 224.0.1.187 | The IPv4 to which the Discovery Handler sends the packets | | ||
| coap.configuration.discoveryDetails.staticIpAddresses | Array of strings | [] | Additional static IP addresses to look for during the discovery. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a description of when to use coap.configuration.discoveryDetails.staticIpAddresses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just finished looking through everything and wow! This is some awesome work. The coap-demo.md
was smooth. I was able to get through it without any problems! I've made a lot of comments about nits and conventions, but overall this is looking great. One more push and we should be there. Once you have addressed the docs comments, go ahead and put a PR into akri-docs. Another note, maybe run cargo clippy -p coap-discovery-handler
and cargo clippy -p coap-broker
to grab any syntax fixes.
|
||
### Broker Pod Settings | ||
|
||
If you would like workloads ("broker" Pods) to be deployed automatically to discovered devices, a broker image should be specified in the Configuration. Alternatively, if it meets your scenario, you could use the Akri's default CoAP broker ("ghcr.io/deislabs/akri/coap-broker"). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove "the" in "you could use the Akri's default CoAP broker"
|
||
### Other settings | ||
|
||
The CoAP Discovery Handlers supports the same "Capacity" and "Automatic Service Creation" as OPC UA. Refer to the latter [documentation](https://github.com/deislabs/akri/blob/main/docs/opcua-configuration.md#disabling-automatic-service-creation) for additional information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is supported by all discovery handlers, but we are repeating the setting in each configuration file so they can be standalone. Can you copy and past that section into here, chaning opcua
to coap
where applicable?
|
||
fn discover_endpoint( | ||
coap_client: &impl CoAPClient, | ||
ip_address: &String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you aren't using &str
?
akri-discovery-utils = { path = "../../discovery-utils" } | ||
anyhow = "1.0.41" | ||
async-trait = "0.1.51" | ||
coap = { git = "https://github.com/Covertness/coap-rs" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we point to their crate instead of GitHub repo. coap = "0.11"
?
@@ -62,7 +62,7 @@ jobs: | |||
BUILD_ARM64: 0 | |||
BUILD_SLIM_AGENT: 0 | |||
AGENT_FEATURES: "agent-full" | |||
PACKAGES_TO_EXCLUDE: "akri-udev akri-onvif akri-opcua udev-video-broker debug-echo-discovery-handler onvif-discovery-handler opcua-discovery-handler udev-discovery-handler" | |||
PACKAGES_TO_EXCLUDE: "akri-udev akri-onvif akri-opcua udev-video-broker coap-discovery-handler debug-echo-discovery-handler onvif-discovery-handler opcua-discovery-handler udev-discovery-handler" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isnt needed, since it isnt a package in the Agent
## Deploy a Kubernetes application which requests the temperature via HTTP | ||
|
||
The Akri CoAP built-in Broker Pod is automatically deployed when an instance is created. The Broker exposes a RESTful endpoint which translates HTTP request to CoAP requests. To inspect the associated Kubernetes Service, run `kubectl get crd`. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be kubectl get services
?
``` | ||
kubectl run curl --image=radial/busyboxplus:curl -i --tty | ||
|
||
[ root@curl:/ ]$ curl http://akri-coap-920f97-svc/sensors/temp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
specify that the id varies and should be changed when curling
coap-lite = "0.5" | ||
env_logger = "0.8.4" | ||
log = "0.4.14" | ||
tokio = { version = "1", features = ["full"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: specify specific features to minimize unused code
|
||
[dependencies] | ||
akri-shared = { path = "../../../shared" } | ||
coap = { git = "https://github.com/Covertness/coap-rs" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
specify crate instead of github
| coap.configuration.discoveryDetails.multicast | boolean | true | Enable IPv4 multicast discovery | | ||
| coap.configuration.discoveryDetails.multicastIpAddress | string | 224.0.1.187 | The IPv4 to which the Discovery Handler sends the packets | | ||
| coap.configuration.discoveryDetails.staticIpAddresses | Array of strings | [] | Additional static IP addresses to look for during the discovery. | | ||
| coap.configuration.discoveryDetails.queryFilter | { name: string, value: string } | {} | Single name-value pair to filter the discovered resource, as described in [Section 4.1 of RFC 6690](https://datatracker.ietf.org/doc/html/rfc6690#section-4.1) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe break this out into two different fields for clarity, since it seems like the following is the appropriate was to configure the filter:
--set coap.configuration.discoveryDetails.queryFilter.name="rt" \
--set coap.configuration.discoveryDetails.queryFilter.value="oic.r.temperature"
@@ -8,13 +8,16 @@ members = [ | |||
"shared", | |||
"agent", | |||
"controller", | |||
"samples/apps/coap-device", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this should live in akri as it is a mock device. I feel like the coap-demo.md
is sufficient for getting people going. Or we could put it on a branch of akri, or you could host it if interested. @bfjelds for thoughts
serde_json = "1.0.45" | ||
serde_derive = "1.0.104" | ||
serde_yaml = "0.8.17" | ||
tokio = { version = "0.2", features = ["rt-threaded", "sync", "time", "stream", "fs", "macros", "uds"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: We recently went to tokio 1.0. Could you bump to that version? It isnt required since it is not embedded in the agent.
@@ -28,8 +28,6 @@ spec: | |||
cpu: {{ .Values.debugEcho.configuration.brokerPod.resources.cpuRequest }} | |||
limits: | |||
{{`"{{PLACEHOLDER}}"`}} : "1" | |||
memory: {{ .Values.debugEcho.configuration.brokerPod.resources.memoryLimit }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was this removed?
name = "coap-discovery-handler" | ||
version = "0.6.8" | ||
authors = ["Jiayi Hu <[email protected]>"] | ||
edition = "2018" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: 2018? should this be 2021?
@@ -0,0 +1,19 @@ | |||
[package] | |||
name = "coap-broker" | |||
version = "0.1.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the versions match our overall versioning ("0.6.14" ... or better yet "0.7.0")?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and update version.sh for each new Cargo.toml verion location?
there are a lot of |
Thank you to both of you for the comments. Unfortunately, vacation has ended and I'm not able to go back to the PR right now 🙃 It might take a while again |
PR has been automatically marked as stale due to inactivity for 90 days. Update the PR to remove label, otherwise it will be automatically closed." |
@jiayihu, there have been a couple of changes since your PR. Let us know if you need help with rebase once you are ready to work on this again., we'll be glad to help. |
PR has been automatically marked as stale due to inactivity for 90 days. Update the PR to remove label, otherwise it will be automatically closed." |
PR has been automatically marked as stale due to inactivity for 90 days. Update the PR to remove label, otherwise it will be automatically closed." |
What this PR does / why we need it: it adds support for the CoAP protocol, which is an important protocol for RESTful IoT devices.
Special notes for your reviewer:
If applicable:
cargo fmt
)cargo build
)cargo clippy
)cargo test
)cargo doc
)./version.sh
)