Skip to content

Conversation

@ormergi
Copy link
Contributor

@ormergi ormergi commented Feb 6, 2024

What this PR does / why we need it:
Update the linux-bridge component to latest in order to bridge the latest changes of bridge CNI that include the new disableContainerInterface option containernetworking/plugins#997

Special notes for your reviewer:
Since there is no tagged version of container network plugins yet, consume that latest version from main branch containernetworking/plugins@14bdce5

Release note:

NONE

Since we dont have tagged version of plugins package
that containers bridge CNI version with the new
disableContainerInterface flag, consume that latest
version from main branch.

Signed-off-by: Or Mergi <[email protected]>
@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/XS labels Feb 6, 2024
PUSH_IMAGES=yes make bump-linux-bridge

Signed-off-by: Petr Horacek <[email protected]>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 6, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@ormergi
Copy link
Contributor Author

ormergi commented Feb 6, 2024

Verified on local env with latest Kubevirt, the correct bridge CNI is build and deployed by CNAO.
To verify the right bridge CNI version is deployed, I created a net-attach-def with the following net conf:

{   
    "type": "bridge",
    "bridge": "cryptbr",
    "disableContainerInterface": true,
    "ipam": {
        "type": "host-local",
        "subnet": "10.0.0.0/24"
    }
}

The VM is fail to create as expected because its not possible to set both IPAM and disableContainerInterface:

ERRORED: error configuring pod [default/virt-launcher-app-srv1-bzvxw] networking: [default/virt-launcher-app-srv1-bzvxw/9c0687bd-a78d-4239-8cf2-36717e9e2f6a:supersecretnet]: error adding container to network "supersecretnet": plugin type="bridge" failed (add): cannot use IPAM when DisableContainerInterface flag is set

Copy link
Collaborator

@RamLavi RamLavi left a comment

Choose a reason for hiding this comment

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

Hey @ormergi
Moving the update strategy to "static" is less preferable.
Can you explain why you need this disableContainerInterface option?
Is this change urgent? If not, can you try and reach out to https://github.com/containernetworking/plugins maintainers and kindly ask them to issue a release?

@ormergi
Copy link
Contributor Author

ormergi commented Feb 7, 2024

Hey @ormergi Moving the update strategy to "static" is less preferable. Can you explain why you need this disableContainerInterface option? Is this change urgent? If not, can you try and reach out to https://github.com/containernetworking/plugins maintainers and kindly ask them to issue a release?

Hi @RamLavi, thanks for looking at this 🙂
We need that latest version in order to have it deployed by CNAO on kubevirt CI, to address this Kubevirt bug https://issues.redhat.com/browse/CNV-28040
The gist of it is that the migration destination pod interface advertises about its existence before the migration is completed, causing traffic distortion during migration.
The latest bridge CNI bits together with kubevirt latest main solve this issue.

I am in touch with https://github.com/containernetworking/plugins maintainers and we will have it released eventually, once we do, I will post a PR to set CNAO to use the tagged version.
The PR is to address the intermediate state where we need the new bits

@RamLavi
Copy link
Collaborator

RamLavi commented Feb 8, 2024

Hey @ormergi Moving the update strategy to "static" is less preferable. Can you explain why you need this disableContainerInterface option? Is this change urgent? If not, can you try and reach out to https://github.com/containernetworking/plugins maintainers and kindly ask them to issue a release?

Hi @RamLavi, thanks for looking at this 🙂 We need that latest version in order to have it deployed by CNAO on kubevirt CI, to address this Kubevirt bug https://issues.redhat.com/browse/CNV-28040 The gist of it is that the migration destination pod interface advertises about its existence before the migration is completed, causing traffic distortion during migration. The latest bridge CNI bits together with kubevirt latest main solve this issue.

I am in touch with https://github.com/containernetworking/plugins maintainers and we will have it released eventually, once we do, I will post a PR to set CNAO to use the tagged version. The PR is to address the intermediate state where we need the new bits

I'm totally down with that. Please remember to set it back to tagged when a new tag is available.

Copy link
Collaborator

@RamLavi RamLavi left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 8, 2024
@kubevirt-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RamLavi

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

@kubevirt-bot kubevirt-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 8, 2024
@kubevirt-bot kubevirt-bot merged commit 827633e into kubevirt:main Feb 8, 2024
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. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants