Move libdv package out of functional tests

[Ben Oukhanov]

Hi,

I've opened pull request (PR) #13333 to raise this discussion.

This tests/libdv package contains NewDataVolume function options that helps us to construct a new v1.VirtualMachine in other projects, e.g. here as an example.

Currently tests/libdv used only in functional tests and not recommended to be imported by other projects, because test code likely tend to be changed or removed (like tests/utils.go).

I'd like simply to copy libdv from tests to the pkg folder that is likely not to be changed or remvoed, similarly to the pkg/libvmi, so we can import it safely.

What would be your opinion about it? Alex Kalenyuk said in the PR that sig-storage can be the owner of the new pkg/libdv.

Thanks,

Ben

[Itamar Holder]

Hey Ben!

Personally I don't understand what are the downsides of moving this package to /pkg.

The claim that Kubevirt packages cannot be easily importable needs to be addressed, but anyhow libdv, just as libvmi, should not be aimed to be a test package. Keeping it under /tests just makes things worse, as test-dedicated code (like gomega.Expects and such) is more likely to slip in, just like happened with libvmi. I think we should aim to make this package importable, and should move it away from tests as early as possible when it's easy to do so.

p.s. Even if I leave the importability of the package aside, I don't see libvmi or libdv as test packages. Sure, most of the code that's creating VMs lives under /tests, but the package is not inherently a test package. A controller using libdv to define data volumes for VMs or a different controller using libmigration to create migrations more easily makes total sense to me.

Are there any downsides to moving this to /pkg?

--
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 visit https://groups.google.com/d/msgid/kubevirt-dev/7ceff6d5-08b6-40cc-b08f-f185855988f0n%40googlegroups.com.

[Felix Matouschek]

Hey,

Am Mittwoch, dem 27.11.2024 um 10:07 +0200 schrieb Itamar Holder:

Hey Ben!

Personally I don't understand what are the downsides of moving this package to /pkg.

The claim that Kubevirt packages cannot be easily importable needs to be addressed, but anyhow libdv, just as libvmi, should not be aimed to be a test package. Keeping it under /tests just makes things worse, as test-dedicated code (like gomega.Expects and such) is more likely to slip in, just like happened with libvmi. I think we should aim to make this package importable, and should move it away from tests as early as possible when it's easy to do so.

p.s. Even if I leave the importability of the package aside, I don't see libvmi or libdv as test packages. Sure, most of the code that's creating VMs lives under /tests, but the package is not inherently a test package. A controller using libdv to define data volumes for VMs or a different controller using libmigration to create migrations more easily makes total sense to me.

Are there any downsides to moving this to /pkg?

I would not expect any downsides for KubeVirt itself, but importing kubevirt/kubevirt into another project can be a big pain because of the high amount of indirect dependencies that are pulled in.

I'm not sure we should promote importing kubevirt/kubevirt into other projects for this reason.

[Alex Kalenyuk]

I don't see an issue with moving/owning it under sig-storage but just to echo again the concern about importing kubevirt/kubevirt
From an older thread:

I would generally discourage usage of any code from Kubevirt. We do not provide any compatibility guarantees

Normally projects would aim to avoid such imports (similarly to how you would avoid kubernetes/kubernetes) and instead
they maintain separate go packages under the main project that get published on releases:
https://github.com/kubernetes/kubernetes/tree/0e1abc4d18e3531e96ea20d03bc5cfb3d57a89d6/staging/src/k8s.io/component-helpers
https://github.com/kubernetes/component-helpers
https://pkg.go.dev/k8s.io/component-helpers

We do something similar for kubevirt.io/api but I am not sure if we can/want to replicate that for libvmi/libdv

--
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 visit https://groups.google.com/d/msgid/kubevirt-dev/d2ea08cb31c3b8c8b2182e77570bfd5b5ffa438f.camel%40redhat.com.

[Edward Haas]

