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:

[Felix Matouschek]

Thanks for your feedback.

You are right about init() calls, but IMO they can be a smell.

The instances where init() is used in the virtctl code base are easily replaced by package variables

that can be assigned to during unit tests. It avoids setters which in my opinion do not provide any

benefits in these cases and which need to be called in init() functions.

I'll make sure to motivate each cleanup properly, in general I'm going to follow our coding conventions [1].

My plan for virtctl now roughly looks like this:

1. Clean up the virtctl codebase and eliminate code smells with a look at our coding conventions.

2. Ensure each virtctl command is properly tested by unit tests.

    Remove all e2e tests testing virtctl directly or using virtctl to test API endpoints.

    We only want to keep e2e tests for complex functionality like portforwarding or usbredir.

3. Avoid dependency on `tests` in `pkg/virtctl`, in other words move / replace functionality in `tests/clientcmd/virtctl.go`.

I will defer changing the command line semantics for now. We can always do that later if needed.

Any further feedback or opinions are appreciated.

Thanks,

Felix

[1] https://github.com/kubevirt/kubevirt/blob/main/docs/coding-conventions.md

[Edward Haas]

On Mon, Jul 29, 2024 at 12:01 PM Felix Matouschek <fmato...@redhat.com> wrote:

Thanks for your feedback.

You are right about init() calls, but IMO they can be a smell.

The instances where init() is used in the virtctl code base are easily replaced by package variables

that can be assigned to during unit tests. It avoids setters which in my opinion do not provide any

benefits in these cases and which need to be called in init() functions.

Exposing package variables in this way is a type of global.

Tests should not be part of the production package in order to maintain proper API and assure they

do not break when internal changes occur in the production code.

init() is just a way to initialize things  without the need for a central "manager"

to reference them. It is a common practice used to do actions as a pre main() step.

Clean code and architecture practices encourages the flow to be from main() out and for the dependencies

to exist in the opposite direction (or none through interfaces). Sometimes, the reverse dependency can be accomplished with init().

The classic example is the `flag` package.

But it is indeed a per case examination. There is no one right thing to do as a general rule.

--
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/57b918cde862b30ac962e81d855920aa857ccfe4.camel%40redhat.com.

[Andrej Krejcir]

Hi,

The code inspection in GoLand of the "pkg/virtctl" package found 70 unhandled errors. Most of them look safe to ignore. 

Can you also handle these errors, or explicitly ignore them?

Andrej

--
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/57b918cde862b30ac962e81d855920aa857ccfe4.camel%40redhat.com.