-
Notifications
You must be signed in to change notification settings - Fork 348
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
UDN: Patch Kubevirt CR to support managedTap binding #4773
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you only need one in upstream OVN-K, right ?
True, i just wanted to do it smoother, i.e that as long as IPAM repo is not updated, it wont break it |
Whatever is both conceptually correct and simpler for you. |
Removed the passt here - going to test it on the dummy PR oshoval#3 also added the PR for IPAM that works on CI as well kubevirt/ipam-extensions#73 |
injected to use nightly |
either i did something wrong or the managedTap is broken on nightly kubevirt |
added new required FG, lets see it works |
Hi Miguel, please disregard the re-review request, we need anyhow first to discuss offline about generic name and such |
contrib/kind-common
Outdated
@@ -328,6 +328,8 @@ install_kubevirt() { | |||
# nightly tag - install specific nightly (i.e 20240910) | |||
KUBEVIRT_VERSION=${KUBEVIRT_VERSION:-"stable"} | |||
|
|||
KUBEVIRT_VERSION=nightly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well we would need to consume nightly or to release kubevirt or to pin it to one that has all the required code
but for sure can make it on its own commit nicely
failures are not related, the UDN ones passed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions.
|
||
local kubevirt_stable_release_url=$(get_kubevirt_release_url "stable") | ||
local passt_binding_image="quay.io/kubevirt/network-passt-binding:${kubevirt_stable_release_url##*/}" | ||
kubectl -n kubevirt patch kubevirt kubevirt --type=json --patch '[{"op":"add","path":"/spec/configuration/network","value":{}},{"op":"add","path":"/spec/configuration/network/binding","value":{"passt":{"computeResourceOverhead":{"requests":{"memory":"500Mi"}},"migration":{"method":"link-refresh"},"networkAttachmentDefinition":"default/primary-udn-kubevirt-binding","sidecarImage":"'"${passt_binding_image}"'"}}}]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't we want both of them ? and let the user pick which binding to use ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just because you asked so i dropped
i can return it if desired as we spoke
} else { | ||
return kubevirt.LoginToFedoraWithHostname(vmi, "core", "fedora", "localhost") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did this binding type changed the prompt ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since with the new one the DHCP populate now the right hostname, no need this special treatment that was to workaround this when passt is used (when the hostname is not set, prompt has localhost
instead vm name, this what the code that was removed done, and not needed now)
Quique is familiar with it best, and said it is good btw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to me, it seems like we first need to decide if we want to keep both bindings around or not.
like, if we keep passt around, we can't have this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those are the e2e tests, in tests as long as we dont test both passt and managedTap we can have just one here,
(otherwise there should be another condition here based on the binding, but i dont think we want to test both upstream, nor to just have support for one that isnt tested by a new condition on the used binding)
it is orthogonal to have both on the CR and then the user can still choose, this can be done anyhow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to add some context, passt do not support the DHCPv4 option hostname (12), so VM ends up with localhost, also is good that we test this kind of things since at managedTap we pass that with the OVN DHCP.
Signed-off-by: Or Shoval <oshoval@redhat.com>
Since hostname is populated now via DHCP, we can always use the vmi name in the console expecter. Signed-off-by: Or Shoval <oshoval@redhat.com>
Until we have a new stable release, this will allow us to have the new required managedTap and dynamic interface name detection that were presented by kubevirt. Signed-off-by: Or Shoval <oshoval@redhat.com>
Updated as we spoke offline Note please the 3rd commit, if we dont want nightly because it might break OVN if kubevirt breaks, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good as far as CI is happy.
@@ -326,7 +326,7 @@ install_kubevirt() { | |||
# vX.Y.Z - install specific stable (i.e v1.3.1) | |||
# nightly - install newest nightly | |||
# nightly tag - install specific nightly (i.e 20240910) | |||
KUBEVIRT_VERSION=${KUBEVIRT_VERSION:-"stable"} | |||
KUBEVIRT_VERSION=${KUBEVIRT_VERSION:-"nightly"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we wait for a kubevirt release ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will ask if a special one can be created but i doubt as the minor should change maybe,
will also ask when the stable is out, it might take time
imho we can use 20241029
, it is safer, see please
#4773 (comment)
we will just need to update once there is a stable and wont be blocked
but your call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/kubevirt/sig-release/blob/main/releases/v1.4/schedule.md
lets wait, rc should be any day
iiuc it will appear on https://storage.googleapis.com/kubevirt-prow/release/kubevirt/kubevirt/stable.txt that we use by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disregard, stable.txt wont have it, checking about alternatives (i.e new file on that path etc)
} else { | ||
return kubevirt.LoginToFedoraWithHostname(vmi, "core", "fedora", "localhost") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to add some context, passt do not support the DHCPv4 option hostname (12), so VM ends up with localhost, also is good that we test this kind of things since at managedTap we pass that with the OVN DHCP.
What this PR does and why is it needed
Use managedTap instead passt for UDN
Which issue(s) this PR fixes
Fixes #
Special notes for reviewers
How to verify it
Details to documentation updates
Description for the changelog
Does this PR introduce a user-facing change?