Skip to content

[DNM] pkg/cdi: fixes and cleanups - #344

Draft
thaJeztah wants to merge 14 commits into
cncf-tags:mainfrom
thaJeztah:cdi_cleans
Draft

[DNM] pkg/cdi: fixes and cleanups#344
thaJeztah wants to merge 14 commits into
cncf-tags:mainfrom
thaJeztah:cdi_cleans

Conversation

@thaJeztah

@thaJeztah thaJeztah commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

pkg/cdi: simplify some code with slices, maps packages

Both slices.Clone and maps.Clone already handles nil values, so we can
skip our own checks, making the code slightly more straightforward.

Also update GetSpecDirErrors to make sure we perform the copy after
obtaining a lock.

pkg/cdi: clean up watcher lifecycle handling

  • Stop the existing watcher before applying new cache options and resetting
    watcher-related errors, so the old watcher cannot observe partially updated
    configuration.
  • Clear the watcher after closing so that we don't leave behind a stale,
    closed watcher.
  • Combine watcher setup and startup so creating the fsnotify watcher,
    installing the initial directory watches, and starting the event loop happen
    as a single operation.
  • Install the initial watches explicitly instead of routing startup through
    watch.update, and only initialize watch.tracked after successfully
    creating the watcher.
  • Avoid starting the watch goroutine if creating the watcher failed.
  • Remove the now-redundant nil check from watch.watch and use its fsw
    argument directly.

pkg/cdi: simplify refresh logic

All callers of refreshIfRequired, except Refresh, only use it to perform
an automatic refresh when necessary and ignore refresh errors. Refresh,
on the other hand, needs to force a refresh in manual mode and return the
cached errors regardless of whether a refresh took place.

Move the logic to decide between "automatic" or "forced" refresh to the
Refresh method, and move collecting the cached / refreshed errors to
that method. The responsibility of Cache.refresh is now reduced to
refreshing the data and errors, and it's up to callers to collect errors
when needed.

pkg/cdi: scope variables and move them closer to where used

pkg/cdi: ListClasses: combine loops

pkg/cdi: handle fsnotify operations as a bitmask

fsnotify.Event.Op is a bitmask and can contain multiple operations, but the
watcher compared it directly against individual operation values.

As a result, combined events could bypass operation-specific handling, such as
filtering writes and creates by file extension or detecting removal of a
watched directory.

Mask the operations we are interested in and test the individual bits instead.

pkg/cdi: ignore unrelated filesystem events

The watcher filtered write and create events by file extension, but rename
and remove events for unrelated files would still trigger a refresh of the
CDI cache.

Apply the same Spec-file filtering to all relevant filesystem events, while
still processing events for configured Spec directories themselves.

This avoids unnecessary rescans when non-Spec files in watched directories are
renamed or removed, without interfering with handling of removed Spec
directories.

WIP: cleanup / fix watch

@thaJeztah

Copy link
Copy Markdown
Contributor Author

Let me just push the branch I had locally for my own sanity; I think I had some work remaining (last commit), but started to loose track of splitting out to small PRs (for easier review).

@thaJeztah

Copy link
Copy Markdown
Contributor Author

Ah, right; I needed a go bump for some bits;

Error: pkg/cdi/cache.go:354:16: stdversion: slices.Sorted requires go1.23 or later (module is go1.21) (govet)
  	return slices.Sorted(maps.Keys(c.devices))
  	              ^
  Error: pkg/cdi/cache.go:365:16: stdversion: slices.Sorted requires go1.23 or later (module is go1.21) (govet)
  	return slices.Sorted(maps.Keys(c.specs))
  	              ^

Add a matrix to run tests against the oldest supported version (as
specified in go.mod), as well as current and previous stable Go.

Use a custom name for the job so that the names are stable, which
helps when configuring GitHub branch policies.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
InjectDevices only needs to hold the lock while refreshing the cache and
capturing the current devices map. Release the lock before resolving devices
to reduce lock contention while applying the container edits on the passed-in
OCI spec.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This is a constructor, working on a freshly created `Cache`, so there
should be no concurrency to account for. While updating, also set the
default specDirs directly from a clone of `DefaultSpecDirs`, instead of
applying through `WithSpecDirs` to avoid the extra indirect.

Paths are still cleaned, to account for the `DefaultSpecDirs` being
a mutable, package-level variable (but generally not recommended to
update).

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Rename vars that shadowed imports, and make sure that the returned errors
are properly dereferenced. Also append directory errors instead of overwriting
existing errors for the same path.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Both slices.Clone and maps.Clone already handles `nil` values, so we can
skip our own checks, making the code slightly more straightforward.

Also update GetSpecDirErrors to make sure we perform the copy after
obtaining a lock.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- Stop the existing watcher before applying new cache options and resetting
  watcher-related errors, so the old watcher cannot observe partially updated
  configuration.
- Clear the watcher after closing so that we don't leave behind a stale,
  closed watcher.
- Combine watcher setup and startup so creating the fsnotify watcher,
  installing the initial directory watches, and starting the event loop happen
  as a single operation.
- Install the initial watches explicitly instead of routing startup through
  `watch.update`, and only initialize `watch.tracked` after successfully
  creating the watcher.
- Avoid starting the watch goroutine if creating the watcher failed.
- Remove the now-redundant nil check from `watch.watch` and use its `fsw`
  argument directly.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
All callers of refreshIfRequired, except `Refresh`, only use it to perform
an automatic refresh when necessary and ignore refresh errors. `Refresh`,
on the other hand, needs to force a refresh in manual mode and return the
cached errors regardless of whether a refresh took place.

Move the logic to decide between "automatic" or "forced" refresh to the
`Refresh` method, and move collecting the cached / refreshed errors to
that method. The responsibility of `Cache.refresh` is now reduced to
refreshing the data and errors, and it's up to callers to collect errors
when needed.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
`fsnotify.Event.Op` is a bitmask and can contain multiple operations, but the
watcher compared it directly against individual operation values.

As a result, combined events could bypass operation-specific handling, such as
filtering writes and creates by file extension or detecting removal of a
watched directory.

Mask the operations we are interested in and test the individual bits instead.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The watcher filtered write and create events by file extension, but rename
and remove events for unrelated files would still trigger a refresh of the
CDI cache.

Apply the same Spec-file filtering to all relevant filesystem events, while
still processing events for configured Spec directories themselves.

This avoids unnecessary rescans when non-Spec files in watched directories are
renamed or removed, without interfering with handling of removed Spec
directories.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant