Skip to content
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.

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

Spec, libcni: add disableGC flag #1090

Merged
merged 3 commits into from
Jun 10, 2024

Conversation

squeed
Copy link
Member

@squeed squeed commented May 13, 2024

This allows administrators to disable garbage collection in exceptional circumstances, such as multiple runtimes sharing a network configuration.

@squeed squeed requested a review from s1061123 May 13, 2024 19:52
@coveralls
Copy link

coveralls commented May 13, 2024

Coverage Status

coverage: 63.69% (+0.09%) from 63.596%
when pulling c283314 on squeed:gc-improvements
into cebd7df on containernetworking:main.

@s1061123
Copy link
Contributor

Should we modify pkg/types/types.go to add disableGC here?

https://github.com/squeed/cni/blob/a2d52a4f952c8f216b5bf8e8cc7542a8d275fd55/pkg/types/types.go#L117-L123

@henry118
Copy link
Member

Should we modify pkg/types/types.go to add disableGC here?

https://github.com/squeed/cni/blob/a2d52a4f952c8f216b5bf8e8cc7542a8d275fd55/pkg/types/types.go#L117-L123

Same question. However I don't see any references of type NetConfList. Wonder if we can just remove that type?

@s1061123
Copy link
Contributor

Should we modify pkg/types/types.go to add disableGC here?
https://github.com/squeed/cni/blob/a2d52a4f952c8f216b5bf8e8cc7542a8d275fd55/pkg/types/types.go#L117-L123

Same question. However I don't see any references of type NetConfList. Wonder if we can just remove that type?

This is not private scope, just public scope hence other repos use that, like https://github.com/k8snetworkplumbingwg/multus-cni/blob/75c02450209c749c67b571d1add5b0cf5b39d732/pkg/types/types.go#L101

So removing it causes lots of changes out side of this repo, under CNI plugins. So we should not do that.

squeed added 2 commits June 3, 2024 16:51
If there's no cache dir, there are no cached attachments to return. No
sense in propagating that error.

Signed-off-by: Casey Callendrello <[email protected]>
This flag allows administrators to disable garbage collection for
exceptional circumstances.

Signed-off-by: Casey Callendrello <[email protected]>
@squeed
Copy link
Member Author

squeed commented Jun 3, 2024

@henry118 Agreed, NetConfList doesn't seem to be used internally, so adding DisableCheck and DisableGC to it was a bit odd. But better to be consistent.

This allows administrators to disable GC behavior.

Signed-off-by: Casey Callendrello <[email protected]>
@squeed squeed force-pushed the gc-improvements branch from a2d52a4 to c283314 Compare June 3, 2024 14:58
Copy link
Member

@henry118 henry118 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@s1061123 s1061123 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@squeed squeed merged commit 73debca into containernetworking:main Jun 10, 2024
5 checks passed
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.

4 participants