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

🌱 Add KubeVirt support to Tilt dev workflow #11697

Merged
merged 2 commits into from
Mar 20, 2025

Conversation

johananl
Copy link
Member

@johananl johananl commented Jan 17, 2025

What this PR does / why we need it:

This PR adds support for local development using KubeVirt as an alternative to CAPD.

This is useful in cases where CAPD can't be used for whatever reason, with one example being developing Ignition-related features (since Ignition runs in early boot and therefore can't be containerized easily).

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

/area devtools

@k8s-ci-robot k8s-ci-robot added the area/devtools Issues or PRs related to devtools label Jan 17, 2025
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 17, 2025
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 17, 2025
@johananl johananl changed the title Add KubeVirt instructions to tilt.md 📖 Add KubeVirt instructions to tilt.md Jan 17, 2025
@sbueringer
Copy link
Member

Is there someone who is familiar with KubeVirt that could do a first review?

@nunnatsa
Copy link
Contributor

nunnatsa commented Jan 22, 2025

Hi @johananl

Thanks for this PR. As far as I can see, this is based on the KubeVirt sections in the quickstart guide.

I just opened a PR to update it, as it didn't work for me now (I was working when I wrote it...). Maybe you want to take a look?

#11734

Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

Thanks for writing this down.
It will be great if we can do a further step and re-use the existig scrpt for creating kind + registry as well as create a new script for automating most of the steps described in this doc

@johananl johananl force-pushed the tilt-kubevirt branch 9 times, most recently from a560441 to 7bbd562 Compare January 30, 2025 15:01
@johananl
Copy link
Member Author

@fabriziopandini I've refactored the PR and converted most of the instructions to code. We now have make kind-cluster-kubevirt which does almost everything. I've edited the guide accordingly.

Thanks for the suggestion 👍

@johananl johananl changed the title 📖 Add KubeVirt instructions to tilt.md 🌱 Add KubeVirt support to Tilt dev workflow Jan 30, 2025
Copy link
Contributor

@mboersma mboersma left a comment

Choose a reason for hiding this comment

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

/lgtm

The changes make sense to me. I tried this out with make kind-cluster-kubevirt and everything seems to work as advertised in Tilt. I did make serve to follow through the docs, and I think they're clear and the tab formatting works fine.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: b986b25002289d3e30b1412e76e01616859efe9d

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 4, 2025
@k8s-ci-robot k8s-ci-robot requested a review from mboersma February 4, 2025 10:51
@sbueringer sbueringer added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 4, 2025
@sbueringer
Copy link
Member

Thx!

/lgtm

/assign @nunnatsa @fabriziopandini

@johananl johananl force-pushed the tilt-kubevirt branch 2 times, most recently from 56bb5e7 to fe92230 Compare February 4, 2025 14:56
Copy link
Member

@chrischdi chrischdi left a comment

Choose a reason for hiding this comment

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

Just some comments on the doc.

Great work on the bash scripts!

@johananl
Copy link
Member Author

I think I've addressed all the feedback. Happy to get a final review 🙏

Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

just few nits otherwise lgtm, but I'm not sure when I will have bandwidth to test, hopefully someone could get there before me

Comment on lines 72 to 75
while [[ -z $(kubectl -n metallb-system get pods \
-l app=metallb,component=controller -o jsonpath="{.items[0].metadata.name}" 2>/dev/null) ]]; do
sleep 2
done
Copy link
Member

Choose a reason for hiding this comment

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

It seems to waiting for MetalLB Deployment and DaemonSet pods can be a bit simplified with kubectl wait option.

kubectl wait --for condition=available deployment controller  -n metallb-system --timeout 150s
kubectl wait --for condition=ready pod -l app=metallb,component=speaker -n metallb-system --timeout 150s

Copy link
Member Author

@johananl johananl Mar 6, 2025

Choose a reason for hiding this comment

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

I used the loop because with kubectl wait I kept getting errors because the script was trying to check for Ready status on a non-existent pod. We need to allow some time for the deployment or daemon set controller to create the pods.

Good point regarding checking for available status on the deployment rather than the pods. This works. But looks like we don't have an equivalent for daemon sets.

