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

Revert "passt: Build and publish Passt binding CNI image" #81

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

Conversation

RamLavi
Copy link
Contributor

@RamLavi RamLavi commented Dec 16, 2024

What this PR does / why we need it:
This reverts commit 23eed84.

The passt solution was pivoted from passt binding to l2-bridge which does not require a DS deploying its binary.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

Remove Passt binding CNI image

@kubevirt-bot kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Dec 16, 2024
@kubevirt-bot kubevirt-bot requested a review from oshoval December 16, 2024 10:34
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign maiqueb for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@oshoval
Copy link
Collaborator

oshoval commented Dec 16, 2024

once you do that, you will need please to adapt to managedTap (i have a PR about it, which still need some bit to be changed)

and to adapt CNAO to be able to use managedTap (by patching kuebvirt CR)
and of course to remove all passt relevant code from CNAO please

@RamLavi
Copy link
Contributor Author

RamLavi commented Dec 16, 2024

once you do that, you will need please to adapt to managedTap (i have a PR about it, which still need some bit to be changed)

and to adapt CNAO to be able to use managedTap (by patching kuebvirt CR) and of course to remove all passt relevant code from CNAO please

So this PR is blocked by #73 right?

@oshoval
Copy link
Collaborator

oshoval commented Dec 16, 2024

Yep

@RamLavi
Copy link
Contributor Author

RamLavi commented Dec 16, 2024

once you do that, you will need please to adapt to managedTap (i have a PR about it, which still need some bit to be changed)

and to adapt CNAO to be able to use managedTap (by patching kuebvirt CR) and of course to remove all passt relevant code from CNAO please

anything to do on HCO, or you already did that part?

@oshoval
Copy link
Collaborator

oshoval commented Dec 16, 2024

yep, HCO is done already wrt using l2bridge (we don't have generic name nowhere yet)

@oshoval
Copy link
Collaborator

oshoval commented Dec 16, 2024

ah HCO need to remove passt, this wasnt done
it can be done orthogonal to CNAO / ipam please

@RamLavi
Copy link
Contributor Author

RamLavi commented Dec 16, 2024

@oshoval do we have anything to do with the disk space issue on CI?

@oshoval
Copy link
Collaborator

oshoval commented Dec 16, 2024

#80 was merged
so now just rebase please (retest is not enough)

@RamLavi RamLavi closed this Dec 16, 2024
@RamLavi RamLavi reopened this Dec 16, 2024
@maiqueb
Copy link
Collaborator

maiqueb commented Dec 16, 2024

/lgtm @qinqon thoughts ?

@oshoval
Copy link
Collaborator

oshoval commented Dec 16, 2024

the right flow will be first to make sure #73 lands on CNAO safely imo
else we miss stuff

first good to switch to managedTap all over and then to remove passt
else it is risky to stall / break things a bit

@qinqon
Copy link
Collaborator

qinqon commented Dec 16, 2024

We are not cosuming this on CNAO until we don't label so we good.

@qinqon
Copy link
Collaborator

qinqon commented Dec 16, 2024

/lgtm

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

oshoval commented Dec 16, 2024

but we will issue now a label because we need it to adapt CNAO for managedTap and see all is good
and it will break unless all is done on one step, which isn't good to enforce that

/hold
i think it is wrong
if you want remove the hold

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2024
@oshoval
Copy link
Collaborator

oshoval commented Dec 16, 2024

/hold cancel

since it is not approved no need hold, but i still think it is wrong

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2024
@qinqon
Copy link
Collaborator

qinqon commented Dec 16, 2024

/hold

again

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2024
@oshoval
Copy link
Collaborator

oshoval commented Dec 17, 2024

I would also say it is best to remove this please from right to left
i.e if we have
IPAM CNAO x y z (where each element uses the previous)
then removal order is Z down to IPAM

EDIT - note that some parts of that might need to be done at once.
The above is the 2nd phase, as agreed offline between Miguel and Quique, first phase is to adapt
CNAO to use managedTap please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants