Baremetal Inspector Client Node Introspection by elfosardo · Pull Request #1486 · gophercloud/gophercloud

elfosardo · 2019-02-27T15:10:51Z

This patch adds the Node Introspection API calls according to [1]

Start Introspection (POST)
Get Introspection status (GET)
List all Introspection statuses (GET)
Abort Introspection (POST)
Get Introspection Data (GET)
Reapply Introspection on stored data (POST)

Source code reference:
https://github.com/openstack/ironic-inspector/tree/master/ironic_inspector

[1] https://developer.openstack.org/api-ref/baremetal-introspection/#node-introspection

theopenlab-ci · 2019-02-27T16:13:25Z

Build succeeded.

gophercloud-unittest : SUCCESS in 3m 04s
gophercloud-acceptance-test : SUCCESS in 1h 01m 31s

theopenlab-ci · 2019-02-27T17:40:24Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 28s
gophercloud-acceptance-test : SUCCESS in 54m 58s

coveralls · 2019-02-27T18:48:38Z

Coverage increased (+0.07%) to 76.597% when pulling 9e222ec on elfosardo:baremetal-inspector-support into 6e3895e on gophercloud:master.

theopenlab-ci · 2019-02-27T19:22:14Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 12s
gophercloud-acceptance-test : SUCCESS in 1h 27m 30s

theopenlab-ci · 2019-02-28T10:07:42Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 40s
gophercloud-acceptance-test : SUCCESS in 1h 27m 44s

theopenlab-ci · 2019-02-28T12:46:14Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 35s
gophercloud-acceptance-test : SUCCESS in 1h 26m 47s

theopenlab-ci · 2019-02-28T15:45:54Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 12s
gophercloud-acceptance-test : SUCCESS in 40m 45s

theopenlab-ci · 2019-03-01T11:25:15Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 10s
gophercloud-acceptance-test : SUCCESS in 1h 02m 06s

elfosardo · 2019-03-01T11:28:54Z

@jtopjian I'm seeing again the same issue mentioned in #1465
any advice on how to avoid that here ?

theopenlab-ci · 2019-03-01T13:09:30Z

Build succeeded.

gophercloud-unittest : SUCCESS in 3m 03s
gophercloud-acceptance-test : SUCCESS in 1h 03m 57s

jtopjian · 2019-03-01T15:29:46Z

@elfosardo Try removing all occurrences of fields that are set to interface{}(nil). The tests should still pass (omitting the field is the same as setting it to interface{}(nil)) and gofmt should work with all versions.

theopenlab-ci · 2019-03-01T17:23:37Z

Build succeeded.

gophercloud-unittest : SUCCESS in 3m 06s
gophercloud-acceptance-test : SUCCESS in 1h 27m 01s

elfosardo · 2019-03-04T10:35:26Z

@jtopjian I tried removing the occurrences but it was giving an error as it was expecting them with a value of nil
I checked the format script and I can see there are 2 files in an exclusion list that seem to have the same issue:

ignore_files=(
  "./openstack/compute/v2/extensions/quotasets/testing/fixtures.go"
  "./openstack/networking/v2/extensions/vpnaas/ikepolicies/testing/requests_test.go"
)

I think adding fixtures.go to the ignore list is reasonable at this point considering that the format is actually correct and it's just a problem between go versions.

iurygregory

Looks good

jtopjian

@elfosardo This is looking good - thank you for the time you've put in.

I've added some comments with some links to help with additional structs. I could be wrong with them, though. Please let me know if you have any questions.

openstack/baremetalinspector/v1/introspection/testing/fixtures.go

openstack/baremetalinspector/v1/introspection/results.go

jtopjian · 2019-03-13T00:09:09Z

@juliakreger @elfosardo Attempting to clarify my comments a little more:

First, I fully understand if I'm coming off as being overly picky with an addition of OCD. Can the entire result be combined into a generic map[string]interface{}? Yes, absolutely. But by doing that, we're removing a large part of why someone would choose Go over another language: the strict type implementation.

If Ironic Inspector doesn't actually have an official API response definition, okay... but that makes API guarantees difficult, not to mention actually using the result in production. If we were to use a generic map[string]interface{} for everything, production code implementing Gophercloud would need to look like:

