Skip to content

fix: return success on DEL when CNI cache is missing #1498

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kvaps wants to merge 1 commit into from
+11 −25
Closed

fix: return success on DEL when CNI cache is missing #1498

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 11 additions & 25 deletions pkg/multus/multus.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -954,33 +954,19 @@ func CmdDel(args *skel.CmdArgs, exec invoke.Exec, kubeClient *k8s.ClientInfo) er
}

if !useCacheConf {
// Fetch delegates again if cache is not exist and pod info can be read
if os.IsNotExist(err) && pod != nil {
if in.ClusterNetwork != "" {
_, err = k8s.GetDefaultNetworks(pod, in, kubeClient, nil)
if err != nil {
return cmdErr(k8sArgs, "failed to get clusterNetwork/defaultNetworks: %v", err)
}
// First delegate is always the master plugin
in.Delegates[0].MasterPlugin = true
}

// Get pod annotation and so on
_, _, err := k8s.TryLoadPodDelegates(pod, in, kubeClient, nil)
if err != nil {
if len(in.Delegates) == 0 {
// No delegate available so send error
return cmdErr(k8sArgs, "failed to get delegates: %v", err)
}
// Get clusterNetwork before, so continue to delete
logging.Errorf("Multus: failed to get delegates: %v, but continue to delete clusterNetwork", err)
}
} else {
// The options to continue with a delete have been exhausted (cachefile + API query didn't work)
// We cannot exit with an error as this may cause a sandbox to never get deleted.
logging.Errorf("Multus: failed to get the cached delegates file: %v, cannot properly delete", err)
// If cache does not exist, ADD was never completed for this sandbox.
// Attempting DEL with a config reconstructed from the API will fail because
// delegate plugins (e.g. kube-ovn) require runtime state that only exists
// after a successful ADD (socket paths, cached results, etc.).
// Return success to allow containerd to release the sandbox name reservation.
if os.IsNotExist(err) {
logging.Verbosef("Multus: no cache found for container %s (ADD was never completed), skip DEL", args.ContainerID)
return nil
}
// The options to continue with a delete have been exhausted
// We cannot exit with an error as this may cause a sandbox to never get deleted.
logging.Errorf("Multus: failed to get the cached delegates file: %v, cannot properly delete", err)
return nil
Comment on lines +962 to +969

@gemini-code-assist gemini-code-assist Bot Mar 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The error logging and logic in this block can be improved for better observability and clarity:

  1. Context: The log message at line 968 loses the pod/container context. Including args.ContainerID (as done in the Verbosef call at line 963) would significantly help in identifying which container failed cleanup in the logs.
  2. Clarity: If err is nil (which occurs if the cache file was read successfully but failed to unmarshal at line 941), the log will show : <nil>, which is misleading. It should ideally distinguish between a filesystem error and a parsing failure.
  3. Simplification: Since both branches return nil, the logic can be combined to reduce redundancy and improve readability.
Suggested change
if os.IsNotExist(err) {
logging.Verbosef("Multus: no cache found for container %s (ADD was never completed), skip DEL", args.ContainerID)
return nil
}
// The options to continue with a delete have been exhausted
// We cannot exit with an error as this may cause a sandbox to never get deleted.
logging.Errorf("Multus: failed to get the cached delegates file: %v, cannot properly delete", err)
return nil
if os.IsNotExist(err) {
logging.Verbosef("Multus: no cache found for container %s (ADD was never completed), skip DEL", args.ContainerID)
} else {
// The options to continue with a delete have been exhausted
// We cannot exit with an error as this may cause a sandbox to never get deleted.
logging.Errorf("Multus: failed to get or parse the cached delegates file for container %s: %v, cannot properly delete", args.ContainerID, err)
}
return nil

}

// set CNIVersion in delegate CNI config if there is no CNIVersion and multus conf have CNIVersion.
Expand Down