Added GetMetadata function to Machine #114

cpheps · 2019-08-22T14:25:26Z

Issue #, if available:
Resolves #109

Description of changes:
Added a function GetMetadata function to the Machine to wrap the GetMmds of client. I had the function return a map[string]interface{}, I figured this was the path of least resistance to return the meta data and allow the caller to parse it.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

xibz · 2019-08-22T18:13:12Z

machine.go

+		return nil, err
+	}
+
+	payload, ok := resp.Payload.(map[string]interface{})


I don't know if I like defaulting to map[string]interface{} as []interface{} seems just as feasible. I think we need to think about this a little more, maybe use reflection to unmarshal into a structure may be nice.

func(m *Machine) GetMetadata(ctx context.Context, v interface{}) error { // TODO: use reflection here }

How do you feel about just updating the client for now and removing the machine GetMetadata and adding an issue asking for a higher level abstraction? Or if you prefer to implement the reflection portion, that'd be very helpful :D

I thought about reflection too, I wasn't sure if you guys wanted all that code inside of the sdk. I don't think it'll be too bad since the MMDs only allows strings, objects, and arrays of strings.

I assume for the reflection we would just unmarshal off of the json struct tags?

@cpheps - Yea, using json tags is the way to go, and I think this creates a much better interface for users to be able to have the SDK do the unmarshaling for them.

Agreed, I'll go down that path. I also noticed the Patch MMDs was not hooked up to the machine, that seems fairly straight forward and I need that functionality so I'm going to throw that onto this PR.

cpheps · 2019-08-26T13:08:11Z

@xibz Added reflection unmarshaling to GetMmds and hooked up the PATCH MMDs to the machine.

kzys · 2019-08-26T16:45:29Z

machine.go

+	}
+
+	m.logger.Printf("GetMetadata successful")
+	return unmarshalMetadata(resp.Payload, v)


Can't we just use json.Unmarshal here?

Ha, hadn't thought of that. Yes, as long as we can turn resp.Payload into a []byte. I'm not quite sure how to do that, I found this example

var buf bytes.Buffer enc := gob.NewEncoder(&buf) err = enc.Encode(resp.Payload) if err != nil { return err } return json.Unmarshal(buf.Bytes(), v)

but it returns an error during unmarshal.

Best I can come up with is to json.Marshal(resp.Payload) then feed those bytes into the json.Unmarshal

I think that's probably the best approach for now. The response is unmarshalled into an interface{} by the runtime.JSONConsumer and the godoc for json.Unmarshal says this:

To unmarshal JSON into an interface value, Unmarshal stores one of these in the interface value:

bool, for JSON booleans
float64, for JSON numbers
string, for JSON strings
[]interface{}, for JSON arrays
map[string]interface{}, for JSON objects
nil for JSON null

We don't actually know which type will end up in the resp.Payload field and would have to deal with that. Since we expect that the MMDS will contain JSON (that's the documented use), and since json.Unmarshal and json.Marshal (intend to) have inverse encodings, I think it's reasonable to let them operate together rather than rewriting it ourselves.

I think this is difficult primarily because the get operation's return value is defined as object in the Swagger spec, and that gets converted into interface{} in our client. If it were a string in the Swagger spec, we could either get a string or []byte back from the client, and then apply our own deserialization instead of one of these approaches.

I've opened firecracker-microvm/firecracker#1229 to discuss that possibility with the Firecracker team.

I'm not apposed to doing the json.Marshal into a json.Unmarshal. The reason I hand rolled the unmarshaling is the set of possible JSON that could be put into MMDS seemed limited. I could be wrong but seems like the top level object is either an array or json object that will eventual cascade down into a string value.

The json package will do this fine, and I don't think there is any danger of getting a non-string value out since we have to process something coming out of the MMDS.

I'll keep the custom code for now but let me know if we want to switch to the json package, it's easy enough to swap out.

@samuelkarp Thanks for the submission to the Firecracker team. One note I would add is currently the swagger is incorrect. It says object but it could also return an array of strings or objects. The swagger overall is a bit misleading if you aren't familiar with using MMDS.

My preference would be to switch to the json package, as then there's less code for us to test and maintain over time.

I could be wrong but seems like the top level object is either an array or json object that will eventual cascade down into a string value.

MMDS currently accepts strings, dictionaries, and objects at the top level, but I'd prefer not to have to know that (and maintain code that knows that).

@samuelkarp Works for me, just pushed the update.

kzys

Looks good to me!

xibz · 2019-08-27T19:02:37Z

machine.go

+
+// GetMetadata gets the machine's metadata from MDDS and unmarshals it into v
+func (m *Machine) GetMetadata(ctx context.Context, v interface{}) error {
+	if v == nil {


One minor comment is that we probably don't need to check for nil here as json.Unmarshal seems to behave fine and does not return an error.

Oh good call, left over artifact. I'll remove that

Can you also clean up the commits into a single commit and ensure the author is correct? If you take a look at the current commits only one of them actually references you.

xibz

Awesome! Thanks @cpheps for the contribution :D

samuelkarp

Thanks @cpheps! The code looks good to me, but would you be able to squash this all into a single commit? If not, we can take care of that when we merge it.

Added test for GetMetadata on machine Fixed some spelling issues and actually put test for GetMetadata Added MMDs PATCH and GET reflection (#1) Hooked up machine function for PATCH Mmds and added reflection unmarshaling for GetMmds Replaced custom marshaling for using the json package in GetMmds Removed extra nil check in GetMetadata

cpheps · 2019-08-28T12:17:59Z

@samuelkarp Sqashed all the commits.

cpheps · 2019-08-29T12:13:47Z

@samuelkarp Do you know if you guys are waiting for a resolution on firecracker-microvm/firecracker#1229 before merging? We're currently using my fork in production but would like to get back to the actual library just wondering on the timeline.

samuelkarp

Thanks @cpheps! My apologies for the delay; I had intended to merge this yesterday but ran out of time.

I'll be merging this with a rewritten commit message, so the PR may not show up as "Merged" but the code will be (and your email address will still be credited as the author).

samuelkarp · 2019-08-29T17:26:21Z

Merged as b000873, commit sha is different as the commit message was rewritten c3a3e5c.

mxpv approved these changes Aug 22, 2019

View reviewed changes

xibz reviewed Aug 22, 2019

View reviewed changes

kzys reviewed Aug 26, 2019

View reviewed changes

samuelkarp mentioned this pull request Aug 26, 2019

Consider specifying /mmds get schema as string rather than object firecracker-microvm/firecracker#1229

Closed

kzys approved these changes Aug 27, 2019

View reviewed changes

xibz reviewed Aug 27, 2019

View reviewed changes

xibz approved these changes Aug 27, 2019

View reviewed changes

samuelkarp reviewed Aug 27, 2019

View reviewed changes

samuelkarp approved these changes Aug 29, 2019

View reviewed changes

samuelkarp closed this Aug 29, 2019

Added GetMetadata function to Machine #114

Added GetMetadata function to Machine #114

Uh oh!

Conversation

cpheps commented Aug 22, 2019

Uh oh!

xibz Aug 22, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

cpheps commented Aug 26, 2019

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!

kzys left a comment

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!

xibz left a comment

Choose a reason for hiding this comment

Uh oh!

samuelkarp left a comment

Choose a reason for hiding this comment

Uh oh!

cpheps commented Aug 28, 2019

Uh oh!

cpheps commented Aug 29, 2019

Uh oh!

samuelkarp left a comment

Choose a reason for hiding this comment

Uh oh!

samuelkarp commented Aug 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xibz Aug 22, 2019 •

edited

Loading