Skip to content

Update CNI and CNI plugins #268

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update CNI and CNI plugins #268

wants to merge 1 commit into from

Conversation

saschagrunert
Copy link
Member

What type of PR is this?

/kind dependency-change

What this PR does / why we need it:

Update CNI and the plugins to the latest release.

Which issue(s) this PR fixes:

Fixes #259

Special notes for your reviewer:

None

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/dependency-change Categorizes issue or PR as related to changing dependencies labels Aug 19, 2025
@openshift-ci openshift-ci bot requested review from mrunalp and sameo August 19, 2025 08:12
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Aug 19, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: saschagrunert

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 Aug 19, 2025
@coveralls
Copy link

coveralls commented Aug 19, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 17370686602

Details

  • 7 of 14 (50.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 61.695%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/ocicni/ocicni.go 7 14 50.0%
Totals Coverage Status
Change from base Build 17315370839: 0.04%
Covered Lines: 575
Relevant Lines: 932
💛 - Coveralls

@saschagrunert saschagrunert force-pushed the cni-and-plugins branch 3 times, most recently from b4ede86 to 71e9a01 Compare August 19, 2025 08:27
@saschagrunert
Copy link
Member Author

@cri-o/cri-o-maintainers PTAL, we have to cut a new release after this.

bitoku
bitoku reviewed Aug 19, 2025
View reviewed changes
pkg/ocicni/ocicni.go Outdated
Comment on lines 500 to 503
return nil, nil, err
}

if cniNet.config.Plugins == nil {
Copy link

Choose a reason for hiding this comment

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

When the confBytes has an empty plugins (not nil), it loads plugins before change, but it returns an error after change. Is that expected?

https://github.com/containernetworking/cni/blob/260917dab05ca7352c90c97a3537f8c808aa56b2/libcni/conf.go#L229-L231

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm working around exactly this issue by omitting the error check. This could be a regression in CNI from my point of view.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or rather an regression more than a behavioral change from the API.

Copy link

Choose a reason for hiding this comment

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

It seems only when it meets this condition, plugins can be 0-length slice or nil.
https://github.com/containernetworking/cni/blob/260917dab05ca7352c90c97a3537f8c808aa56b2/libcni/conf.go#L221-L223

It looks ok to me, but I want to wait a bit for others to look. @cri-o/ocicni-maintainers @cri-o/cri-o-maintainers

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2025
@saschagrunert
Update CNI and CNI plugins
c343694 
Signed-off-by: Sascha Grunert <[email protected]>
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrunalp mrunalp Awaiting requested review from mrunalp

@sameo sameo Awaiting requested review from sameo

1 more reviewer

@bitoku bitoku bitoku left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
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/dependency-change Categorizes issue or PR as related to changing dependencies release-note-none Denotes a PR that doesn't merit a release note.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update CNI to v1.3.0
4 participants
@saschagrunert @coveralls @bitoku @openshift-merge-robot