conversion-gen: support recursive types #49747
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @nikhita. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
the
needs-ok-to-test
k8s-github-robot
added
size/S
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
|
@lavalamp I think you reviewed basically the same thing in gengo already. This time it's in conversion-gen. ptal. |
| if currentlyBuildingTypes[in] { | ||
| return true | ||
| } | ||
| currentlyBuildingTypes[in] = true |
There was a problem hiding this comment.
In Go passing map means effectively passing a pointer to map. Since noone is deleting anything from that map, you end up with map with a bunch of things at the end.
This probably isn't a big deal, but I think calling it "currentlyBuildingTypes" is a bit misleading then.
There was a problem hiding this comment.
I agree. I was a bit unsure about the name too. How about "alreadyVisitedTypes"?
There was a problem hiding this comment.
It's definitely better in my opinion.
nikhita
force-pushed
the
conversion-gen-recursive-types
branch
from
ad46f2a
to
23d0445
Compare
|
/retest |
| @@ -338,36 +338,42 @@ func (e equalMemoryTypes) Equal(a, b *types.Type) bool { | |||
| if equal, ok := e[conversionPair{b, a}]; ok { | |||
| return equal | |||
| } | |||
| result := e.equal(a, b) | |||
| result := e.equal(a, b, alreadyVisitedTypes) | |||
There was a problem hiding this comment.
Wondering, do we need the parameter inEqual at all? Can't we create the empty map here?
There was a problem hiding this comment.
We can't do that because we need the populated map in line 366, 372, 374 and 376 below. If we create an empty map here and those lines are executed, they will see the type as "not visited" - which will defeat our purpose. :)
Also, tried it out. Gives the goroutine stack exceeds 1000000000-byte limit error.
There was a problem hiding this comment.
oh, didn't realize the recursion goes through Equal and not equal.
There was a problem hiding this comment.
But, everything here in Equal can be moved to equal and then we can recursively call equal instead of Equal, can't it? Then my suggestion should work.
There was a problem hiding this comment.
Doesn't make the code easier to read though. Let's leave it as it is.
nikhita
force-pushed
the
conversion-gen-recursive-types
branch
from
23d0445
to
f98b74b
Compare
|
/retest |
| @@ -329,6 +329,13 @@ func (e equalMemoryTypes) Skip(a, b *types.Type) { | |||
| } | |||
|
|
|||
| func (e equalMemoryTypes) Equal(a, b *types.Type) bool { | |||
| // alreadyVisitedTypes holds all the types that have been converted. This is used to avoid | |||
There was a problem hiding this comment.
here it's not about "been converted", but about comparing that two structs have the same memory structure. If they have, we can use unsafe.Pointer to replace conversion by a type cast.
Better:
// alreadyVisitedTypes holds all the types that have already been checked in the structural type recursion.
Currently, the conversion-gen goes into an infinite recursion for recursive types. rename: currentlyBuildingTypes -> alreadyVisitedTypes use a cachingEqual func update comment
nikhita
force-pushed
the
conversion-gen-recursive-types
branch
from
f98b74b
to
c4656c3
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
@wojtek-t approved? |
|
/retest |
|
/approve no-issue |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nikhita, sttts, wojtek-t Associated issue: 47263 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
the
approved
|
/retest |
1 similar comment
|
/retest |
|
/test pull-kubernetes-unit |
|
/test pull-kubernetes-e2e-kops-aws |
|
/test pull-kubernetes-bazel |
|
/retest |
|
/retest Review the full test history for this PR. |
|
Submit Queue says |
|
/retest |
|
/test pull-kubernetes-e2e-kops-aws |
|
/retest |
1 similar comment
|
/retest |
|
Automatic merge from submit-queue (batch tested with PRs 49651, 49707, 49662, 47019, 49747) |
Automatic merge from submit-queue apiextensions: validation for customresources - [x] Add types for validation of CustomResources - [x] Fix conversion-gen: #49747 - [x] Fix defaulter-gen: kubernetes/gengo#61 - [x] Convert to OpenAPI types - [x] Validate CR using go-openapi - [x] Validate CRD Schema - [x] Add integration tests - [x] Fix round trip tests: #51204 - [x] Add custom fuzzer functions - [x] Add custom conversion functions - [x] Fix data race while updating CRD: #50098 - [x] Add feature gate for CustomResourceValidation - [x] Fix protobuf generation Proposal: kubernetes/community#708 Additional discussion: #49879, #50625 **Release note**: ```release-note Add validation for CustomResources via JSON Schema. ``` /cc @sttts @deads2k
Currently, conversion-gen goes into an infinite recursion for recursive types. This fixes it to support recursive types.
Needed for #47263.
Release note:
/cc @sttts