Change directory tree to the new kubebilder template

[llamerada-jp]

a part of: #760
ref: https://book.kubebuilder.io/migration/migration_guide_gov3_to_gov4

[jakobmoellerdev]

Hi guys, even though I generally approve of standardization changes, this would make it impossible for us to consume the controllers of topolvm via go import as you move them to the internal package. This is a severe issue for us as we would need to constantly patch a huge mv from files everytime we do a rebase. The reason we do this is because we dont bundle the cmds as they do not contain enough flexibility for us to integrate them into lvm-operator in openshift.

Is it possible to challenge moving everything to internal and keeping controllers in pkg? If not could we at least wait until we have a discussion on how to solve our downstream so that we can ensure continuity?

cc @suleymanakbas91

[llamerada-jp]

@jakobmoellerdev
We can move the necessary directories under the pkg or top dir. Is the lvm-operator all of the controller dir required? Or specific constants or interface definitions?

[jakobmoellerdev]

Thanks, we appreciate you modifying the current approach:

These are all the usages in lvm-operator of any topolvm package as of now:

• github.com/topolvm/topolvm/lvmd (used for config parsing, elements such as DeviceClass or ThinPool)
• github.com/topolvm/topolvm/pkg/lvmd/cmd (used for config parsing)
• github.com/topolvm/topolvm/api/v1 (used for CR interaction via k8s client)
• github.com/topolvm/topolvm/controllers (used to start the TopoLVM controllers embedded with custom settings for things like leader election), afaik only for NewLogicalVolumeReconcilerWithServices, NewNodeReconciler, NewPersistentVolumeClaimReconciler
• github.com/topolvm/topolvm/driver (to start the CSI gRPC identity and controller server)
• github.com/topolvm/topolvm/client (to configure the controllers of github.com/topolvm/topolvm/controllers canonically)
• github.com/topolvm/topolvm/lvmd/command (to set the Containerized flag)
• github.com/topolvm/topolvm/runners (to configure the metrics exporter embedded)

[llamerada-jp]

@jakobmoellerdev

In the last commit of this PR, the functions used from lvm-operator are put in the pkg dir.

1. Is this implementation way ok?
2. I have put all the functions used in lvm-operator in the pkg or top dir. I would like you to check if anything is missing.
3. The client package should not be needed if lvm-operator does not support the legacy CR (logicalvolumes.topolvm.cybozu.com). Is it necessary?

[jakobmoellerdev]

Ill pull your branch and do a temporary recompile to see if we have any incompatibilities. Please give me some time to confirm, will report ASAP.

[jakobmoellerdev]

Okay so maybe my Feedback here:

I think this will work (at least my compile didnt go crazy, but I may have to open up follow up issues if there is in the future any type that would need to be exported (something we can do though and not blocking this PR)

However, I think the passing of internal to pkg (via aliasing of a pkg type into the internal pacakge) to expose the types is quite convoluted and makes the code harder to maintain. I'm wondering why it is necessary for us to adopt "internal" over pkg, since we have no sensitive code that is not "allowed" to be imported. Why is it a must have for us to adopt this suggested build pattern from kubebuilder v4?

[llamerada-jp]

@jakobmoellerdev

Thanks for the confirmation. We think about the directory structure as follows.

When adding a new controller or modules, kubeuilder generates a skeleton in the specific directory. So we were going to do this code migration to ease future development.

We'll keep pkg dir for now. However, we don't expect the code under this directory to be used for other projects and want to do this migration in the future. Could you modify lvm-operator to remove the dependency on this directory and let us know if it's done?

[jakobmoellerdev]

When adding a new controller or modules, kubeuilder generates a skeleton in the specific directory. So we were going to do this code migration to ease future development.

I understand the incentive but the only alternative for us is to use the commands as is which prohibits us from starting the controllers within the same controller manager. Thus we need direct access to the Controller structs

We'll keep pkg dir for now. However, we don't expect the code under this directory to be used for other projects and want to do this migration in the future. Could you modify lvm-operator to remove the dependency on this directory and let us know if it's done?

I'm not certain how this would ever work out for us (and we cannot migrate unless you change your package structures anyway in main, since we depend on the latest topolvm code). We just recently switched to a model where we incorporated all the controllers directly because of resource constraints: Starting the controllers like in https://github.com/openshift/lvm-operator/blob/main/cmd/vgmanager/vgmanager.go#L153-L196 is currently only possible via go import.

If there is a way to do this without actually importing the code from these packages, Im open to this of course. We could for example change the way that the topolvm cmd package is started because this is non-standard kubebuilder. If we find a way to incorporate the manager into this then I believe this can solve our issue.

[jakobmoellerdev]

Maybe let me summarize: I am not opposed to this change (its always good to stick to upstream), but I also dont think that moving the controllers into internal is really a must-enforce. Kubebuilder does a suggested layout like this because most controllers actually nowadays are downstream controllers that do not need to have controllers exposed.

[llamerada-jp]

@jakobmoellerdev
Thank you for your comment.
TopoLVM's modules were not designed to be referenced from the outside. By limiting the interface to pkg, I expect to notice when the interface definition is changed. Because so, for this PR, I want to proceed with putting interfaces in the pkg dir. If I come up with a better way or if we need to change the interface in the future, we can make another PR.

[cupnes]

Please check the following. Other than that, I don't think there are any problems.

[satoru-takeuchi]

@llamerada-jp Could you add this link to the PR description and the description in the following commit?

Change directory tree to the new kubebilder template

In addition, how about adding this link to the following commits?

make interfaces to be used externally

I believe that both links will help us very much to understand why we made these changes.