Skip to content

Conversation

@qinqon
Copy link

@qinqon qinqon commented Jul 9, 2025

This PR fixes error handling in the GCNetworkList function where it was returning nil instead of the actual error from GetCachedAttachments, causing silent failures.

Changes

  • Modified GCNetworkList function in libcni/api.go to return the actual error from GetCachedAttachments instead of nil
  • This ensures proper error propagation and prevents silent failures during garbage collection

Context

The issue was located around lines 772-775 in libcni/api.go where the error handling incorrectly returned nil instead of the error when GetCachedAttachments failed.

Testing

  • Fixed function now properly propagates errors from GetCachedAttachments
  • Error handling is consistent with other functions in the codebase

Return actual error from GetCachedAttachments instead of nil to properly
propagate errors and avoid silent failures when getting cached attachments
fails during garbage collection.

Signed-off-by: Enrique Llorente <[email protected]>
@qinqon qinqon force-pushed the fix-gc-network-list-error-handling branch from f09423c to 2b26a54 Compare July 9, 2025 14:38
@MikeZappa87
Copy link
Contributor

cni/SPEC.md

Line 384 in ee7c96f

Plugins should generally complete a `GC` action without error. If an error is encountered, a plugin should continue; removing as many resources as possible, and report the errors back to the runtime.

@squeed looks like the we should be returning an error here, referring to the spec link above

cachedAttachments, err := c.GetCachedAttachments("")
if err != nil {
return nil
return err
Copy link
Member

Choose a reason for hiding this comment

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

should we return err, or skip straight to the GC call?

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.

3 participants