kubevirtci
kubevirtci copied to clipboard
kubevirt
Convert the vm.sh script into a go binary
What this PR does / why we need it: This adds a new Go CLI based on Cobra that will replace the vm.sh script used when provisioning a cluster to set up the VMs that will contain the k8s cluster nodes. This is done in order to increase the maintainability of this script in the future.
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 #1160
Special notes for your reviewer: The CLI flags were kept as-is so this can be used as a drop-in replacement.
I've already started to split the functions into multiple files so it is (hopefully) already a bit cleaner than the current shell script.
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR. Approvers are expected to review this list.
- [ ] Design: A design document was considered and is present (link) or not required
- [ ] PR: The PR description is expressive enough and will help future contributors
- [ ] Code: Write code that humans can understand and Keep it simple
- [ ] Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
- [ ] Upgrade: Impact of this change on upgrade flows was considered and addressed if required
- [ ] Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
- [ ] Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
- [ ] Community: Announcement to kubevirt-dev was considered
Release note:
NONE
[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 awels for approval. For more information see the Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
Hi @Kuruyia. Thanks for your PR.
PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.
I understand the commands that are listed here.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
@Kuruyia thanks for the work! Can you implement a couple of unit tests for the functions?
@Kuruyia thanks for the work! Can you implement a couple of unit tests for the functions?
Hey, thanks for your review!
I'll address your change requests and add the unit tests this week. I guess we're only testing the functions that don't have any side effects for now?
It'll be more involved to test functions that execute qemu-img, try to create a /dev/kvm device, ...
@Kuruyia thanks for the work! Can you implement a couple of unit tests for the functions?
Hey, thanks for your review!
I'll address your change requests and add the unit tests this week. I guess we're only testing the functions that don't have any side effects for now?
It'll be more involved to test functions that execute
qemu-img, try to create a/dev/kvmdevice, ...
Since we didn't have any tests for the bash script, I would just add some simple ones. For example, some that check the qemu command line. The rest is a bit difficult because this run multiple binaries, for example, the part that creates the the iptables, I would skip it for now.
Another place where you can add unit tests is CalcNextDisk. In this case, you could for example declare type and a function type and overwrite it in the unit tests for files, err := os.ReadDir(searchDir). Something like
type readDirFunc func(n int) ([]os.DirEntry, error)
type DiskUtil struct {
readDirFunc readDir
}
func NewDiskUtil() *DiskUtil {
return &DiskUtil{
readDir = os.ReadDir
}
}
and in the tests something like this:
type mockDirEntry struct {
name string
}
func newmockDirEntry(name string) os.DirEntry {
return &mockDirEntry{name:name}
}
func (m *mockDirEntry) Name() string {
return m.name
}
[...] // implement the rest of the function for DirEntry
func mockReadDir(n int) ([]os.DirEntry, error) {
return os.DirEntry{
newmockDirEntry("disk0.qcow2"),
newmockDirEntry("disk1.qcow2"),
}, nil
}
diskUtil := &DiskUtil{
readDir = mockReadDir
}
// test
diskUtil.CalcNextDisk(...)
@dhiller @brianmcarey do you have any idea how we could test this?
@Kuruyia thank you for your work here!
@alicefr I agree with what you suggested. Since we just didn't (or rather couldn't) unit-test anything here, let's try to increase coverage of testing where we can - anything is better than nothing, as it is currently.
/cc
FWIW https://github.com/kubevirt/kubevirtci/pull/1174 is about to merge and will need to be covered here. I wasn't aware of this PR prior to starting this work sorry otherwise I would've extended it myself.
/cc
FWIW #1174 is about to merge and will need to be covered here. I wasn't aware of this PR prior to starting this work sorry otherwise I would've extended it myself.
No problem, I was working on the unit tests today so I'll work on adding your modifications as well afterwards, thanks for letting me know.
Hey! Sorry for the delay, I just added the unit tests as you asked, and ported here the NUMA feature that was recently added to vmcli.sh as well.
Here's what I think is left to do:
- run those unit tests in the CI
- integrate this new CLI in place of the
vmcli.shshell script - support RHEL and RHEL-based distributions (when starting QEMU)
As this PR is getting bigger, it might be better to offload those tasks to separate PRs imho.
Hey! Sorry for the delay, I just added the unit tests as you asked, and ported here the NUMA feature that was recently added to
vmcli.shas well.Here's what I think is left to do:
- run those unit tests in the CI
- integrate this new CLI in place of the
vmcli.shshell script- support RHEL and RHEL-based distributions (when starting QEMU)
As this PR is getting bigger, it might be better to offload those tasks to separate PRs imho.
Yes, I think we could replace the bash script in a second PR. For example, we might need to think also how to transition form it to the golang binary. We might want to have a version with both to be able to switch to the old version if there are any problem. However, one thing that is necessary are the build of the binary and to copy it inside the provisioner image.
Yes, I think we could replace the bash script in a second PR. For example, we might need to think also how to transition form it to the golang binary. We might want to have a version with both to be able to switch to the old version if there are any problem. However, one thing that is necessary are the build of the binary and to copy it inside the provisioner image.
Yup no problem, what do you think about adding a new stage at the beginning of the centos9 Dockerfile to build this binary, and then copy it in the current (single) stage?
Yes, I think we could replace the bash script in a second PR. For example, we might need to think also how to transition form it to the golang binary. We might want to have a version with both to be able to switch to the old version if there are any problem. However, one thing that is necessary are the build of the binary and to copy it inside the provisioner image.
Yup no problem, what do you think about adding a new stage at the beginning of the centos9 Dockerfile to build this binary, and then copy it in the current (single) stage?
Yes, sound as a good idea!
@alicefr I changed the tests to use Ginkgo instead, it is much cleaner now :)
I also modified the Dockerfile to split it into two stages. The first stage is in charge of building this new Go cli, and the second stage (which was the old Dockerfile) copies the binary from that first stage into /vmcli.
This is looking great, some quick comments but this should be ready to come out of draft and possibly land shortly.
Can you provide a Makefile that we could then call from a presubmit as we do for gocli?
Do you want a Makefile that replaces the build.sh script?
@Kuruyia please squash the commits that changes the vmcli into a single one, for example, the ones that inline the errors.
@Kuruyia I think now what we need it is a plan to integrate this into main. /cc @dhiller @brianmcarey I will also bring this topic to the next community meeting
@alicefr: GitHub didn't allow me to request PR reviews from the following users: also, community, will, bring, this, to, the, I, meeting.
Note that only kubevirt members and repo collaborators can review this PR, and authors cannot review their own PRs.
In response to this:
@Kuruyia I think now what we need it is a plan to integrate this into main. /cc @dhiller @brianmcarey I will also bring this topic to the next community meeting
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
This is looking great, some quick comments but this should be ready to come out of draft and possibly land shortly. Can you provide a Makefile that we could then call from a presubmit as we do for gocli?
Do you want a Makefile that replaces the
build.shscript?
@Kuruyia I don't quite understand why you need the build.sh. AFAIU, you added an additional building step and it should be automatically executed when we build the centos stream 9 node.
@lyarwood there is a difference here between gocli and vmcli. We build gocli on the host and then copy the binary, while here vmcli is built into the container. IMO, the second approach is better and we should also do the same for gocli. It is rare, but building gocli depends on the go version installed on the host. It could happen that the go version is too old. Building the binary in the container guarantees that we don't have any golang version mismatch.
@Kuruyia please squash the commits that changes the vmcli into a single one, for example, the ones that inline the errors.
I squashed all the commits for the vmcli into a single one, and have another commit for the modifications done on the Dockerfile. Please tell me if you want a more granular squash.
@Kuruyia I don't quite understand why you need the build.sh. AFAIU, you added an additional building step and it should be automatically executed when we build the centos stream 9 node.
I don't really understand what targets are needed inside this Makefile. I assumed it would replace the build.sh script as the example @lyarwood provided shows the gocli Makefile used for building the container image.
@brianmcarey: The following commands are available to trigger required jobs:
/test check-provision-alpine-with-test-tooling/test check-provision-centos-base/test check-provision-k8s-1.28/test check-provision-k8s-1.29/test check-provision-k8s-1.30/test check-provision-manager/test check-up-kind-ovn/test check-up-kind-sriov
The following commands are available to trigger optional jobs:
/test check-gocli/test check-up-kind-1.27-vgpu
Use /test all to run the following jobs that were automatically triggered:
check-provision-alpine-with-test-toolingcheck-provision-k8s-1.28check-provision-k8s-1.29check-provision-k8s-1.30check-provision-managercheck-up-kind-1.27-vgpucheck-up-kind-sriov
In response to this:
/test ?
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.
@Kuruyia: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.
In response to this:
/retest
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.