Mount udev devpath in Akri brokers

[kate-goldenring]

What this PR does / why we need it:
fixes #521

Currently, Akri defines a udev device by its devnode; however, not all devices have nodes. This changes the device identifier to by devpath and creates a new environment variable UDEV_DEVPATH which is injected into brokers.

Special notes for your reviewer:
@agracey does this fix your issue of using the dev rule of SUBSYSTEM=="drm", ATTR{status}=="connected" with Akri?

Docs will need to be updated:

• https://docs.akri.sh/discovery-handlers/udev#specifying-a-broker-pod-image
• https://docs.akri.sh/development/broker-development#discovery-handler-specified-environment-variables

If applicable:

• this PR has an associated PR with documentation in akri-docs
• this PR contains unit tests
• added code adheres to standard Rust formatting (cargo fmt)
• code builds properly (cargo build)
• code is free of common mistakes (cargo clippy)
• all Akri tests succeed (cargo test)
• inline documentation builds (cargo doc)
• all commits pass the DCO bot check by being signed off -- see the failing DCO check for instructions on how to retroactively sign commits

Signed-off-by: Kate Goldenring [email protected]

[kate-goldenring]

I was able to confirm that this change enables the dynamic discovery of an HDMI monitor.
Currently, the following does not discover the monitor because devpath /devices/pci0000:00/0000:00:02.0/drm/card0/card0-HDMI-A-1 does not have a devnode:

helm install akri akri-helm-charts/akri \
     $AKRI_HELM_CRICTL_CONFIGURATION \
     --set udev.configuration.enabled=true \
     --set udev.discovery.enabled=true \
     --set udev.configuration.name=akri-hdmi \
     --set udev.configuration.discoveryDetails.udevRules[0]='SUBSYSTEM=="drm"\, ATTR{status}=="connected"\, KERNEL=="*HDMI*"'

Running these changes locally and deploying the chart without the udev discovery handler:

helm install akri akri-helm-charts/akri \
     $AKRI_HELM_CRICTL_CONFIGURATION \
     --set udev.configuration.enabled=true \
     --set udev.configuration.name=akri-hdmi \
     --set udev.configuration.discoveryDetails.udevRules[0]='SUBSYSTEM=="drm"\, ATTR{status}=="connected"\, KERNEL=="*HDMI*"'

And in another terminal

sudo RUST_LOG=info DISCOVERY_HANDLERS_DIRECTORY=/var/lib/akri AGENT_NODE_NAME=kagold-thinkpad-x1-carbon-6th ./target/release/udev-discovery-handler

Resultant Akri instance:

apiVersion: v1
items:
- apiVersion: akri.sh/v0
  kind: Instance
  metadata:
    creationTimestamp: "2022-11-02T19:28:39Z"
    generation: 1
    name: akri-hdmi-cc5ea6
    namespace: default
    ownerReferences:
    - apiVersion: akri.sh/v0
      blockOwnerDeletion: true
      controller: true
      kind: Configuration
      name: akri-hdmi
      uid: 6d6714eb-d010-441c-9866-671366729f57
    resourceVersion: "5469"
    uid: be443857-889c-49dc-bb43-8f19abed3f1b
  spec:
    brokerProperties:
      UDEV_DEVPATH: /devices/pci0000:00/0000:00:02.0/drm/card0/card0-HDMI-A-1
    configurationName: akri-hdmi
    deviceUsage:
      akri-hdmi-cc5ea6-0: ""
    nodes:
    - kagold-thinkpad-x1-carbon-6th
    shared: false
kind: List
metadata:
  resourceVersion: ""

Instance disappears when HDMI is unplugged (as expected)

[kate-goldenring]

/version patch

[agracey]

I think something like unwrap_or_default("") might be good for both of these in case udev doesn't have a devpath or devnode

[agracey]

nvm... looking at your example, I think rust likely does the same thing under the covers

[kate-goldenring]

I agree that these are pretty, but they are transforming from OsStr to String. All devices have a devpath (get_devpath returns OsStr) while not all have devnodes as you pointed out (get_devnode returns Option<OsStr>)

[agracey]

Yay!!! This looks like it'll fix my issue nicely

[adithyaj]

I see K3s is failing here again and likely related to #531, I'll restart it and it should go away

[kate-goldenring]

@adithyaj @bfjelds @romoh I wonder if this should be a minor (rather than patch) version change since it changes the way that we create Instance ids for udev devices (a hash of devpath rather than devnode). If someone has a pre-existing deployment based on assumed instance ids, this would break it.

[adithyaj]

@adithyaj @bfjelds @romoh I wonder if this should be a minor (rather than patch) version change since it changes the way that we create Instance ids for udev devices (a hash of devpath rather than devnode). If someone has a pre-existing deployment based on assumed instance ids, this would break it.

I agree, since it would be a breaking change it may be better to do a minor bump to have some way to distinguish it. Also, last time there was a breaking change there was a minor version bump too (#437). Let's give this a minor version bump.

[kate-goldenring]

/version minor

[kate-goldenring]

@adithyaj @bfjelds @romoh I wonder if this should be a minor (rather than patch) version change since it changes the way that we create Instance ids for udev devices (a hash of devpath rather than devnode). If someone has a pre-existing deployment based on assumed instance ids, this would break it.

I agree, since it would be a breaking change it may be better to do a minor bump to have some way to distinguish it. Also, last time there was a breaking change there was a minor version bump too (#437). Let's give this a minor version bump.

I think this does point out the fact that we may need to decide scope of "Akri" do breaking changes in DHs mean breaking change to Akri? I'm imagining if the DH's lived in separate repos for example. For now, we can assume yes, but i expect this will evolve

[kate-goldenring]

accidentally bumped version twice: do not merge good to merge