many: add cuda-driver-libs interface #15063

alfonsosanchezbeato · 2025-02-11T21:32:36Z

Add cuda-driver-libs interface. This is the first interface using the ldconfig backend. Additionally, it is the first example of an implicit plug, so some changes in ifacestate are also part of this change.

github-actions · 2025-02-11T23:24:36Z

Tue Feb 25 15:14:27 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/13502715560

Failures:

Preparing:

openstack:opensuse-tumbleweed-64
openstack:opensuse-tumbleweed-64
openstack:opensuse-tumbleweed-64
openstack:opensuse-15.6-64
openstack:opensuse-15.6-64
openstack:opensuse-tumbleweed-64
openstack:opensuse-tumbleweed-64
openstack:opensuse-tumbleweed-64

Executing:

google-distro-1:amazon-linux-2-64:tests/main/progress
google-distro-2:arch-linux-64:tests/main/interfaces-network-status-classic
openstack:debian-sid-64:tests/main/auto-refresh-gating
google-arm:ubuntu-20.04-arm-64:tests/main/progress
google-arm:ubuntu-core-22-arm-64:tests/core/services
google:ubuntu-25.04-64:tests/main/snap-user-service-socket-activation
google:ubuntu-25.04-64:tests/main/security-device-cgroups:uinput
google:ubuntu-25.04-64:tests/main/progress
google:ubuntu-25.04-64:tests/main/snap-ns-forward-compat
google:ubuntu-25.04-64:tests/main/security-device-cgroups-serial-port
google:ubuntu-25.04-64:tests/main/security-device-cgroups:kmsg
google:ubuntu-25.04-64:tests/main/security-device-cgroups-helper
google:ubuntu-25.04-64:tests/main/security-device-cgroups-required-or-optional
google:ubuntu-25.04-64:tests/main/security-device-cgroups-self-manage
google:ubuntu-25.04-64:tests/main/cgroup-devices-v2
google:ubuntu-25.04-64:tests/main/security-device-cgroups-strict-enforced
google:ubuntu-16.04-64:tests/main/microk8s-smoke:edge

Restoring:

openstack:opensuse-tumbleweed-64
openstack:opensuse-tumbleweed-64
openstack:opensuse-tumbleweed-64
openstack:opensuse-15.6-64
openstack:opensuse-15.6-64
openstack:opensuse-tumbleweed-64
openstack:opensuse-tumbleweed-64
openstack:opensuse-tumbleweed-64
google:ubuntu-25.04-64:tests/main/security-device-cgroups-strict-enforced

codecov · 2025-02-12T15:15:43Z

Codecov Report

Attention: Patch coverage is 83.68794% with 23 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@c4ff9e7). Learn more about missing BASE report.
Report is 267 commits behind head on master.

Files with missing lines	Patch %	Lines
interfaces/builtin/cuda_driver_libs.go	86.25%	8 Missing and 3 partials ⚠️
overlord/ifacestate/ifacestate.go	0.00%	2 Missing and 3 partials ⚠️
overlord/ifacestate/handlers.go	0.00%	0 Missing and 4 partials ⚠️
overlord/ifacestate/helpers.go	0.00%	0 Missing and 2 partials ⚠️
interfaces/ldconfig/spec.go	95.83%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #15063   +/-   ##
=========================================
  Coverage          ?   78.08%           
=========================================
  Files             ?     1184           
  Lines             ?   158105           
  Branches          ?        0           
=========================================
  Hits              ?   123457           
  Misses            ?    26976           
  Partials          ?     7672

