pkg/cdi: add functions for generating Spec names and removing Spec files. #77

klihub · 2022-08-09T19:34:33Z

Add convenience functions for generating Spec names for ordinary and transient Spec files. The generated names can be passed to WriteSpec() of Cache or Registry to atomically write a Spec file. Also add a function for removing Spec files created with WriteSpec().

klihub · 2022-08-09T19:43:44Z

@bart0sh @klueska I cooked up this patch set for dealing with consistently named vendor-specific dynamically generated Spec files. Using the added two functions should guarantee global uniqueness of the generated Spec files with atomic updates to the cache/registry from the Refresh() point of view.

If this is found to be a usable abstraction then I'll spell out the usage of these functions in doc.go... And I'd also like to replace the current WriteSpec() with a corresponding pair of CreateSpec()/RemoveSpec(). These would provide atomic updates and use a similar vendor+device-namespaced naming convention but with reproducible file names (and take an overwrite flag like WriteSpec()).

pkg/cdi/cache.go

klueska · 2022-08-10T12:08:54Z

I only took a high-level look at the code, but in general, I agree with the other sentiments given.

Rather than create / delete files through the CDI library, we should add functions to:

Generate a transient / unique file name following a specific convention
Generate a transient / unique device name following a specific convention

It is then up to the user to call these functions to generate these transient filenames / device names, but other than that, nothing changes about the way the current API is used to create a spec / write a spec file out to disk.

klihub · 2022-08-10T12:21:50Z

So how about something like this. This is the same as the combined single function above, but it does not hardcode JSON as the file format. I tried to formulate the documentation of how to use it, including the driver and transientID .

// GetSpecName generates a vendor+class scoped Spec file name. The name
// can be passed to WriteSpec() to write a Spec file to the file system.
//
// The file name is generated without a ".json" or ".yaml" extension.
// The caller can append the desired extension to choose a particular
// encoding. Otherwise WriteSpec() will use its default encoding.
//
// vendor and class should match the vendor and class of the CDI Spec.
// driver can be left empty if only a single entity is generating Spec
// files for the given vendor/class combination on the host. Otherwise
// driver should uniquely identify the caller among the multiple ones
// that generate Spec files with the same vendor and class.
//
// If the lifecycle of the Spec file is tied to the lifecycle of some
// entity external to CDI then a non-empty transientID should be used.
// This should uniquely identify the external entity. For instance, it
// can be the ID of a container, if the Spec will be generated for use
// by a single container.
func GetSpecName(vendor, class, driver, transientID string) string {
  // ...
}

We could also use the same Spec the client will pass to WriteSpec() to dig out vendor/class ourselves. With that the signature would be something like func GetSpecName(spec *cdi.Spec, driver, transientID string) string.

pkg/cdi/spec.go

pkg/cdi/cache.go

pkg/cdi/spec.go

bart0sh · 2022-08-16T10:23:59Z

@klihub As far as I understood there are 2 ways of writing spec to the file:
spec.Write(overwrite bool)
cache.WriteSpec(cache.raw *cdi.Spec, name string)

Can you describe possible usage scenarios and recommendations for each?

klihub · 2022-08-16T10:32:34Z

@klihub As far as I understood there are 2 ways of writing spec to the file: spec.Write(overwrite bool) cache.WriteSpec(cache.raw *cdi.Spec, name string)

Can you describe possible usage scenarios and recommendations for each?

Unless you are doing something special which requires you to fully control the exact path where the Spec file gets written to, you should always just use WriteSpec(). And as I mentioned already earlier, it might be better to change Spec.Write() into spec.write() (IOW, make it package private function) to eliminate any confusion which one should be used.

pkg/cdi/doc.go

pkg/cdi/spec.go

bart0sh · 2022-08-17T09:49:43Z

@klihub the PR looks good to me. I made some nit-picking comments, but the overall impression is good. Thank you!

klihub · 2022-08-17T09:54:17Z

@klihub the PR looks good to me. I made some nit-picking comments, but the overall impression is good. Thank you!

Thanks ! Those are all valid. I'll address/fix them once I'm back from lunch.

klihub · 2022-08-17T12:28:00Z

@klihub the PR looks good to me. I made some nit-picking comments, but the overall impression is good. Thank you!

Thanks ! Those are all valid. I'll address/fix them once I'm back from lunch.

@bart0sh Done.

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

RemoveSpec() can be used to remove a Spec file written by WriteSpec(). It takes the same name that was passed to WriteSpec() to create the file. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

bart0sh · 2022-09-28T10:12:03Z

/lgtm
@klueska @elezar @pohly can you review again?

pohly

Just one small nit.

However, my earlier comment regarding these new functions still stands: because the library assumes that it has direct access to the directories, it cannot be used for the Kubernetes test/e2e/dra/test-driver where all of the logic runs outside of the cluster and only indirectly manipulates files.

pkg/cdi/doc.go

bart0sh · 2022-10-25T07:07:43Z

@klueska @pohly @elezar all comments have been addressed. Can you review again and merge?

bart0sh · 2022-10-25T07:12:10Z

@elezar would it make sense to make a new release after merging this? I hope I can update CDI in containerd before their release if we release fast enough.

pohly · 2022-10-25T10:48:30Z

I hope I can update CDI in containerd before their release if we release fast enough.

Does this change matter for containerd?

bart0sh · 2022-10-25T13:35:53Z

This change doesn't matter for containerd. However, I'd prefer to update CDI there to the latest possible CDI release.

elezar

@klihub I think this looks good from my side. I'll just give @klueska @pohly and @bart0sh a moment to respond before merging.

pkg/cdi/spec.go

elezar · 2022-10-26T14:39:53Z

@elezar would it make sense to make a new release after merging this? I hope I can update CDI in containerd before their release if we release fast enough.

@bart0sh sure. We can cut a new release as soon as possible after this has landed. The operation is lightweight enough and we don't expect any breaking changes from automatic updates by consumers.

pkg/cdi/doc.go

Add functions for generating ordinary or transient Spec file names. The generated names can be passed to WriteSpec() to create the actual Spec files and to RemoveSpec() to remove them. Also update the package introduction in the golang docs describing how dynamic Spec files should be generated using the API. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

This should reduce confusion about how generates Specs should be written (cache.WriteSpec() vs. spec.Write()) and also reduce the possibility/temptation for users to bypass the registry/cache when interacting with Specs. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

Alias specs.CurrentVersion as pkg/cdi.CurrentVersion. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

elezar

Thanks again for the work here @klihub.

I think we can merge this and then address any minor points in a follow-up.

elezar · 2022-10-28T13:42:01Z

pkg/cdi/cache.go

 		return errors.New("no Spec directories to write to")
 	}

-	prio = len(c.specDirs) - 1
-	specDir = c.specDirs[prio]
 	path = filepath.Join(specDir, name)
 	if ext := filepath.Ext(path); ext != ".json" && ext != ".yaml" {


This is already set in newSpec, correct? So we could drop this entire conditional.

pohly · 2022-11-04T15:52:51Z

I updated to the new CDI release and this PR broke how the DRA test driver generates specs:

spec := &specs.Spec{ ... }
cdiSpec, err := cdiapi.NewSpec(&spec, filePath, 1)

What is the right way of creating a cdiapi.Spec instance now?

I need it further down for:

dev := cdiSpec.GetDevice(deviceName).GetQualifiedName()
resp := &drapbv1.NodePrepareResourceResponse{CdiDevices: []string{dev}}

pohly · 2022-11-04T16:10:43Z

I tried this instead, but tests didn't pass:

diff --git a/test/e2e/dra/test-driver/app/kubeletplugin.go b/test/e2e/dra/test-driver/app/kubeletplugin.go
index 945a5dad8c3..29e8fb174f7 100644
--- a/test/e2e/dra/test-driver/app/kubeletplugin.go
+++ b/test/e2e/dra/test-driver/app/kubeletplugin.go
@@ -145,10 +145,11 @@ func (ex *ExamplePlugin) NodePrepareResource(ctx context.Context, req *drapbv1.N
        }
 
        deviceName := "claim-" + req.ClaimUid
-       spec := specs.Spec{
-               // TODO: clarify what version to put here (https://github.com/container-orchestrated-devices/container-device-interface/issues/60).
-               Version: "0.2.0",
-               Kind:    ex.driverName + "/test",
+       vendor := ex.driverName
+       class := "test"
+       spec := &specs.Spec{
+               Version: cdiapi.CurrentVersion,
+               Kind:    vendor + "/" + class,
                // At least one device is required and its entry must have more
                // than just the name.
                Devices: []specs.Device{
@@ -161,12 +162,7 @@ func (ex *ExamplePlugin) NodePrepareResource(ctx context.Context, req *drapbv1.N
                },
        }
        filePath := ex.getYAMLFilePath(req.ClaimUid)
-       cdiSpec, err := cdiapi.NewSpec(&spec, filePath, 1)
-       if err != nil {
-               return nil, fmt.Errorf("create spec: %v", err)
-       }
-
-       buffer, err := yaml.Marshal(*cdiSpec)
+       buffer, err := yaml.Marshal(spec)
        if err != nil {
                return nil, fmt.Errorf("marshal spec: %v", err)
        }
@@ -178,7 +174,7 @@ func (ex *ExamplePlugin) NodePrepareResource(ctx context.Context, req *drapbv1.N
                return nil, fmt.Errorf("failed to write CDI file %v", err)
        }
 
-       dev := cdiSpec.GetDevice(deviceName).GetQualifiedName()
+       dev := cdiapi.QualifiedName(vendor, class, deviceName)
        resp := &drapbv1.NodePrepareResourceResponse{CdiDevices: []string{dev}}
 
        ex.mutex.Lock()

pohly · 2022-11-04T17:11:17Z

Something is odd here. yaml.Marshal doesn't encode the content of the variable:

(dlv) p spec
*github.com/container-orchestrated-devices/container-device-interface/specs-go.Spec {
	Version: "0.5.0",
	Kind: "dra-1856.k8s.io/test",
	Devices: []github.com/container-orchestrated-devices/container-device-interface/specs-go.Device len: 1, cap: 1, [
		(*"github.com/container-orchestrated-devices/container-device-interface/specs-go.Device")(0xc000548700),
	],
	ContainerEdits: github.com/container-orchestrated-devices/container-device-interface/specs-go.ContainerEdits {
		Env: []string len: 0, cap: 0, nil,
		DeviceNodes: []*github.com/container-orchestrated-devices/container-device-interface/specs-go.DeviceNode len: 0, cap: 0, nil,
		Hooks: []*github.com/container-orchestrated-devices/container-device-interface/specs-go.Hook len: 0, cap: 0, nil,
		Mounts: []*github.com/container-orchestrated-devices/container-device-interface/specs-go.Mount len: 0, cap: 0, nil,},}
(dlv) p spec.Devices[0]
github.com/container-orchestrated-devices/container-device-interface/specs-go.Device {
	Name: "claim-b558fe5f-16db-4d95-b17f-c128873a9208",
	ContainerEdits: github.com/container-orchestrated-devices/container-device-interface/specs-go.ContainerEdits {
		Env: []string len: 1, cap: 1, [
			"user_a=b",
		],
		DeviceNodes: []*github.com/container-orchestrated-devices/container-device-interface/specs-go.DeviceNode len: 0, cap: 0, nil,
		Hooks: []*github.com/container-orchestrated-devices/container-device-interface/specs-go.Hook len: 0, cap: 0, nil,
		Mounts: []*github.com/container-orchestrated-devices/container-device-interface/specs-go.Mount len: 0, cap: 0, nil,},}

(dlv) p string(buffer)
"cdiVersion: 0.5.0\ncontainerEdits: {}\ndevices:\n- containerEdits:\n"

elezar · 2022-11-04T17:45:57Z

Could you check whether re-exporting the NewSpec method does what you want? I could review it a bit later tonight and cut a new release with the fix?

…

On Fri, Nov 4, 2022, 18:11 Patrick Ohly ***@***.***> wrote: Something is odd here. yaml.Marshal doesn't encode the content of the variable: (dlv) p spec *github.com/container-orchestrated-devices/container-device-interface/specs-go.Spec { Version: "0.5.0", Kind: "dra-1856.k8s.io/test", Devices: []github.com/container-orchestrated-devices/container-device-interface/specs-go.Device len: 1, cap: 1, [ (*"github.com/container-orchestrated-devices/container-device-interface/specs-go.Device")(0xc000548700), ], ContainerEdits: github.com/container-orchestrated-devices/container-device-interface/specs-go.ContainerEdits { Env: []string len: 0, cap: 0, nil, DeviceNodes: []*github.com/container-orchestrated-devices/container-device-interface/specs-go.DeviceNode len: 0, cap: 0, nil, Hooks: []*github.com/container-orchestrated-devices/container-device-interface/specs-go.Hook len: 0, cap: 0, nil, Mounts: []*github.com/container-orchestrated-devices/container-device-interface/specs-go.Mount len: 0, cap: 0, nil,},} (dlv) p spec.Devices[0]github.com/container-orchestrated-devices/container-device-interface/specs-go.Device { Name: "claim-b558fe5f-16db-4d95-b17f-c128873a9208", ContainerEdits: github.com/container-orchestrated-devices/container-device-interface/specs-go.ContainerEdits { Env: []string len: 1, cap: 1, [ "user_a=b", ], DeviceNodes: []*github.com/container-orchestrated-devices/container-device-interface/specs-go.DeviceNode len: 0, cap: 0, nil, Hooks: []*github.com/container-orchestrated-devices/container-device-interface/specs-go.Hook len: 0, cap: 0, nil, Mounts: []*github.com/container-orchestrated-devices/container-device-interface/specs-go.Mount len: 0, cap: 0, nil,},} (dlv) p string(buffer) "cdiVersion: 0.5.0\ncontainerEdits: {}\ndevices:\n- containerEdits:\n" — Reply to this email directly, view it on GitHub <#77 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHDMQNVP7ZVC6AUA3C4BDTWGU7T7ANCNFSM56B55WTA> . You are receiving this because you modified the open/close state.Message ID: <container-orchestrated-devices/container-device-interface/pull/77/c1303897573 @github.com>

pohly · 2022-11-04T18:05:57Z

I found a solution (manual encoding, use version 0.2.0 instead of 0.5.0, use JSON instead of YAML). I filed issues for future investigations.

elezar · 2022-11-04T18:09:48Z

0.2.0 is pretty old. Not sure whether the version is checked anywhere.. As long as you're unblocked we can look into this on Monday.

…

On Fri, Nov 4, 2022, 19:06 Patrick Ohly ***@***.***> wrote: I found a solution (manual encoding, use version 0.2.0 instead of 0.5.0, use JSON instead of YAML). I filed issues for future investigations. — Reply to this email directly, view it on GitHub <#77 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHDMQJADCO6SMVBZBJXIEDWGVGA7ANCNFSM56B55WTA> . You are receiving this because you modified the open/close state.Message ID: <container-orchestrated-devices/container-device-interface/pull/77/c1303961862 @github.com>

pohly reviewed Aug 10, 2022

View reviewed changes

pkg/cdi/cache.go Outdated Show resolved Hide resolved

pkg/cdi/cache.go Outdated Show resolved Hide resolved

klihub force-pushed the transient-specs branch from 01f1e28 to f183951 Compare August 10, 2022 06:10

pohly mentioned this pull request Aug 10, 2022

kubelet plugin pohly/kubernetes#12

Closed

klihub force-pushed the transient-specs branch from f183951 to c1d5064 Compare August 10, 2022 13:24

klihub changed the title ~~pkg/cdi: add functions for handling transient Specs.~~ pkg/cdi: add functions for generating Spec names. Aug 10, 2022

klihub force-pushed the transient-specs branch from c1d5064 to 93a7e7a Compare August 10, 2022 14:06

bart0sh reviewed Aug 10, 2022

View reviewed changes

pkg/cdi/spec.go Outdated Show resolved Hide resolved

klihub force-pushed the transient-specs branch from 93a7e7a to 8abef17 Compare August 10, 2022 19:43

bart0sh reviewed Aug 10, 2022

View reviewed changes

pkg/cdi/cache.go Show resolved Hide resolved

bart0sh reviewed Aug 10, 2022

View reviewed changes

pkg/cdi/spec.go Outdated Show resolved Hide resolved

klihub force-pushed the transient-specs branch 3 times, most recently from 4b74fc5 to 63de3f7 Compare August 12, 2022 09:00

klihub marked this pull request as ready for review August 12, 2022 09:38

klihub changed the title ~~pkg/cdi: add functions for generating Spec names.~~ pkg/cdi: add functions for generating Spec names and removing Spec files. Aug 15, 2022

klihub force-pushed the transient-specs branch from 63de3f7 to 53baa32 Compare August 15, 2022 08:28

klihub requested a review from bart0sh August 15, 2022 11:02

klihub force-pushed the transient-specs branch from 6029c04 to b986ea7 Compare August 17, 2022 08:50

bart0sh reviewed Aug 17, 2022

View reviewed changes

pkg/cdi/doc.go Outdated Show resolved Hide resolved

bart0sh reviewed Aug 17, 2022

View reviewed changes

pkg/cdi/doc.go Outdated Show resolved Hide resolved

bart0sh reviewed Aug 17, 2022

View reviewed changes

pkg/cdi/spec.go Outdated Show resolved Hide resolved

klihub force-pushed the transient-specs branch from b986ea7 to e8560f9 Compare August 17, 2022 12:26

klihub force-pushed the transient-specs branch from 6e955f2 to 0dd793f Compare August 30, 2022 06:34

klihub added 2 commits September 27, 2022 14:24

pkg/cdi: fix incorrect WriteSpec docstring.

c6bf82c

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

pkg/cdi: add RemoveSpec() for removing Spec files.

c5a7189

RemoveSpec() can be used to remove a Spec file written by WriteSpec(). It takes the same name that was passed to WriteSpec() to create the file. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

klihub force-pushed the transient-specs branch from 0dd793f to fbf3535 Compare September 27, 2022 11:26

pohly reviewed Sep 29, 2022

View reviewed changes

pkg/cdi/doc.go Outdated Show resolved Hide resolved

klihub force-pushed the transient-specs branch from fbf3535 to 791ea8f Compare October 24, 2022 14:31

elezar approved these changes Oct 26, 2022

View reviewed changes

pkg/cdi/spec.go Outdated Show resolved Hide resolved

pkg/cdi/spec.go Outdated Show resolved Hide resolved

klueska reviewed Oct 27, 2022

View reviewed changes

pkg/cdi/doc.go Outdated Show resolved Hide resolved

klihub force-pushed the transient-specs branch 2 times, most recently from 1bbc039 to 9ce34af Compare October 27, 2022 18:17

klihub added 4 commits October 27, 2022 21:24

pkg/cdi: default to YAML spec encoding.

7c8505c

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

pkg/cdi: expose [specs.]CurrentVersion.

a5fbc2f

Alias specs.CurrentVersion as pkg/cdi.CurrentVersion. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

klihub force-pushed the transient-specs branch from 9ce34af to a5fbc2f Compare October 27, 2022 18:24

elezar self-requested a review October 28, 2022 13:39

elezar approved these changes Oct 28, 2022

View reviewed changes

elezar merged commit cbac83c into cncf-tags:main Oct 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/cdi: add functions for generating Spec names and removing Spec files. #77

pkg/cdi: add functions for generating Spec names and removing Spec files. #77

klihub commented Aug 9, 2022 •

edited

Loading

klihub commented Aug 9, 2022 •

edited

Loading

klueska commented Aug 10, 2022

klihub commented Aug 10, 2022

bart0sh commented Aug 16, 2022 •

edited

Loading

klihub commented Aug 16, 2022 •

edited

Loading

bart0sh commented Aug 17, 2022

klihub commented Aug 17, 2022

klihub commented Aug 17, 2022

bart0sh commented Sep 28, 2022

pohly left a comment

bart0sh commented Oct 25, 2022

bart0sh commented Oct 25, 2022

pohly commented Oct 25, 2022

bart0sh commented Oct 25, 2022

elezar left a comment

elezar commented Oct 26, 2022

elezar left a comment

elezar Oct 28, 2022

pohly commented Nov 4, 2022

pohly commented Nov 4, 2022

pohly commented Nov 4, 2022

elezar commented Nov 4, 2022 via email

pohly commented Nov 4, 2022

elezar commented Nov 4, 2022 via email

pkg/cdi: add functions for generating Spec names and removing Spec files. #77

pkg/cdi: add functions for generating Spec names and removing Spec files. #77

Conversation

klihub commented Aug 9, 2022 • edited Loading

klihub commented Aug 9, 2022 • edited Loading

klueska commented Aug 10, 2022

klihub commented Aug 10, 2022

bart0sh commented Aug 16, 2022 • edited Loading

klihub commented Aug 16, 2022 • edited Loading

bart0sh commented Aug 17, 2022

klihub commented Aug 17, 2022

klihub commented Aug 17, 2022

bart0sh commented Sep 28, 2022

pohly left a comment

Choose a reason for hiding this comment

bart0sh commented Oct 25, 2022

bart0sh commented Oct 25, 2022

pohly commented Oct 25, 2022

bart0sh commented Oct 25, 2022

elezar left a comment

Choose a reason for hiding this comment

elezar commented Oct 26, 2022

elezar left a comment

Choose a reason for hiding this comment

elezar Oct 28, 2022

Choose a reason for hiding this comment

pohly commented Nov 4, 2022

pohly commented Nov 4, 2022

pohly commented Nov 4, 2022

elezar commented Nov 4, 2022 via email

pohly commented Nov 4, 2022

elezar commented Nov 4, 2022 via email

klihub commented Aug 9, 2022 •

edited

Loading

klihub commented Aug 9, 2022 •

edited

Loading

bart0sh commented Aug 16, 2022 •

edited

Loading

klihub commented Aug 16, 2022 •

edited

Loading