We could do what you suggested and check for available deployment and ready pod, however there will be a race between speaker pod creation and the wait on speaker pod.

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've adopted your suggestion for now to make progress. Maybe we can leave the race be since it's probably quite rare that it would trip someone.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, totally agree about potential race between controller and speaker pods.

Probably, I've found solution for the DaemonSet wait for ready condition:

kubectl wait -n metallb-system daemonsets.apps speaker --for=jsonpath='{.status.numberReady}'=$(kubectl get daemonsets.apps speaker -n metallb-system  -ojsonpath='{.status.currentNumberScheduled}'

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, but it doesn't work: When trying to run

kubectl wait pods -n metallb-system -l app=metallb,component=speaker --for condition=Ready --timeout 5m

right after

kubectl wait -n metallb-system daemonsets.apps speaker \
  --for jsonpath='{.status.numberReady}'="$(kubectl get daemonsets.apps speaker -n metallb-system -ojsonpath='{.status.currentNumberScheduled}')" --timeout 5m
error: no matching resources found

Copy link
Member

Choose a reason for hiding this comment

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

It's strange, but let's keep the original approach. Thank you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I've already adopted your suggestion partially, just without waiting for the daemonset (see current state of code). Think we should go back to the sleep loops?

Copy link
Member

Choose a reason for hiding this comment

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

No, adopted part is good. Without waiting for the DaemonSet approach, as it's seems to not working in some cases.

Probably this code should work. I tried to run it locally and it's working for me

kubectl wait -n metallb-system daemonsets.apps -l app=metallb,component=speaker --for=jsonpath='{.status.numberReady}'=$(kubectl get daemonsets.apps -l app=metallb,component=speaker -n metallb-system  -ojsonpath='{.items[0].status.currentNumberScheduled}')

Copy link
Member Author

Choose a reason for hiding this comment

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

It's "working" for me only because by the time the controller deployment has converged the speaker DS has converged, too. I ran

kubectl wait -n metallb-system daemonsets.apps -l app=metallb,component=speaker \
  --for=jsonpath='{.status.numberReady}'="$(kubectl get daemonsets.apps -l app=metallb,component=speaker -n metallb-system  -ojsonpath='{.items[0].status.currentNumberScheduled}')"

kubectl wait pods -n metallb-system -l app=metallb,component=speaker --for condition=Ready --timeout 5m

for testing and got:

error: no matching resources found

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's keep the current state and if anyone ever complains about the race we switch to sleep.

@johananl johananl force-pushed the tilt-kubevirt branch 2 times, most recently from 2b628c3 to f45295f Compare March 6, 2025 15:36
@johananl
Copy link
Member Author

johananl commented Mar 7, 2025

I'd like to merge this ASAP since we depend on this PR for the Node Bootstrapping working group (https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/community/20241112-node-bootstrapping.md). We can't test Ignition functionality using CAPD so we need the CAPK Tilt workflow.

I think we can follow up with fixes or enhancements at any point later on but for now let's have something that works. Also, this is a dev tool rather than a user-facing feature so IMO it's OK if it's not 100% polished.

Copy link
Member

@chrischdi chrischdi left a comment

Choose a reason for hiding this comment

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

Last nit, happy to merge afterwards :-)

Copy link
Member

@chrischdi chrischdi left a comment

Choose a reason for hiding this comment

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

Last nit, happy to merge afterwards :-)

Signed-off-by: Johanan Liebermann <jliebermann@microsoft.com>
Signed-off-by: Johanan Liebermann <jliebermann@microsoft.com>
@johananl
Copy link
Member Author

Thanks @chrischdi. I think we should be good to go.

@sbueringer
Copy link
Member

sbueringer commented Mar 20, 2025

/assign

(I want to take a look at the delta since my last lgtm)

@chrischdi
Copy link
Member

lgtm from my side, pending stefans review :-)

@sbueringer
Copy link
Member

Thank you!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 20, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: e48a1686713852f0d48b09e29f0462214759ac18

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 20, 2025
@k8s-ci-robot k8s-ci-robot merged commit 129b395 into kubernetes-sigs:main Mar 20, 2025
19 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.11 milestone Mar 20, 2025
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. area/devtools Issues or PRs related to devtools cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants