Compute v2: Availability Zones Info #704

dstdfx · 2018-01-03T19:07:27Z

Add ability to get list and detailed AZs info

For #320

Links to the line numbers/files in the OpenStack source code that support the
code in this PR:

/os-availability-zone
- API doc
- Source code
/os-availability-zone/detail
- API doc
- Source code

Add ability to get list and detailed AZs info

theopenlab-ci · 2018-01-03T19:44:40Z

Build succeeded.

gophercloud-unittest http://logs.openlabtesting.org/logs/04/704/bfb4a53d21cb62881faf33a82dcbe4cde8ce7621/check/gophercloud-unittest/efb81ac/ : SUCCESS in 1m 16s
gophercloud-acceptance-test http://logs.openlabtesting.org/logs/04/704/bfb4a53d21cb62881faf33a82dcbe4cde8ce7621/check/gophercloud-acceptance-test/6efa0af/ : SUCCESS in 27m 39s

coveralls · 2018-01-03T20:09:44Z

Coverage increased (+0.1%) to 72.953% when pulling a3e8e09 on dstdfx:availability-zones-info into 2f0d338 on gophercloud:master.

theopenlab-ci · 2018-01-03T20:17:30Z

Build succeeded.

gophercloud-unittest http://logs.openlabtesting.org/logs/04/704/a3e8e09223c8285c97d9b4851a7388c7157ab98d/check/gophercloud-unittest/f13f1b0/ : SUCCESS in 1m 11s
gophercloud-acceptance-test http://logs.openlabtesting.org/logs/04/704/a3e8e09223c8285c97d9b4851a7388c7157ab98d/check/gophercloud-acceptance-test/912147f/ : SUCCESS in 27m 12s

jtopjian

@dstdfx Thank you for working on this. I did a cursory review and spotted a few things that need changed.

First, can you use the following test fixture?

{
    "availabilityZoneInfo": [
        {
            "hosts": {
                "localhost": {
                    "nova-cert": {
                        "active": true,
                        "available": false,
                        "updated_at": "2017-10-14T17:03:39.000000"
                    },
                    "nova-conductor": {
                        "active": true,
                        "available": false,
                        "updated_at": "2017-10-14T17:04:09.000000"
                    },
                    "nova-consoleauth": {
                        "active": true,
                        "available": false,
                        "updated_at": "2017-10-14T17:04:18.000000"
                    },
                    "nova-scheduler": {
                        "active": true,
                        "available": false,
                        "updated_at": "2017-10-14T17:04:30.000000"
                    }
                },
                "openstack-acc-tests.novalocal": {
                    "nova-cert": {
                        "active": true,
                        "available": true,
                        "updated_at": "2018-01-04T04:11:19.000000"
                    },
                    "nova-conductor": {
                        "active": true,
                        "available": true,
                        "updated_at": "2018-01-04T04:11:22.000000"
                    },
                    "nova-consoleauth": {
                        "active": true,
                        "available": true,
                        "updated_at": "2018-01-04T04:11:20.000000"
                    },
                    "nova-scheduler": {
                        "active": true,
                        "available": true,
                        "updated_at": "2018-01-04T04:11:23.000000"
                    }
                }
            },
            "zoneName": "internal",
            "zoneState": {
                "available": true
            }
        },
        {
            "hosts": {
                "openstack-acc-tests.novalocal": {
                    "nova-compute": {
                        "active": true,
                        "available": true,
                        "updated_at": "2018-01-04T04:11:23.000000"
                    }
                }
            },
            "zoneName": "nova",
            "zoneState": {
                "available": true
            }
        }
    ]
}

If I recall correctly, the blocker I spotted in #327 was that the test fixture was incorrect. The above fixture is pulled from a running openstack environment so we can guarantee it works.

Second, OS should be removed from the names/variables. ie instead of OSAvailabilityZonePage, it should be AvailabilityZonePage.

I haven't done an exhaustive review of this yet, but the above items should be fixed before any further review is done.

Again, thank you for working on this and let me know if you have any questions.

jtopjian · 2018-01-04T04:13:53Z

openstack/compute/v2/extensions/availabilityzones/results.go

+// UnmarshalJSON to override default
+func (r *ServerAvailabilityZoneExt) UnmarshalJSON(b []byte) error {
+	return nil
+}


This was removed sometime after #327 was opened. Can you remove this function?

Thank you for feedback!

@jtopjian is it okay to leave ComputeAvailabilityZonePage name instead OSAvailabilityZonePage ? The problem is that we've already used AvailabilityZonePage name in openstack/sharedfilesystems/v2/availabilityzones/results.go.

You can re-use the name AvailabilityZonePage. The other AvailabilityZonePage is in another Go package and, unless I'm mistaken, there won't be a name collision.