Flag	Coverage Δ
unittests	`78.08% <83.68%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

andrewphelpsj

Thank you!

andrewphelpsj · 2025-02-14T15:42:08Z

interfaces/builtin/cuda_driver_libs.go

+	if len(fields) != 1 && len(fields) != 3 {
+		return fmt.Errorf("wrong format for api-version: %q", versRange)
+	}
+	switch len(fields) {


A comment here with examples of the expected input might be good. I think something like this?

// valid input examples: // 1.2.3 // 1.2.3 .. 2.3.4

Good point, I've added a comment

pedronis

did a first pass, don't we need a configfile part to this?

pedronis · 2025-02-17T10:30:53Z

interfaces/ldconfig/backend.go

+	// For the moment only the system snap is supported - the
+	// snap.system.conf file is owned by it and the set-up of other snaps
+	// must not affect it.
+	if !interfaces.IsTheSystemSnap(appSet.InstanceName()) {


we discussed before that this doesn't make a lot of sense because of the two of the interface, why is it needed?

It is actually needed, otherwise when the slot side builds its profiles after, say, a disconnection, it overrides the content of snap.system.conf. That file is owned by the system plug and no other plug/slot should touch it.

For instance, this is seen when there are 2 snaps providing slots, if one is removed, when reconstructing the slot profiles it sees no spec.libDirs and it removes the file, overriding what the plug profile wrote just before.

let's talk about this in our 1-1, I'm really worried that these are the only interfaces that need this kind of behavior and they assume things about two levels up in the code from them

71aca8f adds behavior so instead of checking the snap name we check if the snap owns plugs that need configuration. We cannot rely only in the connected plugs unfortunately.

pedronis · 2025-02-17T10:33:15Z

interfaces/builtin/cuda_driver_libs.go

+	}
+	// Validate directories
+	for _, dir := range libDirs {
+		if !strings.HasPrefix(dir, "$SNAP") && !strings.HasPrefix(dir, "${SNAP}") {


shouldn't this also include the / ?

Good point, changed

pedronis · 2025-02-17T10:34:43Z

@alfonsosanchezbeato maybe it would make sense to make the changes to support implicit plugs their own PR?

alfonsosanchezbeato

@pedronis thanks for the review, comments addressed. Regarding configfile, no, for th cuda interface we do not have configuration files that we need to take care of.

alfonsosanchezbeato · 2025-02-19T13:13:08Z

interfaces/ldconfig/backend.go

+	// For the moment only the system snap is supported - the
+	// snap.system.conf file is owned by it and the set-up of other snaps
+	// must not affect it.
+	if !interfaces.IsTheSystemSnap(appSet.InstanceName()) {


It is actually needed, otherwise when the slot side builds its profiles after, say, a disconnection, it overrides the content of snap.system.conf. That file is owned by the system plug and no other plug/slot should touch it.

For instance, this is seen when there are 2 snaps providing slots, if one is removed, when reconstructing the slot profiles it sees no spec.libDirs and it removes the file, overriding what the plug profile wrote just before.

alfonsosanchezbeato · 2025-02-19T13:16:27Z

interfaces/builtin/cuda_driver_libs.go

+	}
+	// Validate directories
+	for _, dir := range libDirs {
+		if !strings.HasPrefix(dir, "$SNAP") && !strings.HasPrefix(dir, "${SNAP}") {


Good point, changed

pedronis

thanks for the changes, one comment about a detail

pedronis · 2025-02-20T10:35:58Z

interfaces/ldconfig/spec.go

@@ -61,10 +63,18 @@ func (spec *Specification) AddLibDirs(dirs []string) error {
 	return nil
 }

+func (spec *Specification) AddPlug(plug string) {


this doesn't need to be public and done by the interfaces, we can do it for them in:

snapd/interfaces/ldconfig/spec.go

Line 114 in 71aca8f

func (spec *Specification) AddPermanentPlug(iface interfaces.Interface, plug *snap.PlugInfo) error {

no?

Right, I've changed this now: a5b040c

pedronis · 2025-02-20T10:37:06Z

interfaces/ldconfig/backend.go

@@ -59,6 +59,12 @@ func (b *Backend) Setup(appSet *interfaces.SnapAppSet, opts interfaces.Confineme
 			appSet.InstanceName(), err)
 	}

+	// Setup ldconfig only of the snap has plugs that require it. For the
+	// moment this is only the system snap.
+	if len(spec.(*Specification).plugs) == 0 {


nitpick: I would do this check in the helper, also because it gets the spec casted already

pedronis

question about the last change

pedronis · 2025-02-21T08:36:52Z

interfaces/ldconfig/spec.go

@@ -102,6 +108,9 @@ func (spec *Specification) AddConnectedSlot(iface interfaces.Interface, plug *in

 // AddPermanentPlug records ldconfig-specific side-effects of having a plug.
 func (spec *Specification) AddPermanentPlug(iface interfaces.Interface, plug *snap.PlugInfo) error {
+	// Keep track of interfaces using this backend on the consumer side


shouldn't we do this only if the interface has actually LdconfigPermanentPlug or LdconfigConnectedPlug implemented? otherwise I think this method is called for all plugs

I check now for LdconfigConnectedPlug, I want to avoid the interface to implement an empty LdconfigPermanentPlug.

I thought you mentioned you need to run things even if the plug is not connected? as I said you can check for either definer in AddPermantPlug

Yes, otherwise we do not know if we need to remove configuration if all connections are gone.

Latest commit checks the definer in AddPermantPlug

pedronis

thanks

pedronis · 2025-02-21T15:34:54Z

interfaces/ldconfig/spec.go

 		if !interfaces.IsTheSystemSnap(plug.Snap().InstanceName()) {
 			return errors.New("internal error: ldconfig plugs can be defined only by the system snap")
 		}


would it make sense to move the check into the getConnectedPlugCallback ?

this has changed quite a bit since the initial review

Meulengracht

LGTM

Grow test for interfaces that export libraries to the rootfs, like cuda-driver-libs.

Make sure that Setup touches the ldconfig configuration only for the system snap, as it is the owner of it. Otherwise the configuration would be destroyed when the other snaps are configured.

Register snap plugs so we know when we need to generate an ldconfig file.

interface wants to use the ldconfig backend.

andrewphelpsj · 2025-02-25T12:52:43Z

interfaces/ldconfig/spec.go

-			slot *interfaces.ConnectedSlot) error
+	connectedPlugCallback, err := getConnectedPlugCallback(iface, plug.Snap().InstanceName())
+	if err != nil {
+		return err
 	}


Thanks for this, it'd be nice if we could do this change for other things. It makes reading the code in this package harder when the interfaces are defined locally like this.

andrewphelpsj

Thanks!

alfonsosanchezbeato requested review from pedronis and andrewphelpsj February 11, 2025 21:32

alfonsosanchezbeato force-pushed the add-cuda-driver-libs branch from d09bff4 to fd9d5c0 Compare February 12, 2025 14:57

alfonsosanchezbeato force-pushed the add-cuda-driver-libs branch from fd9d5c0 to 540bc1d Compare February 12, 2025 17:32

andrewphelpsj previously approved these changes Feb 14, 2025

View reviewed changes

pedronis requested changes Feb 17, 2025

View reviewed changes

alfonsosanchezbeato commented Feb 19, 2025

View reviewed changes

alfonsosanchezbeato requested a review from pedronis February 19, 2025 13:29

pedronis reviewed Feb 20, 2025

View reviewed changes

alfonsosanchezbeato added 3 commits February 20, 2025 13:27

interfaces/builtin: define cuda-driver-libs

7c7d885

o/ifacestate: support implicit plugs, starting with cuda-driver-libs

5a4ee0e

interfaces: add support for implicit plugs

e639441

alfonsosanchezbeato force-pushed the add-cuda-driver-libs branch from 71aca8f to a5b040c Compare February 20, 2025 18:27

alfonsosanchezbeato requested a review from pedronis February 20, 2025 18:29

pedronis requested changes Feb 21, 2025

View reviewed changes

alfonsosanchezbeato requested a review from pedronis February 21, 2025 13:29

pedronis approved these changes Feb 21, 2025

View reviewed changes

alfonsosanchezbeato requested a review from andrewphelpsj February 21, 2025 15:41

alfonsosanchezbeato force-pushed the add-cuda-driver-libs branch from a1cc010 to 757157f Compare February 21, 2025 15:54

alfonsosanchezbeato requested a review from Meulengracht February 24, 2025 13:56

Meulengracht approved these changes Feb 24, 2025

View reviewed changes

alfonsosanchezbeato added 4 commits February 24, 2025 11:18

run-checks: skip cuda-driver-libs iface as snaps cannot have it as plug

4258c5d

tests/interfaces-userspace-libs: test for interfaces exposing libs

fb2f6b1

Grow test for interfaces that export libraries to the rootfs, like cuda-driver-libs.

interfaces/ldconfig: only the system snap can touch the configuration

a73e2ab

Make sure that Setup touches the ldconfig configuration only for the system snap, as it is the owner of it. Otherwise the configuration would be destroyed when the other snaps are configured.

interfaces/ldconfig: register snap plugs

719f8e0

Register snap plugs so we know when we need to generate an ldconfig file.

interfaces/ldconfig: check for connected plug to know if the

ad8f627

interface wants to use the ldconfig backend.

alfonsosanchezbeato force-pushed the add-cuda-driver-libs branch from 757157f to ad8f627 Compare February 24, 2025 16:18

andrewphelpsj reviewed Feb 25, 2025

View reviewed changes

andrewphelpsj approved these changes Feb 25, 2025

View reviewed changes

alfonsosanchezbeato merged commit 2e0be42 into canonical:master Feb 25, 2025
71 of 78 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

many: add cuda-driver-libs interface #15063

many: add cuda-driver-libs interface #15063

alfonsosanchezbeato commented Feb 11, 2025

github-actions bot commented Feb 11, 2025 •

edited

Loading

codecov bot commented Feb 12, 2025 •

edited

Loading

andrewphelpsj left a comment

andrewphelpsj Feb 14, 2025

alfonsosanchezbeato Feb 19, 2025

pedronis left a comment

pedronis Feb 17, 2025

alfonsosanchezbeato Feb 19, 2025 •

edited

Loading

pedronis Feb 19, 2025 •

edited

Loading

alfonsosanchezbeato Feb 20, 2025

pedronis Feb 17, 2025

alfonsosanchezbeato Feb 19, 2025

pedronis commented Feb 17, 2025

alfonsosanchezbeato left a comment

alfonsosanchezbeato Feb 19, 2025 •

edited

Loading

alfonsosanchezbeato Feb 19, 2025

pedronis left a comment

pedronis Feb 20, 2025

alfonsosanchezbeato Feb 20, 2025

pedronis Feb 20, 2025 •

edited

Loading

pedronis left a comment

pedronis Feb 21, 2025

alfonsosanchezbeato Feb 21, 2025

pedronis Feb 21, 2025

alfonsosanchezbeato Feb 21, 2025

alfonsosanchezbeato Feb 21, 2025

pedronis left a comment

pedronis Feb 21, 2025

Meulengracht left a comment

andrewphelpsj Feb 25, 2025

andrewphelpsj left a comment

many: add cuda-driver-libs interface #15063

many: add cuda-driver-libs interface #15063

Conversation

alfonsosanchezbeato commented Feb 11, 2025

github-actions bot commented Feb 11, 2025 • edited Loading

Failures:

Preparing:

Executing:

Restoring:

codecov bot commented Feb 12, 2025 • edited Loading

Codecov Report

andrewphelpsj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfonsosanchezbeato Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

pedronis Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis commented Feb 17, 2025

alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

alfonsosanchezbeato Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Meulengracht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewphelpsj left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 11, 2025 •

edited

Loading

codecov bot commented Feb 12, 2025 •

edited

Loading

alfonsosanchezbeato Feb 19, 2025 •

edited

Loading

pedronis Feb 19, 2025 •

edited

Loading

alfonsosanchezbeato Feb 19, 2025 •

edited

Loading

pedronis Feb 20, 2025 •

edited

Loading