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

Update libcni to latest version #115

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

MikeZappa87
Copy link
Contributor

@MikeZappa87 MikeZappa87 commented Feb 27, 2024

Updating libcni to support latest version

@MikeZappa87 MikeZappa87 changed the title Add additional fields to interface struct [DRAFT] Add additional fields to interface struct Apr 15, 2024
@MikeZappa87 MikeZappa87 force-pushed the updateinterface branch 2 times, most recently from 078beec to 525aa26 Compare May 20, 2024 17:32
@MikeZappa87 MikeZappa87 changed the title [DRAFT] Add additional fields to interface struct [DRAFT] Update libcni to latest version May 20, 2024
@MikeZappa87 MikeZappa87 force-pushed the updateinterface branch 2 times, most recently from 84261d7 to 240a77e Compare May 20, 2024 17:43
@MikeZappa87 MikeZappa87 changed the title [DRAFT] Update libcni to latest version Update libcni to latest version May 20, 2024
@MikeZappa87 MikeZappa87 self-assigned this May 20, 2024
@MikeZappa87 MikeZappa87 requested a review from mikebrow May 20, 2024 17:44
@MikeZappa87 MikeZappa87 marked this pull request as ready for review May 20, 2024 17:44
@MikeZappa87 MikeZappa87 force-pushed the updateinterface branch 2 times, most recently from 7d2140e to 59ac461 Compare May 20, 2024 17:48
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

Need a TestLibCNIType120 see:
https://github.com/containerd/go-cni/pull/115/files#diff-6cdf85f441267859d068399676e5f0ad8946811bd7e951b4e80db98d9fd2489cL200

may help draw out any issues that may arise as we move up..

Also we should open an issue for the new libcni1.2 changes that need further work..

  • GC verb - allows runtimes to specify the set of known-good attachments, allowing plugins to clean up stale and leaked resources such as IPAM reservations. Libcni will also synthesize a CNI DEL for any stale cached attachments, so all users will have a form of GC, even if their plugins do not support CNI v1.1
  • The STATUS verb allows a plugin to report its readiness to accept ADD requests.
  • config now includes Version negotiation w/ new cniVersions
    "cniVersion": "1.0.0",
    "cniVersions": ["1.0.0", "1.1.0"]
    -GetVersion()
  • ** check if version imbalance is an issue now that libcni version 1.20 matches to cni version 1.1.0 .. make sure no issues for the ci/cd/build test/upgrade/downgrade scenarios

go.mod Outdated
github.com/stretchr/testify v1.8.0
)

require (
github.com/Masterminds/semver/v3 v3.2.1 // indirect
Copy link
Member

Choose a reason for hiding this comment

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

background discussion with CNI team if exposure of this package was on purpose .. check if a newer tagged version of this semver project is needed etc..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@squeed thoughts? We could just revert that pr unless we have a better route

Copy link
Member

Choose a reason for hiding this comment

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

Noting nil version equality fix for panic.. Masterminds/semver#213 is not in this tagged version. Would prefer using golang public []semver or []string if possible to reduce externalized package dependencies.

That said we can handle the two asks separately if desired by the CNI team.

@MikeZappa87
Copy link
Contributor Author

@squeed just want to get this to the top of your email

Signed-off-by: Michael Zappa <[email protected]>
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

Copy link
Member

@dims dims left a comment

Choose a reason for hiding this comment

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

LGTM

@mikebrow mikebrow merged commit 1c1be5e into containerd:main Jun 24, 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.

3 participants