if v, ok := result["macs"]; ok {
  if v, ok := v.([]interface{}); ok {
    for _, i := range v {
      if mac, ok := i.(string); ok {
        fmt.Println(mac)
      }
    }
  }
}

Instead of:

for _, mac := range result.MACs {
  fmt.Println(mac)
}

Based on the above, you can see how digging into a more nested piece of data such as ipv4_address would become much more difficult. There is a lot of room for causing panics due to not checking if a key exists or not doing a type assertion.

This is why I place so much emphasis on data modeling.

Second, since I don't have access to an Ironic/Inspector environment, I can only go off of what the fixtures are telling me. Looking at the timestamps of the fixtures, I can tell that the fixtures bundled in this PR were not generated recently in a devstack or prod environment. They were pulled from here:

This means that I must trust that the fixtures are accurate and weren't amended for convenience of writing a quick unit test. This sounds trivial, but incorrect fixtures have occurred before. Go is the kind of language that is going to quickly let you know when a mocked fixture is not the same as what it sees in reality.

The fixtures are telling me that well-formed data structures are being returned. This is because of the amount of null values being returned. If it was truly a generic hash/dictionary/map being returned, then null keys wouldn't even be set and thus not be in the response.

The latest commit has done a very good job at modeling the API response into a series of well-defined structures. The additional comments I gave are an attempt to model the remaining generic map[string]interfaces{}. If that's not possible, that is totally fine.

I apologize if I'm coming across as picking on this PR -- that is not my intention at all. This PR has the ~~unfortunate~~ very difficult goal of modeling a very structured set of data. If you look through the rest of Gophercloud, a lot of the work is done in the modeling of API responses.

acceptance/clients/clients.go

openstack/baremetalinspector/v1/introspection/requests.go

dtantsur · 2019-03-13T15:33:42Z

openstack/baremetalinspector/v1/introspection/results.go

