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

Append hash of device usage id to device property key name #561

Merged

Conversation

johnsonshih
Copy link
Contributor

@johnsonshih johnsonshih commented Feb 28, 2023

What this PR does / why we need it:
When allocate a device plugin, Agent adds the device properties as the container's environment variables. Currently, when a container uses multiple device plugins, all device plugins will set device properties to the same set of environment variables and only the last one write to the environment variable is recorded. This PR suffix device usage id to the device property key name so the environment variables won't be overwritten.

This PR is part of the fix for issue #492

Special notes for your reviewer:

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

@johnsonshih
Copy link
Contributor Author

/version patch

Signed-off-by: Johnson Shih <jshih@microsoft.com>
Signed-off-by: Johnson Shih <jshih@microsoft.com>
Signed-off-by: Johnson Shih <jshih@microsoft.com>
Signed-off-by: Johnson Shih <jshih@microsoft.com>
Signed-off-by: Johnson Shih <jshih@microsoft.com>
@johnsonshih johnsonshih force-pushed the user/jshih/device-plugin-env-var branch from 1b778a6 to b4f2c91 Compare April 5, 2023 22:36
Signed-off-by: Johnson Shih <jshih@microsoft.com>
@kate-goldenring
Copy link
Contributor

I believe this is a breaking change since it will break all existing brokers when they look up properties. We should make this a minor version change and we will need update docs.

@kate-goldenring
Copy link
Contributor

/version minor

@github-actions github-actions bot added the version/minor Minor version change is needed label Apr 6, 2023
github-actions bot and others added 2 commits April 6, 2023 01:40
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Signed-off-by: Johnson Shih <jshih@microsoft.com>
Copy link
Contributor

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

I am concerned about my comment around regenerating random hashes rather than associating properties via an instance's hash

agent/src/util/discovery_operator.rs Outdated Show resolved Hide resolved
samples/brokers/onvif-video-broker/Akri.cs Outdated Show resolved Hide resolved
agent/src/util/device_plugin_service.rs Outdated Show resolved Hide resolved
Signed-off-by: Johnson Shih <jshih@microsoft.com>
Signed-off-by: Johnson Shih <jshih@microsoft.com>
Signed-off-by: Johnson Shih <jshih@microsoft.com>
Copy link
Contributor

@kate-goldenring kate-goldenring left a 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 addition @johnsonshih!

@yujinkim-msft yujinkim-msft merged commit 8125977 into project-akri:main Apr 7, 2023
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
version/minor Minor version change is needed version/patch Patch version change is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable mounting connectivity information for multiple devices/instances in a Pod
3 participants