conversion-gen: support recursive types #49747

nikhita · 2017-07-27T20:54:26Z

Currently, conversion-gen goes into an infinite recursion for recursive types. This fixes it to support recursive types.

Needed for #47263.

Release note:

NONE

/cc @sttts

k8s-ci-robot · 2017-07-27T20:54:33Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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.

sttts · 2017-07-27T21:23:10Z

/ok-to-test

sttts · 2017-07-27T21:24:33Z

@lavalamp I think you reviewed basically the same thing in gengo already. This time it's in conversion-gen. ptal.

wojtek-t · 2017-07-28T12:13:02Z

staging/src/k8s.io/kube-gen/cmd/conversion-gen/generators/conversion.go

+		if currentlyBuildingTypes[in] {
+			return true
+		}
+		currentlyBuildingTypes[in] = true


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.

I agree. I was a bit unsure about the name too. How about "alreadyVisitedTypes"?

It's definitely better in my opinion.

nikhita · 2017-07-28T14:49:21Z

/retest

sttts · 2017-07-28T17:31:10Z

staging/src/k8s.io/kube-gen/cmd/conversion-gen/generators/conversion.go

@@ -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)


Wondering, do we need the parameter inEqual at all? Can't we create the empty map here?

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.

oh, didn't realize the recursion goes through Equal and not equal.

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.

Doesn't make the code easier to read though. Let's leave it as it is.

nikhita · 2017-07-28T19:02:50Z

/retest

sttts · 2017-07-28T19:07:26Z

staging/src/k8s.io/kube-gen/cmd/conversion-gen/generators/conversion.go

@@ -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


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

sttts · 2017-07-28T19:15:16Z

/lgtm

sttts · 2017-07-28T19:15:25Z

@wojtek-t approved?

nikhita · 2017-07-28T19:34:11Z

/retest

wojtek-t · 2017-07-31T08:09:01Z

/approve no-issue

k8s-github-robot · 2017-07-31T08:09:45Z

[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:

~~staging/src/k8s.io/kube-gen/OWNERS~~ [wojtek-t]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

nikhita · 2017-07-31T09:02:38Z

/retest

nikhita · 2017-07-31T09:08:48Z

/retest

nikhita · 2017-07-31T09:36:16Z

/test pull-kubernetes-unit

nikhita · 2017-07-31T09:40:28Z

/test pull-kubernetes-e2e-kops-aws

nikhita · 2017-07-31T09:55:05Z

/test pull-kubernetes-bazel

nikhita · 2017-07-31T10:52:22Z

/retest

fejta-bot · 2017-07-31T16:23:19Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

nikhita · 2017-07-31T16:42:37Z

Submit Queue says Required Github CI test is not green: pull-kubernetes-unit. But pull-kubernetes-unit is green...

nikhita · 2017-07-31T19:19:19Z

/retest

nikhita · 2017-07-31T19:36:13Z

/test pull-kubernetes-e2e-kops-aws

nikhita · 2017-07-31T20:38:35Z

/retest

nikhita · 2017-07-31T20:59:42Z

/retest

k8s-github-robot · 2017-07-31T23:56:59Z

Automatic merge from submit-queue (batch tested with PRs 49651, 49707, 49662, 47019, 49747)

@sttts

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

k8s-ci-robot requested a review from sttts July 27, 2017 20:54

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 27, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 27, 2017

k8s-github-robot assigned wojtek-t and lavalamp Jul 27, 2017

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Jul 27, 2017

nikhita mentioned this pull request Jul 27, 2017

apiextensions: validation for customresources #47263

Merged

13 tasks

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 27, 2017

sttts assigned smarterclayton and lavalamp and unassigned lavalamp Jul 27, 2017

wojtek-t reviewed Jul 28, 2017

View reviewed changes

nikhita force-pushed the conversion-gen-recursive-types branch from ad46f2a to 23d0445 Compare July 28, 2017 12:31

sttts reviewed Jul 28, 2017

View reviewed changes

nikhita force-pushed the conversion-gen-recursive-types branch from 23d0445 to f98b74b Compare July 28, 2017 18:40

sttts reviewed Jul 28, 2017

View reviewed changes

conversion-gen: support recursive types

c4656c3

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 July 28, 2017 19:13

k8s-ci-robot assigned sttts Jul 28, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 31, 2017

k8s-github-robot merged commit 088c198 into kubernetes:master Jul 31, 2017

nikhita mentioned this pull request Mar 1, 2018

conversion-gen fails with stack overflow when encountering a type that references itself #49553

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

conversion-gen: support recursive types #49747

conversion-gen: support recursive types #49747

nikhita commented Jul 27, 2017

k8s-ci-robot commented Jul 27, 2017

sttts commented Jul 27, 2017

sttts commented Jul 27, 2017

wojtek-t Jul 28, 2017

nikhita Jul 28, 2017

wojtek-t Jul 28, 2017

nikhita Jul 28, 2017

nikhita commented Jul 28, 2017

sttts Jul 28, 2017

nikhita Jul 28, 2017

sttts Jul 28, 2017

sttts Jul 28, 2017

sttts Jul 28, 2017

nikhita commented Jul 28, 2017

sttts Jul 28, 2017 •

edited

Loading

nikhita Jul 28, 2017

sttts commented Jul 28, 2017

sttts commented Jul 28, 2017

nikhita commented Jul 28, 2017

wojtek-t commented Jul 31, 2017

k8s-github-robot commented Jul 31, 2017

nikhita commented Jul 31, 2017

nikhita commented Jul 31, 2017

nikhita commented Jul 31, 2017

nikhita commented Jul 31, 2017

nikhita commented Jul 31, 2017

nikhita commented Jul 31, 2017

fejta-bot commented Jul 31, 2017

nikhita commented Jul 31, 2017

nikhita commented Jul 31, 2017

nikhita commented Jul 31, 2017

nikhita commented Jul 31, 2017

nikhita commented Jul 31, 2017

k8s-github-robot commented Jul 31, 2017

conversion-gen: support recursive types #49747

conversion-gen: support recursive types #49747

Conversation

nikhita commented Jul 27, 2017

k8s-ci-robot commented Jul 27, 2017

sttts commented Jul 27, 2017

sttts commented Jul 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhita commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhita commented Jul 28, 2017

sttts Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Jul 28, 2017

sttts commented Jul 28, 2017

nikhita commented Jul 28, 2017

wojtek-t commented Jul 31, 2017

k8s-github-robot commented Jul 31, 2017

nikhita commented Jul 31, 2017

nikhita commented Jul 31, 2017

nikhita commented Jul 31, 2017

nikhita commented Jul 31, 2017

nikhita commented Jul 31, 2017

nikhita commented Jul 31, 2017

fejta-bot commented Jul 31, 2017

nikhita commented Jul 31, 2017

nikhita commented Jul 31, 2017

nikhita commented Jul 31, 2017

nikhita commented Jul 31, 2017

nikhita commented Jul 31, 2017

k8s-github-robot commented Jul 31, 2017

sttts Jul 28, 2017 •

edited

Loading