+// and plugins enabled both in the ramdisk and in inspector itself.
+// This structure has been provided for basic compatibility but it
+// will need extensions
+type Data struct {


I hope these fields are all optional, because we don't have any guarantees about it. Actually, maybe we should leave only inventory since it's the only thing that has a defined (in IPA) format.

Please see my other comments in this area. I'm hesitant to have Data as one large map[string]interface{} for reasons mentioned. I'm a bit surprised if this API has no guaranteed / defined structure at all.

However, as I've also stated, I have no knowledge of any of these APIs, so I'm leaning on other peoples' expertise. If it's best to have generic map[string]interfaces{} in certain areas, okay. But using the results is going to be a headache.

Also, if a JSON response does not contain a field that a struct is expecting, all is fine. If a JSON response contains extra fields that the struct does not account for, all is fine.

The problem with this data is that it's a hybrid of more or less well defined inventory and whatever ironic-inspector plugins put there (which is poorly documented and often legacy). Is it possible in Go to have a hybrid approach as well? I know in serde (Rust serializatin library) you can have a defined structure with one mapping field serving as catch-all for everything that was not mapped to defined fields.

Go doesn't have anything which natively does that (but it would be nice). We have an internal function that does it, though. We use it for things like Keystone where arbitrary extra key/values can be sent in the API response:

https://github.com/gophercloud/gophercloud/blob/master/openstack/identity/v3/users/results.go#L60-L74

I've never tested it with nested things, but it should work.

If we can get a confirmed set of base/expected fields, I'm more than happy to work on options to catch the undefined/optional pieces.

openstack/baremetalinspector/v1/introspection/results.go

dtantsur · 2019-03-13T15:54:18Z

openstack/baremetalinspector/v1/introspection/results.go

+	Inventory     InventoryType          `json:"inventory"`
+	Error         string                 `json:"error"`
+	LocalGB       int                    `json:"local_gb"`
+	AllInterfaces map[string]interface{} `json:"all_interfaces"`


Speaking of poorly defined interfaces, the extra_hardware plugin defines the extra key, which is a 3 level nested dict: https://github.com/openstack/ironic-inspector/blob/master/ironic_inspector/plugins/extra_hardware.py#L66

My understanding is that it will not cause any issue if it's not present in the structure.
Of course it will be interpreted and not formally defined.

elfosardo · 2019-03-13T17:00:01Z

@jtopjian @dtantsur @juliakreger I don't mind moving forward defining an even more formal definition of the data structure, or just refine what we already have.
I think it's clear that because of how the json data is built we need to consider basically the entire response made by optional fields.

theopenlab-ci · 2019-03-13T18:14:59Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 53s
gophercloud-acceptance-test : SUCCESS in 1h 21m 54s
gophercloud-acceptance-test-ironic : SUCCESS in 33m 20s

theopenlab-ci · 2019-03-13T21:22:00Z

Build failed.

gophercloud-unittest : SUCCESS in 2m 42s
gophercloud-acceptance-test : SUCCESS in 42m 28s
gophercloud-acceptance-test-ironic : RETRY_LIMIT in 6s

jtopjian

@elfosardo I've done a detailed review and commented on a collection of minor things to update.

Once these are fixed and if you folks are happy with this, I'm comfortable merging it. If you want to make further changes to how the data is modeled (either to be more specific or more generic), I'll trust your judgements on this.

openstack/baremetalintrospection/v1/introspection/requests.go

openstack/baremetalintrospection/v1/introspection/results.go

openstack/baremetalintrospection/v1/introspection/testing/fixtures.go

openstack/baremetalintrospection/v1/introspection/testing/requests_test.go

theopenlab-ci · 2019-03-14T12:43:18Z

Build failed.

gophercloud-unittest : SUCCESS in 2m 58s
gophercloud-acceptance-test : SUCCESS in 55m 00s
gophercloud-acceptance-test-ironic : RETRY_LIMIT in 6s

elfosardo · 2019-03-14T13:52:03Z

@jtopjian @dtantsur @juliakreger I made some other changes to be as close as possible to the data defined in the base API code (both inspector and IPA) and modified the json data fixture as well since there were deprecated fields.
The schema should be now correct with no generic maps or interfaces.
Please let me know if I forgot anything, thanks!

For gophercloud#1485 This patch adds the Node Introspection API calls according to [1] Start Introspection (POST) Get Introspection status (GET) List all Introspection statuses (GET) Abort Introspection (POST) Get Introspection Data (GET) Reapply Introspection on stored data (POST) Source code reference: https://github.com/openstack/ironic-inspector/tree/master/ironic_inspector [1] https://developer.openstack.org/api-ref/baremetal-introspection/#node-introspection

theopenlab-ci · 2019-03-14T15:06:29Z

Build failed.

gophercloud-unittest : SUCCESS in 3m 03s
gophercloud-acceptance-test : FAILURE in 50m 46s
gophercloud-acceptance-test-ironic : SUCCESS in 25m 36s

jtopjian

LGTM. Please let me know when I should merge this or if you all would like to discuss/work on it more.

juliakreger

Looks good to me.

jtopjian · 2019-03-15T02:04:48Z

I'm going to merge this, but keep in mind this isn't written in stone - follow-up PRs are totally fine.

dtantsur · 2019-03-15T15:31:53Z

openstack/baremetalintrospection/v1/introspection/results.go

+// and plugins enabled both in the ramdisk and in inspector itself.
+// This structure has been provided for basic compatibility but it
+// will need extensions
+type Data struct {


I would repeat my recommendation to leave only Inventory here (and possibly add Extra). Everything else is not set in stone, with some items being legacy from quite ancient times. Instead, I'd provide a way for consumers to retrieve the whole data as map[string]interface and do whatever they want with it.

dtantsur · 2019-03-15T15:32:26Z

openstack/baremetalintrospection/v1/introspection/results.go

+	HasCarrier  bool                   `json:"has_carrier"`
+	IPV4Address string                 `json:"ipv4_address"`
+	IPV6Address string                 `json:"ipv6_address"`
+	Lldp        map[string]interface{} `json:"lldp"`


Why MAC and BIOS, but Lldp?

dtantsur · 2019-03-15T15:32:43Z

openstack/baremetalintrospection/v1/introspection/results.go

+	BIOSDevName string                 `json:"biosdevname"`
+	ClientID    string                 `json:"client_id"`
+	HasCarrier  bool                   `json:"has_carrier"`
+	IPV4Address string                 `json:"ipv4_address"`


I think IPv4Address feels a bit more natural.

dtantsur

Late to the party, but left some comments.

elfosardo · 2019-03-15T15:37:44Z

@dtantsur thanks for the comments, I will do a follow-up with some minor changes and to define better lldp as it has a defined schema

elfosardo mentioned this pull request Feb 27, 2019

Add support for Ironic baremetal Introspection API #1485

Open

13 tasks

elfosardo force-pushed the baremetal-inspector-support branch from 200eaf0 to b8a1a6d Compare February 27, 2019 16:44

elfosardo force-pushed the baremetal-inspector-support branch 3 times, most recently from 8639b80 to 4709d12 Compare February 27, 2019 17:53

elfosardo force-pushed the baremetal-inspector-support branch from 4709d12 to 5097e3c Compare February 28, 2019 08:38

elfosardo force-pushed the baremetal-inspector-support branch 2 times, most recently from 3ba01a3 to d752168 Compare February 28, 2019 11:18

elfosardo force-pushed the baremetal-inspector-support branch from d752168 to 888d5a9 Compare February 28, 2019 15:03

elfosardo force-pushed the baremetal-inspector-support branch 4 times, most recently from 8f0daef to 67b2d94 Compare March 1, 2019 10:21

elfosardo force-pushed the baremetal-inspector-support branch 2 times, most recently from 8f08ec9 to 94cd700 Compare March 1, 2019 12:03

elfosardo changed the title ~~[WIP] Baremetal Inspector Client Node Introspection~~ Baremetal Inspector Client Node Introspection Mar 1, 2019

elfosardo force-pushed the baremetal-inspector-support branch from 94cd700 to 6ffaa15 Compare March 1, 2019 15:55

elfosardo force-pushed the baremetal-inspector-support branch 2 times, most recently from 7855b16 to 5fb7d9b Compare March 8, 2019 14:41

iurygregory approved these changes Mar 11, 2019

View reviewed changes

jtopjian reviewed Mar 12, 2019

View reviewed changes

dtantsur suggested changes Mar 13, 2019

View reviewed changes

dtantsur reviewed Mar 13, 2019

View reviewed changes

elfosardo force-pushed the baremetal-inspector-support branch from 5fb7d9b to 1b26cc9 Compare March 13, 2019 16:51

elfosardo force-pushed the baremetal-inspector-support branch from 1b26cc9 to f6c4085 Compare March 13, 2019 20:38

jtopjian requested changes Mar 14, 2019

View reviewed changes

elfosardo force-pushed the baremetal-inspector-support branch 2 times, most recently from aab8c1f to ec371f2 Compare March 14, 2019 11:47

elfosardo force-pushed the baremetal-inspector-support branch from ec371f2 to cb74dfa Compare March 14, 2019 13:47

elfosardo force-pushed the baremetal-inspector-support branch from cb74dfa to 9e222ec Compare March 14, 2019 14:14

jtopjian approved these changes Mar 14, 2019

View reviewed changes

juliakreger approved these changes Mar 14, 2019

View reviewed changes

jtopjian merged commit 954aa14 into gophercloud:master Mar 15, 2019

dtantsur reviewed Mar 15, 2019

View reviewed changes

Conversation

elfosardo commented Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theopenlab-ci bot commented Feb 27, 2019

Uh oh!

theopenlab-ci bot commented Feb 27, 2019

Uh oh!

coveralls commented Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theopenlab-ci bot commented Feb 27, 2019

Uh oh!

theopenlab-ci bot commented Feb 28, 2019

Uh oh!

theopenlab-ci bot commented Feb 28, 2019

Uh oh!

theopenlab-ci bot commented Feb 28, 2019

Uh oh!

theopenlab-ci bot commented Mar 1, 2019

Uh oh!

elfosardo commented Mar 1, 2019

Uh oh!

theopenlab-ci bot commented Mar 1, 2019

Uh oh!

jtopjian commented Mar 1, 2019

Uh oh!

theopenlab-ci bot commented Mar 1, 2019

Uh oh!

elfosardo commented Mar 4, 2019

Uh oh!

iurygregory left a comment

Choose a reason for hiding this comment

Uh oh!

jtopjian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jtopjian commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elfosardo commented Mar 13, 2019

Uh oh!

theopenlab-ci bot commented Mar 13, 2019

Uh oh!

theopenlab-ci bot commented Mar 13, 2019

Uh oh!

jtopjian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elfosardo commented Feb 27, 2019 •

edited

Loading

coveralls commented Feb 27, 2019 •

edited

Loading

jtopjian commented Mar 13, 2019 •

edited

Loading