Unstructured OpenstackProviderSpec by 2uasimojo · Pull Request #2196 · openshift/hive

2uasimojo · 2024-02-02T16:28:33Z

Another attempt to address the move of OpenstackProviderSpec from cluster-api-provider-openstack to openstack/api/machine. See #2117 for background. In that PR, we simply replaced our imports. The result is that we could only decode the new thing. The problem is that we may still get the old thing if we're talking to an old cluster.

We only need the decoding to steal the osImage from the master machines so we can use that value for new workers. Rather than importing both schemata, trying to figure out which version we're talking to, and decoding against the correct one, this commit unmarshals the providerSpec as raw JSON into an Unstructured object and explicitly paths into it map-wise.

On the creation side, we're told that the spoke cluster itself will understand the new thing even if it's an old version (MAPI also unmarshals the JSON raw). So it should be fine to continue using recent vendored installer code to generate MachineSets (which will contain new-apiVersion OpenstackProviderSpec).

HIVE-2308

2uasimojo · 2024-02-02T16:30:10Z

/assign @lleshchi
/cc @JoelSpeed @dtthuynh

2uasimojo · 2024-02-02T16:39:50Z

/hold

Trying to find out if we can do pre-merge testing.

codecov · 2024-02-02T17:47:31Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (20c3682) 57.80% compared to head (28dac77) 57.88%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2196      +/-   ##
==========================================
+ Coverage   57.80%   57.88%   +0.08%     
==========================================
  Files         188      188              
  Lines       26134    26133       -1     
==========================================
+ Hits        15106    15127      +21     
+ Misses       9756     9733      -23     
- Partials     1272     1273       +1

Files	Coverage Δ
...g/controller/machinepool/machinepool_controller.go	`52.65% <0.00%> (ø)`
pkg/controller/machinepool/openstackactuator.go	`19.09% <81.81%> (+19.09%)`	⬆️

fxierh · 2024-02-04T05:01:49Z

+	var u *unstructured.Unstructured
+	if err := json.Unmarshal(masterMachine.Spec.ProviderSpec.Value.Raw, u); err != nil {


I believe the second argument passed to Unmarshal cannot be nil.

Suggested change

var u *unstructured.Unstructured

if err := json.Unmarshal(masterMachine.Spec.ProviderSpec.Value.Raw, u); err != nil {

var u unstructured.Unstructured

if err := json.Unmarshal(masterMachine.Spec.ProviderSpec.Value.Raw, &u); err != nil {

@fxierh to the rescue again!

Also had to change the line below.

Rebased, revised, rebuilt, and re-pushed quay.io/2uasimojo/hive:openstack-providerspec-unstructured

Thanks!

I PTO-ed yesterday and I'll test it today.

2uasimojo · 2024-02-05T22:42:58Z

/hold cancel

@dtthuynh has verified this fix manually, seems to work 👍

JoelSpeed · 2024-02-06T10:01:21Z

-	providerSpec, err := decodeOpenStackMachineProviderSpec(masterMachine.Spec.ProviderSpec.Value, scheme)
-	if err != nil {
+func getOpenStackOSImage(masterMachine *machinev1beta1.Machine, logger log.FieldLogger) (string, error) {
+	var u unstructured.Unstructured


The OpenStack provider spec go type is in o/api, so you could use the structured type here rather than unstructured

Well, that's the whole issue, isn't it? How would I unmarshal it into a structured type if I don't know which apiVersion it came from? I had considered trying one and then the other... but this seemed more generic.

When the providerspecs were migrated from their original locations to the new locations, they remained backward compatible. The api version changed, but from a yaml unmarshalling and re-marshalling, it's absolutely compatible. You should be able to just work with the newer types from o/api

I'm still missing something though: How do we unmarshal the old-apiVersion Raw into the new-apiVersion go object? That's what the code was attempting to do before, and it was erroring. Or do we do it by casting the Object instead? Or is it because we were using a decoder that had some internal validation for the apiVersion?

Don't use the decoder. The old raw in a standard json.Unmarshal will unmarshal without error into the new struct. The decoder validates internally by trying to find the correct go type for a registered and known API group and version, but we weren't registering those because they are raw extensions

Okay, I think I get it. Using json.Unmarshal doesn't try to validate that the go object was defined in the same library that the json blob's .apiVersion field represents. (How would it? That's the point of registering the schema.) So all it would be doing is translating fields using the standard json-to-golang rules and blatting in the values.

Side note: It's legal for the schema to not follow those standard field mapping rules. So in theory we could end up with blank fields in the go object that would have been populated if we were using the schema-based decoder. Right?

Side note: Presumably we would want to use non-strict unmarshaling. If (additive, backward-compatible) changes are made to the APIs and there's a lag before we revendor to incorporate those changes, we don't want the unmarshal to puke because it can't match a field in the struct. (In theory the aforementioned nonstandard field name mappings could also trigger same.)

Side note: Um, if I unmarshaled an old-apiVersion object into a new-apiVersion struct, I would end up with the apiVersion in the struct being "wrong" -- i.e. not matching the library I imported the struct from -- true? As long as I'm careful about how I'm using the thing, it wouldn't be a problem, but I can see it being dangerous to start sending it around the ecosystem in that form...

All that said, pathing the raw json as we ended up doing here seems like not a significantly different degree of evil, so I'm thinking to keep it as is. Are you okay with that?

Side note: It's legal for the schema to not follow those standard field mapping rules. So in theory we could end up with blank fields in the go object that would have been populated if we were using the schema-based decoder. Right?

Is it? I've not seen that. Can you provide an example?

Side note: Presumably we would want to use non-strict unmarshaling. If (additive, backward-compatible) changes are made to the APIs and there's a lag before we revendor to incorporate those changes, we don't want the unmarshal to puke because it can't match a field in the struct. (In theory the aforementioned nonstandard field name mappings could also trigger same.)

API changes should only be additive, to my knowledge there has been no breaking change in MAPI providerSpecs so you should be able to use unmarshal strict without issue

Side note: Um, if I unmarshaled an old-apiVersion object into a new-apiVersion struct, I would end up with the apiVersion in the struct being "wrong" -- i.e. not matching the library I imported the struct from -- true? As long as I'm careful about how I'm using the thing, it wouldn't be a problem, but I can see it being dangerous to start sending it around the ecosystem in that form...

The string isn't checked anywhere in MAPI, and the marshal/unmarshal shouldn't be affected by the change in go type, as mentioned, you may start seeing additional fields marshalled to their empty values maybe, but, this won't affect MAPI since we always unmarshal directly into the new go types

All that said, pathing the raw json as we ended up doing here seems like not a significantly different degree of evil, so I'm thinking to keep it as is. Are you okay with that?

If you feel more comfortable with that then sure. IMO aligning with what we do in MAPI is probably better but this works as well

lleshchi · 2024-02-06T16:00:20Z

+		}
 	}
-	return spec, nil
+	return u.Object["image"].(string)


Would it be good practice to add some error handling here, in case of an unexpected object error (i.e. future api changes such as this one)?

Totally. I'll work something up.

I'm wondering if it is possible to use utilities defined in the unstructured package like unstructured.NestedString which returns a triplet of type (string, bool, error) ?
e.g.

hive/vendor/k8s.io/kubectl/pkg/cmd/wait/wait.go

Lines 536 to 564 in 25794ae

func (w ConditionalWait) checkCondition(obj *unstructured.Unstructured) (bool, error) {

conditions, found, err := unstructured.NestedSlice(obj.Object, "status", "conditions")

if err != nil {

return false, err

}

if !found {

return false, nil

}

for _, conditionUncast := range conditions {

condition := conditionUncast.(map[string]interface{})

name, found, err := unstructured.NestedString(condition, "type")

if !found || err != nil || !strings.EqualFold(name, w.conditionName) {

continue

}

status, found, err := unstructured.NestedString(condition, "status")

if !found || err != nil {

continue

}

generation, found, _ := unstructured.NestedInt64(obj.Object, "metadata", "generation")

if found {

observedGeneration, found := getObservedGeneration(obj, condition)

if found && observedGeneration < generation {

return false, nil

}

}

return strings.EqualFold(status, w.conditionStatus), nil

}

return false, nil

I also considered using gjson. Figured the paths were small/simple enough that it wasn't worth it... but now looking at it I'm not so sure. Let me play...

Yeah, I like that a lot better @fxierh, changed. Thanks for the suggestion!

Another attempt to address the move of OpenstackProviderSpec from cluster-api-provider-openstack to openstack/api/machine. See openshift#2117 for background. In that PR, we simply replaced our imports. The result is that we could only decode the _new_ thing. The problem is that we may still get the _old_ thing if we're talking to an old cluster. We only need the decoding to steal the osImage from the master machines so we can use that value for new workers. Rather than importing both schemata, trying to figure out which version we're talking to, and decoding against the correct one, this commit unmarshals the providerSpec as raw JSON into an Unstructured object and explicitly paths into it map-wise. On the creation side, we're told that the spoke cluster itself will understand the new thing even if it's an old version (MAPI also unmarshals the JSON raw). So it should be fine to continue using recent vendored installer code to generate MachineSets (which will contain new-apiVersion OpenstackProviderSpec). HIVE-2308

lleshchi · 2024-02-06T18:23:19Z

/lgtm

openshift-ci · 2024-02-06T18:25:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 2uasimojo, lleshchi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [2uasimojo,lleshchi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2024-02-06T19:22:24Z

@2uasimojo: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

2uasimojo · 2024-02-06T19:42:44Z

/cherry-pick mce-2.5

2uasimojo · 2024-02-06T20:29:10Z

/cherry-pick mce-2.5

Sure, just ignore me completely, bot. I'll do it manually: #2200

This is a manual backport of openshift#2196 to address the move of OpenstackProviderSpec from cluster-api-provider-openstack to openstack/api/machine. We need to decode OpenstackProviderSpec to steal the osImage from the master machines so we can use that value for new workers. Rather than importing both schemata, trying to figure out which version we're talking to, and decoding against the correct one, this commit unmarshals the providerSpec as raw JSON into an Unstructured object and explicitly paths into it map-wise. On the creation side, we're told that the spoke cluster itself will understand the new thing even if it's an old version (MAPI also unmarshals the JSON raw). So it should be fine to continue using the existing version of vendored installer code to generate MachineSets. HIVE-2308

openshift-ci Bot assigned lleshchi Feb 2, 2024

openshift-ci Bot requested review from JoelSpeed, dtthuynh, jstuever and lleshchi February 2, 2024 16:30

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2024

openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 2, 2024

fxierh reviewed Feb 4, 2024

View reviewed changes

2uasimojo force-pushed the HIVE-2308/openstack-providerspec-unstructured branch from 25794ae to 6911ed3 Compare February 4, 2024 14:55

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 5, 2024

JoelSpeed reviewed Feb 6, 2024

View reviewed changes

lleshchi reviewed Feb 6, 2024

View reviewed changes

2uasimojo force-pushed the HIVE-2308/openstack-providerspec-unstructured branch from 6911ed3 to a5cf25d Compare February 6, 2024 17:30

2uasimojo force-pushed the HIVE-2308/openstack-providerspec-unstructured branch from a5cf25d to 28dac77 Compare February 6, 2024 17:44

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Feb 6, 2024

openshift-merge-bot Bot merged commit 6f1fd3e into openshift:master Feb 6, 2024

2uasimojo deleted the HIVE-2308/openstack-providerspec-unstructured branch February 6, 2024 19:42

2uasimojo mentioned this pull request Feb 6, 2024

Unstructured OpenstackProviderSpec #2202

Merged

2uasimojo mentioned this pull request Feb 6, 2024

Unstructured OpenstackProviderSpec #2203

Merged

		var u *unstructured.Unstructured
		if err := json.Unmarshal(masterMachine.Spec.ProviderSpec.Value.Raw, u); err != nil {

	func (w ConditionalWait) checkCondition(obj *unstructured.Unstructured) (bool, error) {
	conditions, found, err := unstructured.NestedSlice(obj.Object, "status", "conditions")
	if err != nil {
	return false, err
	}
	if !found {
	return false, nil
	}
	for _, conditionUncast := range conditions {
	condition := conditionUncast.(map[string]interface{})
	name, found, err := unstructured.NestedString(condition, "type")
	if !found \|\| err != nil \|\| !strings.EqualFold(name, w.conditionName) {
	continue
	}
	status, found, err := unstructured.NestedString(condition, "status")
	if !found \|\| err != nil {
	continue
	}
	generation, found, _ := unstructured.NestedInt64(obj.Object, "metadata", "generation")
	if found {
	observedGeneration, found := getObservedGeneration(obj, condition)
	if found && observedGeneration < generation {
	return false, nil
	}
	}
	return strings.EqualFold(status, w.conditionStatus), nil
	}

	return false, nil

Conversation

2uasimojo commented Feb 2, 2024 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

2uasimojo commented Feb 2, 2024

Uh oh!

2uasimojo commented Feb 2, 2024

Uh oh!

codecov Bot commented Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

2uasimojo commented Feb 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fxierh Feb 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lleshchi commented Feb 6, 2024

Uh oh!

openshift-ci Bot commented Feb 6, 2024

Uh oh!

openshift-ci Bot commented Feb 6, 2024

Uh oh!

2uasimojo commented Feb 6, 2024

Uh oh!

2uasimojo commented Feb 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

2uasimojo commented Feb 2, 2024 •

edited by openshift-ci Bot

Loading

codecov Bot commented Feb 2, 2024 •

edited

Loading

fxierh Feb 6, 2024 •

edited

Loading