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

Make instance ID attachment to broker properties optional #704

Closed
kate-goldenring opened this issue Oct 15, 2024 · 5 comments · Fixed by #710
Closed

Make instance ID attachment to broker properties optional #704

kate-goldenring opened this issue Oct 15, 2024 · 5 comments · Fixed by #710

Comments

@kate-goldenring
Copy link
Contributor

Problem

In Akri, Discovery Handlers return device connectivity information as properties that are set as environment variables in containers. Since a broker may wish to use more than one discovered device, we later decided to attach the instance hash to the end of these env var to enable attaching multiple devices of information to the pod. The issue with this is that you now cannot read a known env var, say ONVIF_DEVICE_MAC_ADDRESS; rather you must list all env vars with a prefix to find ONVIF_DEVICE_MAC_ADDRESS_<INSTANCE_ID>.

Docs: https://docs.akri.sh/development/broker-development#discovery-handler-specified-environment-variables

This poses problems for programs that read environment variables at start up. For example, I have been working on integrations of Akri with Spin to bring together IoT and Wasm. In this case, Spin listens to an MQTT topic that it reads from an environment variable. Here is a repo that i have created with the integration with workarounds.

Potential solution 1: add default env var [preferred by author]

One simple solution is to also set information in environment variables without an instance id suffix (just ONVIF_DEVICE_MAC_ADDRESS), so if a broker is given device information about 2 ONVIF cameras, the information would be set as follows:

ONVIF_DEVICE_MAC_ADDRESS=MAC1
ONVIF_DEVICE_MAC_ADDRESS_ID1=MAC1
ONVIF_DEVICE_MAC_ADDRESS_ID2=MAC2

For brokers using more than one device of a config, it would not be recommended to use the default ONVIF_DEVICE_MAC_ADDRESS env var.

Potential solution 2: more deterministic suffixes

A more deterministic solution could also be to use ordered numbers and provide an additional env var that declares quantity of instances of that config. For example, if a broker is given device information about 2 ONVIF cameras, the information would be set as follows:

ONVIF_DEVICE_MAC_ADDRESS_1=MAC1
ONVIF_DEVICE_MAC_ADDRESS_2=MAC2
ONVIF_DEVICES=2

The con to this is that it is a breaking change to how instance information is accessed today, plus instance IDs are deterministic so brokers can target specific instances using the previous approach.

@agracey
Copy link

agracey commented Oct 16, 2024

I like solution 1 but would suggest adding a “well known” field that lists the IDs.

While this could definitely be added later instead of in this issue, it feels like it would likely be editing the same code and easier to add now.

@diconico07
Copy link
Contributor

I also like solution 1, we should just ensure that we don't end up "changing" the "anonymous" device if re-applying the pod manifest.

About the "well known" field listing IDs, I think it should be done in another PR/Issue as I think there is more to discuss on this, as for example is it better to list the IDs or the Instance name, or should we have a list per Configuration, ...

@agracey
Copy link

agracey commented Oct 17, 2024

About the "well known" field listing IDs, I think it should be done in another PR/Issue

Makes sense to me

Another thought (for later discussion as well) is that I'm wondering if mounting the config in as a file is better than environment variables since it allows for the pod to listen for config changes and stay running. I know this is anti-cloud-native but we are talking about control systems where the workloads are likely stateful anyways (at least in terms of binding to hardware)

@kate-goldenring
Copy link
Contributor Author

I also like solution 1, we should just ensure that we don't end up "changing" the "anonymous" device if re-applying the pod manifest.

@diconico07 I am not sure there is a way to do this deterministically. Say you request two ip cameras in your pod, the kubelet issues independent allocate calls to each device plugin. There is no way to determine the order.

I think we can document that this is not deterministic and the non-hashed env should only be used by brokers using one device per DH

@kate-goldenring
Copy link
Contributor Author

I also checked the kubelet device plugin manager implementation to see if it does allocate calls deterministically. Alas, it just iterates through the external resource limits for a container and issues allocate calls, and that list is a map (type ResourceList map[ResourceName]resource.Quantity ) which is not deterministically ordered when iterating. Though i will say in my test of N=3, the same device info is set in the Pod for the nonsuffixed env regardless of resource request order.

Example deployment:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: temp
  labels:
    app: temp
spec:
  replicas: 1
  selector:
    matchLabels:
      app: temp
  template:
    metadata:
      labels:
        app: temp
    spec:
      containers:
      - name: temp
        image: nginx
        resources:
          limits:                    
            akri.sh/akri-debug-echo-foo-ab76e6: "1"    # swap order on subsequent test
            akri.sh/akri-debug-echo-foo-d46b65: "1"
          requests:
            akri.sh/akri-debug-echo-foo-ab76e6: "1"
            akri.sh/akri-debug-echo-foo-d46b65: "1"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants