Open Kuruyia opened 7 months ago
[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.
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.
@Kuruyia thanks for the work! Can you implement a couple of unit tests for the functions?
@dhiller @brianmcarey do you have any idea how we could test this?
@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/kvm
device, ...
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:
vmcli.sh
shell scriptAs 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.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.sh
shell 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
.
@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.
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 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.
/lgtm
/test ?
@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-tooling
check-provision-k8s-1.28
check-provision-k8s-1.29
check-provision-k8s-1.30
check-provision-manager
check-up-kind-1.27-vgpu
check-up-kind-sriov
/test check-gocli /test check-provision-centos-base
New changes are detected. LGTM label has been removed.
/retest
@Kuruyia: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test
message.
/test check-gocli /test check-provision-centos-base
/ok-to-test
/retest
@Kuruyia: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test
message.
/test check-gocli /test check-provision-centos-base
/ok-to-test
I fixed the name of the base golang image, and tested cluster provisioning under Fedora 40 with Podman. It built successfully, so hopefully this time's the charm for the CI as well :)
/retest
@Kuruyia: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test
message.
/retest
hi @Kuruyia thanks for the persistence. I'm in favor of this change. Kind reminder to @xpivarc @brianmcarey
@Kuruyia: The following tests failed, say /retest
to rerun all failed tests or /retest-required
to rerun all mandatory failed tests:
Test name | Commit | Details | Required | Rerun command |
---|---|---|---|---|
check-provision-centos-base | 55c5735a9ecefbe2dffa3c4c8052c44a71b50221 | link | true | /test check-provision-centos-base |
check-up-kind-1.30-vgpu | 0d8d7fa9fa4297105db441c6067fe174840ed6c2 | link | false | /test check-up-kind-1.30-vgpu |
check-up-kind-1.27-vgpu | 0d8d7fa9fa4297105db441c6067fe174840ed6c2 | link | false | /test check-up-kind-1.27-vgpu |
hi @Kuruyia thanks for the persistence. I'm in favor of this change. Kind reminder to @xpivarc @brianmcarey
No problem.
It looks like the check-up-kind-1.*-vgpu
tests failed, but looking at those test histories, they just tend to do that?
I see that the required check-provision-*
tests also didn't run on the last commit (0d8d7fa
). Could you just rerun those?
Thanks for your contribution.
It looks like the
check-up-kind-1.*-vgpu
tests failed, but looking at those test histories, they just tend to do that? I see that the requiredcheck-provision-*
tests also didn't run on the last commit (0d8d7fa
). Could you just rerun those?
Centos9 folder which is the only changed folder doesnt affect kind-X providers. Jobs are running, here for example https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_kubevirtci/1164/check-provision-k8s-1.28/1824330643983568896 Prow / Tide takes care about it.
Please consider that check-provision uses podman, but the post submit itself uses docker, worth to check it as well, maybe for example by creating a draft PR that use docker to run presubmits and run rehearse there.
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 #1160Special 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.
Release note: