add device containment support to IMP exec helper#7568
Conversation
|
Just curious if you considered rewriting the helper in Python? it would be much easier to maintain and wouldn't require extending sdbus to get new properties if needed in the future (it can just grab the output of |
|
One additional thought is if we're already translating device path to major,minor pairs in C anyway, could we do this in the unprivileged IMP instead, allowing it to support raw |
|
My original thought was to do it in python, but I figured it would add unnecessary latency to job start. I also thought about just having the IMP talk directly to systemd but before when we wanted to call I guess I was thinking helper already has a broker connection, broker already has a dbus connection, this is a reasonable path? Edit: hmm, having the IMP use libsystemd to access these properties does have some appeal. It's a well known API so it might make the IMP design more understandable in isolation. And the hard coded |
|
That seems reasonable to me! Since flux-core controls the helper it would be easy to change to some other language if it comes up in the future. My worry is about other "resource owner options" to the IMP that may need to be added to the
It would be interesting to measure this, I don't think there'd be enough latency to outweigh the benefits, and I thought we weren't that worried about latency for system jobs. (That's why we chose the sdexec-mapper approach instead of doing the resource mapping in sdexec) That being said, we probably should just move forward with a working solution and not debate the details. This is all contained in flux-core and easy to change if necessary in the future! |
I wasn't thinking of the IMP talking to systemd, but more about the fact that the helper has to transform devices into the major,minor,etc form expected by the IMP - it might be easier to just move the C code that does that to IMP and just send the devices over? Then the IMP really is just being used to workaround lack of delgation for DeviceAllow, not some new thing that takes Devices in a different format. In the end it isn't all that important though, and I'd trust your judgement here more than mine since you've dug into it. |
|
Good point about system job startup latency not being a big deal. Well, I'll think about what might be reasonably moved into the imp. Now that the C code is written that would be easy. I wonder if I should go ahead and plumb through X-IMP-* string properties in any case? |
I tried using |
|
Ok, on reflection, your instincts are I think correct. I'll move the "normalization", the standard device list, etc (basically what systemd does) over to flux-security and leave the helper with a simpler job: fetch properties, set options. |
garlick
force-pushed
the
imp_helper
branch
2 times, most recently
from
aa5dd09 to
e97ff61
Compare
|
FYI: I'm running with this branch + #7566 and flux-framework/flux-security#228 on my test system and everything appears to be working end-to-end. As a test for device containment I created a $ ls -l /dev/flux-test
crw-rw-rw- 1 root root 1, 200 Apr 26 19:29 /dev/flux-test
$ cat /dev/flux-test
cat: /dev/flux-test: No such device or address
$ flux alloc -n1
$ cat /dev/flux-test
cat: /dev/flux-test: Operation not permitted
$ lstopo
Machine (3790MB total)
Package L#0
NUMANode L#0 (P#0 3790MB)
L2 L#0 (1024KB) + L1d L#0 (32KB) + L1i L#0 (48KB) + Core L#0 + PU L#0 (P#3) |
|
Very cool! The nvidia device containment will be interesting to test on IPA and that DGX system. Maybe we can make a plan for that this week, before our release deadline. |
Problem: the IMP helper needs to support device containment options, which will be difficult to implement in a shell script. Convert to a built-in C command with no functional changes. The hyphens in the name won't work in a built-in, so use underscores. Update tests and job-exec to use the new name.
Problem: the DeviceAllow systemd unit property has D-Bus type a(ss) (array of string-pair structs), which sdbus cannot currently decode from a variant wrapper. Add sdmsg_get_struct_ss to decode a single (ss) struct into a two-element JSON array, extend sdmsg_get to dispatch on "(ss)", and handle the a(ss) variant case explicitly in sdmsg_get_variant and sdmsg_read. Add a unit test and update the capability table in the sdbus guide. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Problem: fetching device containment properties for a running systemd unit requires resolving the unit object path from its invocation ID first, but GetUnitByInvocationID is not yet registered in the sdbus interface table. Add the entry using the generic callbacks; the method takes a byte array (ay) and returns an object path (o), both of which are already handled generically. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Problem: the sdbus guide describes type translation and the RPC protocol but does not enumerate which (interface, member) pairs are actually registered, leaving users to read interface.c to find out what can be called. Add a Supported Methods table listing all registered interfaces and members with their input/output type signatures, and add the new method names to the spellcheck wordlist. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Problem: INVOCATION_ID is wrong in the systemd unit when job-exec starts it. job-exec sets up the IMP's environment starting with the leader broker environment. When the broker is started by systemd, INVOCATION_ID refers to the leader flux system unit. Clear INVOCATION_ID so that the value set by systemd for the transient unit is not clobbered, similar to how NOTIFY_SOCKET is handled.
Problem: the IMP exec helper doesn't pass DevicePolicy and DeviceAllow from systemd to the IMP, so the IMP doesn't know how to restrict device access. If INVOCATION_ID is set in the environment, assume the IMP is being launched in a flux-owned systemd unit and the DevicePolicy and DeviceAllow properties should be relayed to the IMP. Look up the unit name using INVOCATION_ID, then fetch the two properties and add them to the options key (see RFC 15) in the IMP input object. If INVOCATION_ID is not set, skip setting these options, which the IMP will interpret as "allow all". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Problem: the IMP exec helper has no test coverage. Add a sharness test that covers usage when not run under systemd, and some error paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Problem: there is no test coverage for the IMP exec helper's interaction with systemd. Add t2415-sdexec-device.t, which uses the helper's --test-nojob option under flux exec --service=sdexec to check its handling of DeviceAllow and DevicePolicy property settings. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Problem: the FLUX_IMP_EXEC_HELPER environment variable is undocumented. Add it to the environment man page.
Problem: the IMP may inherit a bogus INVOCATION_ID when not launched from systemd, if it is set in the leader broker environment. Clear INVOCATION_ID in the rexec server, similar to how NOTIFY_SOCKET is handled.
|
This branch is now simplified so that the imp helper just forwards the I did not convert the helper to python as this is working but we can trivially do that later if it is needed. Ready for a review. |
garlick
changed the title
grondo
left a comment
grondo
left a comment
There was a problem hiding this comment.
This LGTM! Been using it at home 10/10
|
OK! Thanks, I'll set MWP. |
Merge Queue Status
This pull request spent 15 seconds in the queue, including 2 seconds running CI. Required conditions to merge
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7568 +/- ##
========================================
Coverage 83.92% 83.92%
========================================
Files 569 570 +1
Lines 95505 95624 +119
========================================
+ Hits 80157 80257 +100
- Misses 15348 15367 +19
🚀 New features to boost your workflow:
|
2 participants
This replaces the shell script IMP helper with a C version and modifies it to translate
DevicePolicyandDeviceAllowsystemd properties set by the job exec system (#7566) into a simpler form that is provided as input to the IMP.I've done some testing with flux-framework/flux-security#228 and it seems to work now, after a BPF problem that I'll push a fix for shortly. Then we can add #7566 and we might have the whole containment picture, or at least the first functional iteration.
Marking WIP for now but wanted to get this piece posted.