I think the suggestion to promote libdv is valid.

I would try to take an existing unit-test and use it as a caller, to express the usage and internal need,

same as we did with libvmi.

Lastly, the existing import list of the package needs to be revisited.

E.g. "kubevirt.io/kubevirt/pkg/pointer" cannot remain there.

On Wed, Nov 27, 2024 at 10:36 AM Alex Kalenyuk <akal...@redhat.com> wrote:

I don't see an issue with moving/owning it under sig-storage but just to echo again the concern about importing kubevirt/kubevirt
From an older thread:

I would generally discourage usage of any code from Kubevirt. We do not provide any compatibility guarantees

Normally projects would aim to avoid such imports (similarly to how you would avoid kubernetes/kubernetes) and instead
they maintain separate go packages under the main project that get published on releases:
https://github.com/kubernetes/kubernetes/tree/0e1abc4d18e3531e96ea20d03bc5cfb3d57a89d6/staging/src/k8s.io/component-helpers
https://github.com/kubernetes/component-helpers
https://pkg.go.dev/k8s.io/component-helpers

 

I understand the concern.

But at the same time, for the same reasons a package could be used from outside the project as requested here,

the package maintainer should have an interest to keep it "clean", modular and easy to consume without a dependency hell.

Even internal k/k packages have an interest to import only "clean" packages, avoiding taking in half the project as a dependency.

We do something similar for kubevirt.io/api but I am not sure if we can/want to replicate that for libvmi/libdv

We could, but it is probably not worth it.

On Wed, Nov 27, 2024 at 10:23 AM Felix Matouschek <fmato...@redhat.com> wrote:

Hey,

Am Mittwoch, dem 27.11.2024 um 10:07 +0200 schrieb Itamar Holder:

Hey Ben!

Personally I don't understand what are the downsides of moving this package to /pkg.

The claim that Kubevirt packages cannot be easily importable needs to be addressed, but anyhow libdv, just as libvmi, should not be aimed to be a test package. Keeping it under /tests just makes things worse, as test-dedicated code (like gomega.Expects and such) is more likely to slip in, just like happened with libvmi. I think we should aim to make this package importable, and should move it away from tests as early as possible when it's easy to do so.

p.s. Even if I leave the importability of the package aside, I don't see libvmi or libdv as test packages. Sure, most of the code that's creating VMs lives under /tests, but the package is not inherently a test package. A controller using libdv to define data volumes for VMs or a different controller using libmigration to create migrations more easily makes total sense to me.

Are there any downsides to moving this to /pkg?

I would not expect any downsides for KubeVirt itself, but importing kubevirt/kubevirt into another project can be a big pain because of the high amount of indirect dependencies that are pulled in.

I'm not sure we should promote importing kubevirt/kubevirt into other projects for this reason.

You express a risk that other projects choose to take.

The package owner should make sure to preserve low dependencies on other packages, thinking about cleanliness and the ability to re-use the package.

On Tue, 26 Nov 2024 at 15:04, Ben Oukhanov <bouk...@redhat.com> wrote:

Hi,

I've opened pull request (PR) #13333 to raise this discussion.

This tests/libdv package contains NewDataVolume function options that helps us to construct a new v1.VirtualMachine in other projects, e.g. here as an example.

Currently tests/libdv used only in functional tests and not recommended to be imported by other projects, because test code likely tend to be changed or removed (like tests/utils.go).

I'd like simply to copy libdv from tests to the pkg folder that is likely not to be changed or remvoed, similarly to the pkg/libvmi, so we can import it safely.

What would be your opinion about it? Alex Kalenyuk said in the PR that sig-storage can be the owner of the new pkg/libdv.

Thanks,

Ben

--
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 visit https://groups.google.com/d/msgid/kubevirt-dev/d2ea08cb31c3b8c8b2182e77570bfd5b5ffa438f.camel%40redhat.com.

--
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 visit https://groups.google.com/d/msgid/kubevirt-dev/CAHahevfs2ydTGgLAB9t292AFe4RF2%3D1sCXLeLZNTYr54Uy2RQg%40mail.gmail.com.

[Alvaro Romero]

Regarding avoiding imports from Kubevirt, we've been discussing for some time now an upcoming epic to include a package in Kubevirt that allows retrieving all objects related to a VM (vm object graph). This package is designed for use by other projects (especially DR) as a single source of truth. The idea has been thoroughly discussed, and it seems we're moving forward with it. Given that context, importing libdv seems reasonable too and I wouldn't worry about it, unless there are concerns about the vm object graph.

I second Alex with moving/owning it under sig-storage

To view this discussion visit https://groups.google.com/d/msgid/kubevirt-dev/CAHahevfs2ydTGgLAB9t292AFe4RF2%3D1sCXLeLZNTYr54Uy2RQg%40mail.gmail.com.

[Luboslav Pivarc]

On Wed, Nov 27, 2024 at 10:35 AM Edward Haas <edw...@redhat.com> wrote:

I think the suggestion to promote libdv is valid.

I would try to take an existing unit-test and use it as a caller, to express the usage and internal need,

same as we did with libvmi.

I am not against promoting libdv, libvmi and such but I do have a concern that I have raised in the past https://github.com/kubevirt/kubevirt/pull/11005. I think this is generic enough and applies to libdv as well. As an external user of these packages I would probably be dissatisfied with the current state.

 

Lastly, the existing import list of the package needs to be revisited.

E.g. "kubevirt.io/kubevirt/pkg/pointer" cannot remain there.

On Wed, Nov 27, 2024 at 10:36 AM Alex Kalenyuk <akal...@redhat.com> wrote:

I don't see an issue with moving/owning it under sig-storage but just to echo again the concern about importing kubevirt/kubevirt
From an older thread:

I would generally discourage usage of any code from Kubevirt. We do not provide any compatibility guarantees

Normally projects would aim to avoid such imports (similarly to how you would avoid kubernetes/kubernetes) and instead
they maintain separate go packages under the main project that get published on releases:
https://github.com/kubernetes/kubernetes/tree/0e1abc4d18e3531e96ea20d03bc5cfb3d57a89d6/staging/src/k8s.io/component-helpers
https://github.com/kubernetes/component-helpers
https://pkg.go.dev/k8s.io/component-helpers

 

I understand the concern.

But at the same time, for the same reasons a package could be used from outside the project as requested here,

the package maintainer should have an interest to keep it "clean", modular and easy to consume without a dependency hell.

Even internal k/k packages have an interest to import only "clean" packages, avoiding taking in half the project as a dependency.

We do something similar for kubevirt.io/api but I am not sure if we can/want to replicate that for libvmi/libdv

We could, but it is probably not worth it.

Why not @Edward Haas ?

 

To view this discussion visit https://groups.google.com/d/msgid/kubevirt-dev/CALmkdFRttYqL7eUxCRSbELOrzpeprNb2uX-a2_VLxNgoxaZw_w%40mail.gmail.com.

[Edward Haas]

On Wed, Nov 27, 2024 at 12:35 PM Luboslav Pivarc <lpi...@redhat.com> wrote:

On Wed, Nov 27, 2024 at 10:35 AM Edward Haas <edw...@redhat.com> wrote:

I think the suggestion to promote libdv is valid.

I would try to take an existing unit-test and use it as a caller, to express the usage and internal need,

same as we did with libvmi.

I am not against promoting libdv, libvmi and such but I do have a concern that I have raised in the past https://github.com/kubevirt/kubevirt/pull/11005. I think this is generic enough and applies to libdv as well. As an external user of these packages I would probably be dissatisfied with the current state.

This is an opinionated behavior you expect from a spec builder.

I think the behavior of just sticking to building and not validating that you build the correct thing when you combine the blocks is better.

 

