Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove hack/tools/third_party/conversion-gen fork #5153

Open
nrb opened this issue Oct 14, 2024 · 2 comments
Open

Remove hack/tools/third_party/conversion-gen fork #5153

nrb opened this issue Oct 14, 2024 · 2 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-priority triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@nrb
Copy link
Contributor

nrb commented Oct 14, 2024

/kind bug

What steps did you take and what happened:
[A clear and concise description of what the bug is.]

Currently, we have a forked version of the conversion_gen tool, added in commit 4274a5a

The comment states Temporarily disable duplicate static conversion detection.

When updating to CAPI 1.8 in #5061, Kubernetes 1.30 introduced new changes to the conversion_gen tool, as all of them had been refactored. See this post on the mailing list: https://groups.google.com/a/kubernetes.io/g/dev/c/Ix-ACY9DhEs/m/vEoBkBs9AAAJ

I have removed the forked copy and configured the Makefile to use the upstream conversion_gen tool, however, when I ran the make .build/generate-go-apis command, I received the following error.

(previous output elided)

panic: duplicate static conversion defined: sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2.Instance -> sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta1.Instance from:
sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta1.Convert_v1beta2_Instance_To_v1beta1_Instance
sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta1.Convert_v1beta2_Instance_To_v1beta1_Instance

goroutine 1 [running]:
k8s.io/code-generator/cmd/conversion-gen/generators.getManualConversionFunctions(0x140647e7680, 0x14062af0510, 0x140649e7a10)
        /Users/nbrubake/go/1.22.7/pkg/mod/k8s.io/code-generator@v0.30.2/cmd/conversion-gen/generators/conversion.go:184 +0x1038
k8s.io/code-generator/cmd/conversion-gen/generators.GetTargets(0x140647e7680, 0x140000cf500)
        /Users/nbrubake/go/1.22.7/pkg/mod/k8s.io/code-generator@v0.30.2/cmd/conversion-gen/generators/conversion.go:309 +0xf10
main.main.func1(0x140000a4b80?)
        /Users/nbrubake/go/1.22.7/pkg/mod/k8s.io/code-generator@v0.30.2/cmd/conversion-gen/main.go:123 +0x20
k8s.io/gengo/v2.Execute(0x140001e3ef0, {0x104f21238, 0x6}, 0x1406450ff20, {0x104f4c81d?, 0x14?}, {0x14000057770, 0x1, 0x5})
        /Users/nbrubake/go/1.22.7/pkg/mod/k8s.io/gengo/v2@v2.0.0-20240228010128-51d4e06bde70/execute.go:92 +0x2a0
main.main()
        /Users/nbrubake/go/1.22.7/pkg/mod/k8s.io/code-generator@v0.30.2/cmd/conversion-gen/main.go:127 +0x3d8
make: *** [.build/generate-go-apis] Error 2

What did you expect to happen:
Moving back to the upstream version of conversion-gen should have worked; however, they still have the offending error at https://github.com/kubernetes/code-generator/blob/release-1.30/cmd/conversion-gen/generators/conversion.go#L184.

Anything else you would like to add:
[Miscellaneous information that will assist in solving the issue.]
I think this is happening because we have the exp directory, which is the experimental APIs.

I'll note that the core CAPI project has an exp/api directory (https://github.com/kubernetes-sigs/cluster-api/tree/release-1.8/exp/api/v1beta1), but it is not breaking. I have not done a thorough reading of the code, but I suspect they're unaffected because they do not have a type defined in both, whereas this project has v1beta2.Instance in both our exp/api and api directories.


Also, it wasn't immediately obvious to me why the collision was happening in our code, since the Instance type is in two different packages.

The problem is that the conversion file goes into the v1beta1 package, and anything other than the API version and type name are omitted in the function. Thus, v1beta1.Instance can only be converted to one v1beta2.Instance.

See the signature found:

❯ rg Convert_v1beta2_Instance_To_v1beta1_Instance
api/v1beta1/zz_generated.conversion.go
569:            return Convert_v1beta2_Instance_To_v1beta1_Instance(a.(*v1beta2.Instance), b.(*Instance), scope)
1062:           if err := Convert_v1beta2_Instance_To_v1beta1_Instance(*in, *out, s); err != nil {
2010:func autoConvert_v1beta2_Instance_To_v1beta1_Instance(in *v1beta2.Instance, out *Instance, s conversion.Scope) error {

api/v1beta1/conversion.go
50:func Convert_v1beta2_Instance_To_v1beta1_Instance(in *v1beta2.Instance, out *Instance, s conversion.Scope) error {
51:     return autoConvert_v1beta2_Instance_To_v1beta1_Instance(in, out, s)

exp/api/v1beta1/conversion.go
184:// Convert_v1beta2_Instance_To_v1beta1_Instance is a conversion function.
185:func Convert_v1beta2_Instance_To_v1beta1_Instance(in *infrav1.Instance, out *infrav1beta1.Instance, s apiconversion.Scope) error {
186:    return infrav1beta1.Convert_v1beta2_Instance_To_v1beta1_Instance(in, out, s)
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 14, 2024
@nrb
Copy link
Contributor Author

nrb commented Oct 14, 2024

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 14, 2024
@nrb
Copy link
Contributor Author

nrb commented Oct 14, 2024

I've captured the Makefile changes that I've made here: nrb@25d57a8

Copying the commit description just in case it gets overridden:

NOTE: This commit still needs work. Generated files were not included.

Specifically, many of the CRD will need fixing because the updated version
of conversion_gen registers some of code comments as being something to
put into the YAML; the comments will need to be re-arranged in the Go files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-priority triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

2 participants