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.

Sign up for GitHub

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

Jump to bottom

Fix int/float conversion issues in CRD version remapping plugin #2322

Merged
merged 7 commits into from
Mar 10, 2020
Merged

Fix int/float conversion issues in CRD version remapping plugin #2322

merged 7 commits into from
Mar 10, 2020

Conversation

nrb
Copy link
Contributor

@nrb nrb commented Mar 5, 2020 •
edited
Loading

Fixes #2319

nrb added 2 commits March 5, 2020 15:48
@nrb
Add builders for CRD schemas
50c28da 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Add test case for vmware-tanzu#2319
3f97eaa 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb nrb self-assigned this Mar 5, 2020
nrb added 2 commits March 5, 2020 16:02
@nrb
Add failing test case
add14dd 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Remove unnecessary print and temporary variable
6aaf019 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb nrb mentioned this pull request Mar 5, 2020
Closed
nrb
nrb commented Mar 5, 2020
View reviewed changes
pkg/backup/remap_crd_version_action.go Outdated
@@ -61,6 +61,7 @@ func (a *RemapCRDVersionAction) Execute(item runtime.Unstructured, backup *v1.Ba
}

// We've got a v1 CRD, so proceed.
// TODO: why is this causing int/float errors?
var crd apiextv1.CustomResourceDefinition
if err := runtime.DefaultUnstructuredConverter.FromUnstructured(item.UnstructuredContent(), &crd); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing this to a JSON marshalling dance causes the unit test to pass, but I have no measured impact on actual restores yet.

	var crd apiextv1.CustomResourceDefinition
	js, err := json.Marshal(item.UnstructuredContent())
	if err != nil {
		return nil, nil, errors.Wrap(err, "unable to convert unstructured item to JSON")
	}

	if err = json.Unmarshal(js, &crd); err != nil {
		return nil, nil, errors.Wrap(err, "unable to convert JSON to CRD")
	}

I'm also exploring using the helpers in the unstructured package, but I think they may be unwieldy given the number of fields we access in this plugin.

I'll try to see if I can log any sort of timing data to help make this decision.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the timing data at https://docs.google.com/spreadsheets/d/16YNPgBOlAzSBxyg_6Bsm0cLtZ2hBob0Vr_nz9vwYYL8/edit?usp=sharing in the crd-log tab, I'm leaning towards doing the JSON marshalling dance and no the unstructured helpers & casting for readability. Thoughts, @skriss and @ashish-amarnath?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So basically, using FromUnstructured does not properly convert the integer value to a float, but marshaling the Unstructured to JSON and then using json.Unmarshal does? Is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that appears to be the case. Also in my test case, if the value was 5.0, it would fail with FromUnstructured, but it would be fine with 5.2. So any whole number appears to have problems with the FromUnstructured implementation right now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I'm onboard with going through JSON for this -- let's just add a good comment explaining.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you wanted to do the same in the other spot too, that'd be OK - not sure if it's worth changing or not, up to you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem sounds super weird! But the changes seem fine.
The impact of the change doesn't seem to be that much. This may be one exception to keep commented code. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashish-amarnath I'll keep a mention of the full path to the call, but not all the error handling, as well as a mention of the Kubernetes issue.

@skriss I'll look at the other site and see if it's worth doing the JSON marshalling. If it's a single field, it's probably not worth it.

nrb added 2 commits March 5, 2020 17:03
@nrb
Add some options for fixing the test case
1adfaf2 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Switch to a JSON middle step to "fix" conversions
717f1b2 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb nrb marked this pull request as ready for review March 6, 2020 22:10
@skriss
Copy link
Contributor

skriss commented Mar 6, 2020

LGTM @nrb, just need to add a changelog

@nrb
Add comment and changelog
343e0e7 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb nrb requested a review from carlisia March 9, 2020 19:01
@nrb
Copy link
Contributor Author

nrb commented Mar 9, 2020

@ashish-amarnath @carlisia Could you please review this today? I'd like to release a v1.3.1 with this fix on Tuesday, March 10 if possible. @NissesSenap tested an image of this with his data and had no issues, but would like to make sure we're all ok with the solution.

ashish-amarnath
ashish-amarnath approved these changes Mar 9, 2020
View reviewed changes
Copy link
Member

@ashish-amarnath ashish-amarnath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

skriss
skriss approved these changes Mar 10, 2020
View reviewed changes
Copy link
Contributor

@skriss skriss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, merging.

@skriss skriss merged commit 040f680 into vmware-tanzu:master Mar 10, 2020
nrb added a commit to nrb/velero that referenced this pull request Mar 10, 2020
@nrb
Fix int/float conversion issues in CRD version remapping plugin (vmwa…
26b06f7 
…re-tanzu#2322)

* Add builders for CRD schemas

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Add test case for vmware-tanzu#2319

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Add failing test case

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Remove unnecessary print and temporary variable

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Add some options for fixing the test case

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Switch to a JSON middle step to "fix" conversions

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Add comment and changelog

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
sseago pushed a commit to sseago/velero that referenced this pull request Mar 12, 2020
@nrb @sseago
Fix int/float conversion issues in CRD version remapping plugin (vmwa…
ed08c6d 
…re-tanzu#2322)

* Add builders for CRD schemas

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Add test case for vmware-tanzu#2319

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Add failing test case

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Remove unnecessary print and temporary variable

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Add some options for fixing the test case

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Switch to a JSON middle step to "fix" conversions

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Add comment and changelog

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skriss skriss skriss approved these changes

@ashish-amarnath ashish-amarnath ashish-amarnath approved these changes

@carlisia carlisia Awaiting requested review from carlisia

Assignees

@nrb nrb

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error: unable to convert unstructured item to a v1 CRD: cannot convert int64 to float64
3 participants
@nrb @skriss @ashish-amarnath