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.

Sign up for GitHub

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

Jump to bottom

udev: Implement a way to group devices of a same tree #564

Merged
merged 3 commits into from
Apr 7, 2023
Merged

udev: Implement a way to group devices of a same tree #564

merged 3 commits into from
Apr 7, 2023

Conversation

diconico07
Copy link
Contributor

@diconico07 diconico07 commented Mar 2, 2023 •
edited
Loading

What this PR does / why we need it:
This PR adds the groupRecursive option to the udev device discovery handler, this configuration switch allows to group devices that share a parent/child relation.

This can be used to group into one akri instance a complex device that is split into multiple nodes. It is up to the user to define the boundaries of the device as there are no standard way to do so.

It works by comparing sysfs path for a device prefixing another one (i.e /devices/pci0 is a parent of /devices/pci0/0.1/0.1.1/0.1.1:1, this example is obviously fictive)

It adds all devnodes to the mounts and add a UDEV_DEVNODE_n property for each one of them.

The UDEV_DEVPATH just need the topmost one as all other are subdirectories of it.

This closes #490

Special notes for your reviewer:
I will cross-link with a documentation PR as soon as I finish writing it.

If applicable:

  • this PR has an associated PR with documentation in akri-docs udev: Add documentation of groupRecursive option akri-docs#64
  • 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

@diconico07 diconico07 mentioned this pull request Mar 2, 2023
Merged
@diconico07 diconico07 marked this pull request as ready for review March 2, 2023 13:18
@harrison-tin harrison-tin linked an issue Mar 7, 2023 that may be closed by this pull request
udev Discovery Handler discovers parent device without the filtered property #550
Closed
@diconico07
Copy link
Contributor Author

/version minor

@github-actions github-actions bot added the version/minor Minor version change is needed label Mar 10, 2023
@diconico07 diconico07 force-pushed the tree-grouping branch 2 times, most recently from 087bdf9 to 009c01d Compare April 6, 2023 11:36
kate-goldenring
kate-goldenring reviewed Apr 6, 2023
View reviewed changes
discovery-handlers/udev/src/discovery_impl.rs
@@ -543,6 +543,24 @@ fn device_or_parents_have_tag(device: &impl DeviceExt, value_regex: &Regex) -> b
}
}

/// Retrieve Parent or Children of a device using their sysfs path.
pub fn get_device_relatives<'a>(
Copy link
Contributor

Choose a reason for hiding this comment

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

@diconico07 can you write a few unit tests for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote a test case, tell me if you feel I missed something 😄

kate-goldenring
kate-goldenring reviewed Apr 6, 2023
View reviewed changes
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.

Just a few nits but overall this looks great. Thanks! A couple unit tests here would be helpful. We also want to be mindful of version here. I believe we have another pr in queue that bumps to 0.10.0.

discovery-handlers/udev/src/discovery_handler.rs Outdated Show resolved Hide resolved
discovery-handlers/udev/src/discovery_handler.rs
if !discovery_handler_config.group_recursive {
devpaths.insert(path.0.clone(), vec![path]);
} else {
match get_device_relatives(&path.0, devpaths.keys()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: i would put this else block in a separate function so we can unit test it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do this next week, as I'll do the unit tests as well (won't have enough time before that), if you want to merge before that, I'll do it in a separate PR.

deployment/helm/values.yaml Show resolved Hide resolved
@diconico07
udev: Implement a way to group devices of a same tree
34d006f 
This commit adds the `groupRecursive` option to the udev device
discovery handler, this configuration switch allows to group devices
that share a parent/child relation.

This can be used to group into one akri instance a complex device that
is split into multiple nodes. It is up to the user to define the
boundaries of the device as there are no standard way to do so.

It works by comparing sysfs path for a device prefixing another one
(i.e `/devices/pci0` is a parent of `/devices/pci0/0.1/0.1.1/0.1.1:1`,
this example is obviously fictive)

It adds all devnodes to the mounts and add a UDEV_DEVNODE_n property for
each one of them.

The UDEV_DEVPATH just need the topmost one as all other are
subdirectories of it.

Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
@diconico07
helm: Add udev groupRecursive switch to Helm chart
341b9da 
Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
@diconico07
Update version
24de50a 
Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
@yujinkim-msft yujinkim-msft merged commit 6ca2874 into project-akri:main Apr 7, 2023
diconico07 added a commit to diconico07/akri that referenced this pull request Apr 13, 2023
@diconico07
udev: Improve testability and tests
3718917 
As indicated in project-akri#564 the new grouping behavior could get more test
coverage. This commit moves the grouping logic to a separate function
and adds relevant tests.

Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
@diconico07 diconico07 mentioned this pull request Apr 14, 2023
Merged
8 tasks
diconico07 added a commit to diconico07/akri that referenced this pull request Apr 14, 2023
@diconico07
udev: Improve testability and tests
6eeb8ee 
As indicated in project-akri#564 the new grouping behavior could get more test
coverage. This commit moves the grouping logic to a separate function
and adds relevant tests.

Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
@diconico07 diconico07 deleted the tree-grouping branch April 17, 2023 07:34
diconico07 added a commit to diconico07/akri that referenced this pull request Apr 19, 2023
@diconico07
udev: Improve testability and tests
97f722e 
As indicated in project-akri#564 the new grouping behavior could get more test
coverage. This commit moves the grouping logic to a separate function
and adds relevant tests.

Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
diconico07 added a commit to diconico07/akri that referenced this pull request Apr 25, 2023
@diconico07
udev: Prevent duplicate nodes from showing
6a6aad4 
This commit prevents duplicate device node from being listed when
multiple rules match the same devices.

This fixes a regression introduced by project-akri#564.

Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
diconico07 added a commit to diconico07/akri that referenced this pull request May 22, 2023
@diconico07
udev: Improve testability and tests
eb907ad 
As indicated in project-akri#564 the new grouping behavior could get more test
coverage. This commit moves the grouping logic to a separate function
and adds relevant tests.

Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
diconico07 added a commit to diconico07/akri that referenced this pull request May 22, 2023
@diconico07
udev: Prevent duplicate nodes from showing
3aa5aee 
This commit prevents duplicate device node from being listed when
multiple rules match the same devices.

This fixes a regression introduced by project-akri#564.

Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kate-goldenring kate-goldenring kate-goldenring approved these changes

@bfjelds bfjelds Awaiting requested review from bfjelds bfjelds is a code owner

@jiria jiria Awaiting requested review from jiria jiria is a code owner

@Britel Britel Awaiting requested review from Britel Britel is a code owner

@romoh romoh Awaiting requested review from romoh romoh is a code owner

@adithyaj adithyaj Awaiting requested review from adithyaj adithyaj is a code owner

Assignees
No one assigned
Labels
version/minor Minor version change is needed
Projects
None yet
Milestone
No milestone
3 participants
@diconico07 @kate-goldenring @yujinkim-msft