coveralls · 2018-01-04T13:47:50Z

Coverage increased (+0.06%) to 72.878% when pulling 15ebf51 on dstdfx:availability-zones-info into f05feab on gophercloud:master.

theopenlab-ci · 2018-01-04T14:07:01Z

Build succeeded.

gophercloud-unittest http://logs.openlabtesting.org/logs/04/704/15ebf51789dc5be22211291de843d8acc3835f28/check/gophercloud-unittest/aefb3fd/ : SUCCESS in 1m 13s
gophercloud-acceptance-test http://logs.openlabtesting.org/logs/04/704/15ebf51789dc5be22211291de843d8acc3835f28/check/gophercloud-acceptance-test/08593b6/ : SUCCESS in 25m 47s

* Change `AvailabilityZoneInfo` type from `struct` to `slice` * Add custom `UnmarshalJSON` to `AvailabilityZoneInfo` * Get rid of 'OS' prefixes in variable's names

coveralls · 2018-01-04T19:38:05Z

Coverage increased (+0.07%) to 72.885% when pulling d7a80ae on dstdfx:availability-zones-info into f05feab on gophercloud:master.

theopenlab-ci · 2018-01-04T20:12:38Z

Build succeeded.

gophercloud-unittest http://logs.openlabtesting.org/logs/04/704/d7a80ae5c3c922dc5c9f712eb11ef29bf53db4f3/check/gophercloud-unittest/241321c/ : SUCCESS in 1m 17s
gophercloud-acceptance-test http://logs.openlabtesting.org/logs/04/704/d7a80ae5c3c922dc5c9f712eb11ef29bf53db4f3/check/gophercloud-acceptance-test/73165aa/ : SUCCESS in 26m 36s

jtopjian

@dstdfx This is looking good - nice work.

I did a more detailed review and left some comments. Please let me know if you have any questions.

Additionally, would you be able to provide an acceptance test for this? The file would be contained under acceptance/openstack/compute/v2.

More information about acceptance tests can be found here: https://github.com/gophercloud/gophercloud/tree/master/acceptance

jtopjian · 2018-01-05T03:04:00Z

openstack/compute/v2/extensions/availabilityzones/doc.go


-	type ServerWithAZ struct {
-		servers.Server
-		availabilityzones.ServerAvailabilityZoneExt


This example should not be removed.

jtopjian · 2018-01-05T03:04:14Z

openstack/compute/v2/extensions/availabilityzones/doc.go


-	allPages, err := servers.List(client, nil).AllPages()
+	allPages, err := az.ListDetail(computeClient).AllPages()


az is not a valid package name.

jtopjian · 2018-01-05T03:05:45Z

openstack/compute/v2/extensions/availabilityzones/doc.go

+			fmt.Println(hostname)
+			// to be continued ...
+			// cut for brevity :)
+		}


The following should work here:

for _, zoneInfo := range availabilityZoneInfo { fmt.Printf("%+v\n", zoneInfo) }

jtopjian · 2018-01-05T03:06:36Z

openstack/compute/v2/extensions/availabilityzones/results.go

 type ServerAvailabilityZoneExt struct {
 	// AvailabilityZone is the availabilty zone the server is in.
 	AvailabilityZone string `json:"OS-EXT-AZ:availability_zone"`
 }
+
+type StateofService struct {


Let's change this to ServiceState.

jtopjian · 2018-01-05T03:32:43Z

openstack/compute/v2/extensions/availabilityzones/results.go

+	NovaScheduler   StateofService `json:"nova-scheduler"`
+	NovaCompute     StateofService `json:"nova-compute"`
+	NovaCert        StateofService `json:"nova-cert"`
+}


The above is hard-coding each of the nova services into Gophercloud. By doing this, each Host result will contain all of the above services even if they are not part of the availability zone. For example, this is the result of running this code in my test environment:

"hosts": { "openstack-acc-tests.novalocal": { "nova-conductor": { "active": false, "available": false, "updated_at": "0001-01-01T00:00:00Z" }, "nova-consoleauth": { "active": false, "available": false, "updated_at": "0001-01-01T00:00:00Z" }, "nova-network": { "active": false, "available": false, "updated_at": "0001-01-01T00:00:00Z" }, "nova-scheduler": { "active": false, "available": false, "updated_at": "0001-01-01T00:00:00Z" }, "nova-compute": { "active": true, "available": true, "updated_at": "2018-01-05T03:16:04Z" }, "nova-cert": { "active": false, "available": false, "updated_at": "0001-01-01T00:00:00Z" } } }, "zoneName": "nova", "zoneState": { "available": true } }

Instead of doing the above, I think we should do:

type Service map[string]ServiceState

When I modify this PR to be the above, I now see this:

"hosts": { "openstack-acc-tests.novalocal": { "nova-compute": { "active": true, "available": true, "updated_at": "2018-01-05T03:26:34Z" } } }, "zoneName": "nova", "zoneState": { "available": true } }

Which I think is more accurate.

Please let me know if I am incorrect about this.

jtopjian · 2018-01-05T03:33:18Z

openstack/compute/v2/extensions/availabilityzones/results.go

+// AvailabilityZone contains all the information associated with an OpenStack
+// AvailabilityZone.
+type AvailabilityZone struct {
+	Hosts `json:"hosts"`


Hosts Hosts `json:"hosts"`

jtopjian · 2018-01-05T03:33:34Z

openstack/compute/v2/extensions/availabilityzones/results.go

+	Hosts `json:"hosts"`
+	// The availability zone name
+	ZoneName  string `json:"zoneName"`
+	ZoneState `json:"zoneState"`


ZoneState ZoneState `json:"zoneState"`

jtopjian · 2018-01-05T03:38:04Z

openstack/compute/v2/extensions/availabilityzones/results.go

+}
+
+// AvailabilityZoneInfo contains list of AvailabilityZone info
+type AvailabilityZoneInfo []AvailabilityZone


This type isn't needed.

jtopjian · 2018-01-05T03:39:18Z

openstack/compute/v2/extensions/availabilityzones/results.go

+func ExtractAvailabilityZones(r pagination.Page) (AvailabilityZoneInfo, error) {
+	var a AvailabilityZoneInfo
+	err := (r.(AvailabilityZonePage)).ExtractInto(&a)
+	return a, err


Instead of the above, use:

var s struct { AvailabilityZoneInfo []AvailabilityZone `json:"availabilityZoneInfo` } err := (r.(AvailabilityZonePage)).ExtractInto(&s) return s.AvailabilityZoneInfo, err

* Rename `ServiceOfState` to `ServiceState` * Change type `Services` from `struct` to `map[string]ServiceState` * Delete useless type `AvailabilityZoneInfo` * Update doc.go

coveralls · 2018-01-05T10:31:07Z

Coverage increased (+0.08%) to 72.901% when pulling a4be04e on dstdfx:availability-zones-info into f05feab on gophercloud:master.

theopenlab-ci · 2018-01-06T01:22:04Z

Build succeeded.

gophercloud-unittest http://logs.openlabtesting.org/logs/04/704/a4be04e79648867a5c4a34c37f821adc386b991d/check/gophercloud-unittest/70f2301/ : SUCCESS in 1m 44s
gophercloud-acceptance-test http://logs.openlabtesting.org/logs/04/704/a4be04e79648867a5c4a34c37f821adc386b991d/check/gophercloud-acceptance-test/9b00135/ : SUCCESS in 32m 06s

jtopjian

@dstdfx Just one more change. I apologize that I didn't catch this yesterday.

Once this change is made, this PR is good to go. Thank you very much for your work on it. 😄

jtopjian · 2018-01-06T03:11:19Z

openstack/compute/v2/extensions/availabilityzones/results.go

+type ServiceState struct {
+	Active    bool      `json:"active"`
+	Available bool      `json:"available"`
+	UpdatedAt time.Time `json:"updated_at"`


UpdatedAt time.Time `json:"-"`

My bad, useless struct tag!

coveralls · 2018-01-06T11:17:08Z

Coverage increased (+0.08%) to 72.901% when pulling e122ead on dstdfx:availability-zones-info into f05feab on gophercloud:master.

theopenlab-ci · 2018-01-06T11:45:25Z

Build succeeded.

gophercloud-unittest http://logs.openlabtesting.org/logs/04/704/e122ead141271acf4787f3f999f63e510e716934/check/gophercloud-unittest/cc355dd/ : SUCCESS in 1m 15s
gophercloud-acceptance-test http://logs.openlabtesting.org/logs/04/704/e122ead141271acf4787f3f999f63e510e716934/check/gophercloud-acceptance-test/21a71e9/ : SUCCESS in 24m 49s

jtopjian · 2018-01-08T04:04:54Z

@dstdfx This looks good to me. I've also verified it on a new Pike environment and everything is parsing correctly.

Nice work!

Availability zones info support (gophercloud#320)

a430976

Add ability to get list and detailed AZs info

Style fixes

a3e8e09

dstdfx force-pushed the availability-zones-info branch from bfb4a53 to a3e8e09 Compare January 3, 2018 19:49

jtopjian requested changes Jan 4, 2018

View reviewed changes

dstdfx added 2 commits January 4, 2018 16:20

Change test fixture to proper

dfd61ad

Delete useless method of ServerAvailabilityZoneExt

15ebf51

dstdfx force-pushed the availability-zones-info branch from ed8c33a to 15ebf51 Compare January 4, 2018 13:40

dstdfx added 2 commits January 4, 2018 22:07

Code improvements

c8d078d

* Change `AvailabilityZoneInfo` type from `struct` to `slice` * Add custom `UnmarshalJSON` to `AvailabilityZoneInfo` * Get rid of 'OS' prefixes in variable's names

Update doc.go

d7a80ae

jtopjian requested changes Jan 5, 2018

View reviewed changes

dstdfx added 2 commits January 5, 2018 12:23

Code refactoring

62f2efa

* Rename `ServiceOfState` to `ServiceState` * Change type `Services` from `struct` to `map[string]ServiceState` * Delete useless type `AvailabilityZoneInfo` * Update doc.go

Acceptance tests for gophercloud#704

a4be04e

jtopjian requested changes Jan 6, 2018

View reviewed changes

Change struct tag updated_at to -

e122ead

jtopjian approved these changes Jan 8, 2018

View reviewed changes

jtopjian merged commit b2bf8a6 into gophercloud:master Jan 8, 2018

jtopjian mentioned this pull request Jan 8, 2018

compute: Lists and gets detailed availability zone information #327

Closed

dstdfx deleted the availability-zones-info branch January 8, 2018 10:05

jtopjian mentioned this pull request Jan 10, 2018

listing region availability zones? #320

Closed

dstdfx changed the title ~~Compute availability zones info support~~ Compute v2: Availability Zones Info Support Jan 17, 2018

dstdfx changed the title ~~Compute v2: Availability Zones Info Support~~ Compute v2: Availability Zones Info support Jan 17, 2018

dstdfx changed the title ~~Compute v2: Availability Zones Info support~~ Compute v2: Availability Zones Info Jan 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compute v2: Availability Zones Info #704

Compute v2: Availability Zones Info #704

dstdfx commented Jan 3, 2018

theopenlab-ci bot commented Jan 3, 2018

coveralls commented Jan 3, 2018 •

edited

Loading

theopenlab-ci bot commented Jan 3, 2018

jtopjian left a comment

jtopjian Jan 4, 2018

dstdfx Jan 4, 2018

dstdfx Jan 4, 2018

dstdfx Jan 4, 2018

jtopjian Jan 4, 2018

coveralls commented Jan 4, 2018 •

edited

Loading

theopenlab-ci bot commented Jan 4, 2018

coveralls commented Jan 4, 2018 •

edited

Loading

theopenlab-ci bot commented Jan 4, 2018

jtopjian left a comment

jtopjian Jan 5, 2018

jtopjian Jan 5, 2018

jtopjian Jan 5, 2018

jtopjian Jan 5, 2018

jtopjian Jan 5, 2018

jtopjian Jan 5, 2018

jtopjian Jan 5, 2018

jtopjian Jan 5, 2018

jtopjian Jan 5, 2018

coveralls commented Jan 5, 2018 •

edited

Loading

theopenlab-ci bot commented Jan 6, 2018

jtopjian left a comment •

edited

Loading

jtopjian Jan 6, 2018

dstdfx Jan 6, 2018

coveralls commented Jan 6, 2018 •

edited

Loading

theopenlab-ci bot commented Jan 6, 2018

jtopjian commented Jan 8, 2018


		allPages, err := servers.List(client, nil).AllPages()
		allPages, err := az.ListDetail(computeClient).AllPages()

Compute v2: Availability Zones Info #704

Compute v2: Availability Zones Info #704

Conversation

dstdfx commented Jan 3, 2018

theopenlab-ci bot commented Jan 3, 2018

coveralls commented Jan 3, 2018 • edited Loading

theopenlab-ci bot commented Jan 3, 2018

jtopjian left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 4, 2018 • edited Loading

theopenlab-ci bot commented Jan 4, 2018

coveralls commented Jan 4, 2018 • edited Loading

theopenlab-ci bot commented Jan 4, 2018

jtopjian left a 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

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

Choose a reason for hiding this comment

coveralls commented Jan 5, 2018 • edited Loading

theopenlab-ci bot commented Jan 6, 2018

jtopjian left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 6, 2018 • edited Loading

theopenlab-ci bot commented Jan 6, 2018

jtopjian commented Jan 8, 2018

coveralls commented Jan 3, 2018 •

edited

Loading

coveralls commented Jan 4, 2018 •

edited

Loading

coveralls commented Jan 4, 2018 •

edited

Loading

coveralls commented Jan 5, 2018 •

edited

Loading

jtopjian left a comment •

edited

Loading

coveralls commented Jan 6, 2018 •

edited

Loading