Lastly, the existing import list of the package needs to be revisited.

E.g. "kubevirt.io/kubevirt/pkg/pointer" cannot remain there.

On Wed, Nov 27, 2024 at 10:36 AM Alex Kalenyuk <akal...@redhat.com> wrote:

I don't see an issue with moving/owning it under sig-storage but just to echo again the concern about importing kubevirt/kubevirt
From an older thread:

I would generally discourage usage of any code from Kubevirt. We do not provide any compatibility guarantees

Normally projects would aim to avoid such imports (similarly to how you would avoid kubernetes/kubernetes) and instead
they maintain separate go packages under the main project that get published on releases:
https://github.com/kubernetes/kubernetes/tree/0e1abc4d18e3531e96ea20d03bc5cfb3d57a89d6/staging/src/k8s.io/component-helpers
https://github.com/kubernetes/component-helpers
https://pkg.go.dev/k8s.io/component-helpers

 

I understand the concern.

But at the same time, for the same reasons a package could be used from outside the project as requested here,

the package maintainer should have an interest to keep it "clean", modular and easy to consume without a dependency hell.

Even internal k/k packages have an interest to import only "clean" packages, avoiding taking in half the project as a dependency.

We do something similar for kubevirt.io/api but I am not sure if we can/want to replicate that for libvmi/libdv

We could, but it is probably not worth it.

Why not @Edward Haas ?

Because we will need to maintain another repo and at the same time keep the same rules.

The alternative of making sure the rules are kept (could be through tooling) without the need to add

another repo to the party seems more appealing to me.

If a strong reasoning is presented to be worth a repo, then we can still do it.

I suggest we do the first part and then consider going one step further.

[Federico Fossemo]

Hey everyone,

I think that libvmi (and libdv as well) in the future should be moved to a separate repo so that it can be consumed

externally. I don't think we should do it now just for convenience; even if it is quite complete we are

still adding/modifing things there; and changing libvmi in kv/kv is easier than adjust kv/libvmi and update kv/kv dependency.
Once we agree that it is stable and complete enough we should move it outside of kv/kv.

Different discussion for the `pointer` pkg. To me, it is still complete and should be moved outside of kv/kv repo.

Thanks,
Federico

To view this discussion visit https://groups.google.com/d/msgid/kubevirt-dev/CALmkdFT_xChPxdQyx0cRZAB3JNV4zFduF32svM_x-A2h1bSLww%40mail.gmail.com.

[Edward Haas]

Although I am not yet convinced we need a separate repo at this time, the cost of it for kv/kv is zero if implemented like the "staging" folder.

I.e. a job just reflects the existing "pkg/foo" to a new repo automatically.

Different discussion for the `pointer` pkg. To me, it is still complete and should be moved outside of kv/kv repo.

It is a convenient tooling package, each project can duplicate it if they want.

It is after-all a 3 line thing with no special logic which merits sharing between projects.

[Nahshon Unna Tsameret]

Hi.

I also think that we can benefit from kubevirt/kubevirt code in other kubevirt repos, but import kubevirt creates a dependency hell. I would like to add also the great "pkg/apimachinery/patch" (a json patch package) package that I would love to adopt into my repo, but can't right now.

[Daniel Hiller]

Hey all,

Let me chime in here with regard to cleanliness of a package: what comes to my mind is an objective metric that expresses "stability" by measuring couplings between packages (outside in vs. inside out, aka afferent vs. efferent coupling) [1]

When skimming over our quality gate I didn't find something to that regard [2], however there's a go tool called effrit [3] that calculates some metrics guiding decisions about cleanliness of package design.

LMK whether we should look into establishing a lane that does the measuring to support further improvements in the described direction.

[1]: https://en.wikipedia.org/wiki/Software_package_metrics  

[2]: https://sonarcloud.io/component_measures?id=kubevirt_kubevirt

[3]: https://github.com/Skarlso/effrit

 

To view this discussion visit https://groups.google.com/d/msgid/kubevirt-dev/b9e03b81-da5e-48a6-ab0e-67f1e9ab477fn%40googlegroups.com.

--

-- 

Best,

Daniel

[Itamar Holder]

We can, and should, discuss how we make packages externally consumable.

While this discussion here is important, I think it diverges a bit from what Ben's PR actually does, which is only to move libdv away from /tests.

Even if we discourage importing kubevirt packages, libdv inherently is not a test package and should not bring test-related dependencies. Keeping it under /tests has no advantages that I can think of, and has the disadvantage of increasing the probability of slipping in test packages as dependencies, making the package inconsumable from production code. It is true that, as a bonus, it's a step towards making it consumable externally as well.

Unless someone can come up with a compelling argument for keeping it under tests, I will be gladly willing to approve the PR.

To view this discussion visit https://groups.google.com/d/msgid/kubevirt-dev/CAK%2BeyL5nWoCPSYGXq%3DdXn_dqovomTWns%3DLebq4DNCJnjhY3%3D%2Bg%40mail.gmail.com.

[Felix Matouschek]

On Friday, November 29, 2024 at 2:35:33 PM UTC+1 Itamar Holder wrote:

We can, and should, discuss how we make packages externally consumable.

Do you think we can discuss it as part of the code quality WG?

Daniel's suggestion about adding metrics for this looks interesting to me as well.

While this discussion here is important, I think it diverges a bit from what Ben's PR actually does, which is only to move libdv away from /tests.

For me, Ben's PR hits a pain point, so I don't think this discussion deviates too much.

What was the initial reason for not having this package in production code?

Even if we discourage importing kubevirt packages, libdv inherently is not a test package and should not bring test-related dependencies. Keeping it under /tests has no advantages that I can think of, and has the disadvantage of increasing the probability of slipping in test packages as dependencies, making the package inconsumable from production code. It is true that, as a bonus, it's a step towards making it consumable externally as well.

Is there production code (potentially other that unit tests), that could make use of it today?

[Itamar Holder]

On Fri, 29 Nov 2024 at 18:16, Felix Matouschek <fmato...@redhat.com> wrote:

On Friday, November 29, 2024 at 2:35:33 PM UTC+1 Itamar Holder wrote:

We can, and should, discuss how we make packages externally consumable.

Do you think we can discuss it as part of the code quality WG?

Daniel's suggestion about adding metrics for this looks interesting to me as well.

Yes, sounds good to me!

While this discussion here is important, I think it diverges a bit from what Ben's PR actually does, which is only to move libdv away from /tests.

For me, Ben's PR hits a pain point, so I don't think this discussion deviates too much.

What was the initial reason for not having this package in production code?

I agree that the discussion is important, but don't think it should block Ben's PR.

I think that it was initiated in the test code since libvmi was initially there (but was moved later to production). IOW I don't think there's any good reason for it.

 

Even if we discourage importing kubevirt packages, libdv inherently is not a test package and should not bring test-related dependencies. Keeping it under /tests has no advantages that I can think of, and has the disadvantage of increasing the probability of slipping in test packages as dependencies, making the package inconsumable from production code. It is true that, as a bonus, it's a step towards making it consumable externally as well.

Is there production code (potentially other that unit tests), that could make use of it today?

One example I can think about from the top of my head is virtctl create vm: https://github.com/kubevirt/kubevirt/blob/87475917dfaa4ce325c4cff88075ecca16ea23ff/pkg/virtctl/create/vm/vm.go#L1290.

However even if I couldn't think of such a usage I'd still think we need to move this package to production since the usage might arise in the future, in which it could be much more difficult to move it (since test specific code would slip in).

 

To view this discussion visit https://groups.google.com/d/msgid/kubevirt-dev/e44c4410-f1e6-4e02-ac6c-dc23aa462d77n%40googlegroups.com.