Code quality of virtctl and potential cleanups

[Felix Matouschek]

Hi all,

to follow the theme of improving the code quality in KubeVirt I wanted
to take upon the task of cleaning up and improving the code base of
virtctl.

For that I had a thorough look at virtctl and tried to identify
potential issues. These are the major pain points I found so far.

1. Cutting down on unnecessary e2e tests

There are many virtctl e2e tests, which are essential useless because
they retest already tested API calls. Ideally the API calls made by
virtctl should be tested tightly through appropriate unit tests instead
of through e2e tests that duplicate the test cases of the used API
endpoints.

A positive side-effect of this would be the overall reduction of e2e
tests and the reduction of failure rates through flakes in our CI.

For a number of e2e tests involving virtctl see this:

$ git grep NewRepeatableVirtctlCommand tests|wc -l
147

2. There are many smells and potential code cleanups in the virtctl
code base (test and productive code). 

For a non-exhaustive list, see things like:

- Getting rid of pkg/virtctl/utils (we like utils, don't we?)
- Use libvmi where possible
- Code duplication, unnecessary long functions and unnecessary
helpers
- Use of globals, use of init() funcs and the naming and order of
imports
- Functions spread across multiple pkgs which logically belong
together (Bring code closer to where it is used!)

3. Reorganization and cleanup of virtctl's commands

Lastly, we could re-organize some of virtctl's commands into sub
commands. There are commands which logically belong together or could
be unified in a single command. (e.g. migrate and migrate-cancel or
restart and soft-reboot).

This would however break the "API" / CLI for end users and would need
carefully planning.

What are your opinions on this? I would be happy for any feedback and
also to hear about any additional pain points you discovered in
virtctl.

Have a great weekend,
Felix

[Orel Misan]

Hi Felix,

Thank you for this initiative Felix.

Another thing that could be done is removing the dependency on "kubevirt.io/kubevirt/tests" from virtctl's tests located under "pkg/virtctl".

WDYT?

Orel

--
You received this message because you are subscribed to the Google Groups "kubevirt-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubevirt-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/effdcb261610639dae14e7c02bace97f7295eb9f.camel%40redhat.com.

[Felix Matouschek]

Hi Orel,

Am Dienstag, dem 23.07.2024 um 09:34 +0300 schrieb Orel Misan:

Another thing that could be done is removing the dependency on "kubevirt.io/kubevirt/tests" from virtctl's tests located under "pkg/virtctl".

WDYT?

that's a great idea! I'll check the code base again for this and make sure to add it to my plan.

Thanks,

Felix

[Dan Kenigsberg]

Thanks, Felix, for taking this initiative.

Bringing modularity and code ownership is very welcome.

So is reducing the runtime and fragility of our tests.

Changing virtctl's command line semantics is less urgent in my opinion, and should be done gradually (if at all) and with proper deprecation notices.

As for the specific clean-ups, please make sure to motivate each of them properly.

For example, using globals is a bad practice, because they let different packages affect each other in intractable manner. But init(), when used properly, can reduce package inter-dependency. A casual `git grep 'func init' pkg/virtctl` did not immediately reveal obviously problematic issues to me.

Regards,

Dan.

On Fri, Jul 19, 2024 at 3:29 PM Felix Matouschek <fmato...@redhat.com> wrote: