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

add CNI GC support #197

Merged
merged 2 commits into from
Apr 23, 2024
Merged

add CNI GC support #197

merged 2 commits into from
Apr 23, 2024

Conversation

squeed
Copy link
Collaborator

@squeed squeed commented Apr 16, 2024

/kind feature

This adds CNI GC support. GC is a CNI verb that allows runtimes (i.e. cri-o) to specify a list of known-good attachments, allowing network plugins to clean up stale resources. This permits cleanup of, for example, leaked IPAM allocations.

This does require rethinking the locking approach slightly, as GC cannot be executed in paralle with other pod operations.

Add CNI GC support, which allows network plugins to clean up stale resources.

@openshift-ci openshift-ci bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Apr 16, 2024
@openshift-ci openshift-ci bot requested review from mrunalp and rajatchopra April 16, 2024 14:30
@coveralls
Copy link

coveralls commented Apr 16, 2024

Pull Request Test Coverage Report for Build 8797168456

Details

  • 81 of 99 (81.82%) changed or added relevant lines in 1 file are covered.
  • 7 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+1.1%) to 61.369%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/ocicni/ocicni.go 81 99 81.82%
Files with Coverage Reduction New Missed Lines %
pkg/ocicni/ocicni.go 7 71.74%
Totals Coverage Status
Change from base Build 8749394908: 1.1%
Covered Lines: 529
Relevant Lines: 862

💛 - Coveralls

Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

Code LGTM but the linter is not happy with that change.

@squeed
Copy link
Collaborator Author

squeed commented Apr 17, 2024

🤦 fixed, thanks!

squeed added 2 commits April 22, 2024 17:29
We have two locks:
- per-pod lock to prevent parallel operations
- plugin-level lock to prevent races on network configurations

We don't actually use the latter correctly; we could find ourselves in a
funny state where we read default network name A, but then it changes
before we issue the CNI ADD.

So, just hold the plugin read-lock during all pod operations. This
prevents any sort of racing between pod operations and config reloading.

An extra quirk is that we opportunistically re-sync config on failure.
This is to fix a podman test flake. It needs a bit of awkward re-locking
but it should rarely happen. It's also not a deadlock risk thanks to
careful ordering.

Signed-off-by: Casey Callendrello <c1@caseyc.net>
This introduces CNI GC support, a mode in which the runtime provides a
list of expected attachments, and then network plugins can clean up stale
resources such as IPAM attachments.

Because CNI GC is not allowed to take place in parallel with any other
pod operations, a new top-level lock was added. I did not use the plugin
lock, as GC should not block non-pod calls such as STATUS.

Signed-off-by: Casey Callendrello <c1@caseyc.net>
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

Thanks! Do we need a new release after this change?

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 23, 2024
Copy link
Contributor

openshift-ci bot commented Apr 23, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: saschagrunert, squeed

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 23, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit ca0b458 into cri-o:master Apr 23, 2024
5 checks passed
@squeed
Copy link
Collaborator Author

squeed commented Apr 23, 2024

Thanks! Do we need a new release after this change?

Eventually, yes. I’d like to prototype GC support in crio first; let me play at that for a little while.

is there a cri-o release imminent?

@saschagrunert
Copy link
Member

@squeed we're planning to cut v1.30.0 in the next week(s), yes. We still need go 1.22 in CI, so we may delay here a bit.

cc @haircommander

@squeed
Copy link
Collaborator Author

squeed commented May 6, 2024

@saschagrunert I haven't gotten time to check this, but that's OK. Cut a release at your leisure :-)

@saschagrunert
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants