Cinder Storage Pools API by spjmurray · Pull Request #205 · gophercloud/gophercloud

spjmurray · 2016-11-25T09:58:24Z

Addresses issue #194. The documentation is a little hazy links to code in the issue discussion thread.

coveralls · 2016-11-25T10:04:50Z

Coverage decreased (-0.07%) to 68.732% when pulling af51c86 on spjmurray:blockstorage_pool_stats into d5eda97 on gophercloud:master.

jtopjian · 2016-11-25T15:45:09Z

+		return err
+	}
+
+	// Should be a numeric, "unknown", "infinite"


Maybe add unit tests for the unknown and infinite cases?

jtopjian · 2016-11-25T15:46:47Z

A few inline comments. In addition, can you run the code through gofmt? If you're a vi user, the vim-go plugin will take care of that :)

jtopjian · 2016-11-25T15:52:02Z

+	Capabilities `json:"capabilities"`
+}
+
+func (s *StoragePool) UnmarshalJSON(b []byte) error {


Style nit: this should be (r *StoragePool)

jtopjian · 2016-11-25T15:52:21Z

+func (s *StoragePool) UnmarshalJSON(b []byte) error {
+	// Unmarshal the generic stuff
+	type tmp StoragePool
+	var p *struct {


Style nit: this should be var s

spjmurray · 2016-11-29T15:20:54Z

@jtopjian it's almost like you knew adding those unit tests would throw up an error ... good catch :)

coveralls · 2016-11-29T15:22:48Z

Coverage increased (+0.04%) to 68.836% when pulling 9f0e2b0 on spjmurray:blockstorage_pool_stats into d5eda97 on gophercloud:master.

jrperritt · 2017-03-10T02:04:45Z

@spjmurray still correctly titled [wip] or ready for review?

Adds in the ability to probe storage backends to get the current and maximum pool utilization. Useful for capacity planning based on committed-to metrics and those as a result of thin-provisioning.

Additionally makes the distinction between required and optional fields based on said documentation, adding in the optional fields

coveralls · 2017-06-02T11:54:13Z

Coverage increased (+0.07%) to 70.074% when pulling 970fc29 on spjmurray:blockstorage_pool_stats into 11fd706 on gophercloud:master.

jtopjian

@spjmurray The bulk of this is really good, so thank you for putting it together :)

A few notes:

The UnmarshalJSON stuff needed fixing up. It was easier for me to just make a diff than try to explain it inline: jtopjian@8ce30e3

I have one question for @jrperritt inline.

Additionally, if you do:

cinder list-extensions

You'll see Scheduler_stats listed, so I think this should be renamed to schedulerstats and placed under the extensions directory.

Edit: One more thing: I think the List call needs to be paginated, even if everything will be in a single page. Just like how your hypervisor stats support was implemented.

Let me know if you have any questions.

jtopjian · 2017-06-03T02:36:07Z

+			case "infinite":
+				r.Capabilities.FreeCapacityGB = math.Inf(1)
+			default:
+				return fmt.Errorf("capabilities.free_capacity_gb: unexpected string %v", keyword)


@jrperritt Would it be better to set the value to 0.0 or return an error?

I think you're right: setting it to 0.0 seems reasonable.

jrperritt · 2017-06-03T02:59:53Z

+			case "infinite":
+				r.Capabilities.FreeCapacityGB = math.Inf(1)
+			default:
+				return fmt.Errorf("capabilities.free_capacity_gb: unexpected string %v", keyword)


I think you're right: setting it to 0.0 seems reasonable.

jrperritt · 2017-06-03T03:00:34Z

+}
+
+// List makes a request against the API to list hypervisors.
+func List(client *gophercloud.ServiceClient, opts ListOptsBuilder) (r ListResult) {


We'll want to paginate this, even it's a single-page type.

jrperritt · 2017-06-03T03:00:57Z

+	// Optional Fields
+	ReservedPercentage       int64   `json:"reserved_percentage"`
+	LocationInfo             string  `json:"location_info"`
+	QoSSupprt                bool    `json:"QoS_support"`


QoSSupport

jrperritt · 2017-06-03T03:02:11Z

+	ReservedPercentage       int64   `json:"reserved_percentage"`
+	LocationInfo             string  `json:"location_info"`
+	QoSSupprt                bool    `json:"QoS_support"`
+	ProvisonedCapacityGB     float64 `json:"provisoned_capacity_gb"`


Haven't looked at the schema, but I suspect these should be "Provisioned" (and "provisioned")

jrperritt · 2017-06-03T03:03:33Z

+
+type StoragePool struct {
+	Name         string `json:"name"`
+	Capabilities `json:"capabilities"`


Capabilities Capabilities `json:"capabilities"`

jrperritt · 2017-06-03T03:04:37Z

+		case float64:
+			r.Capabilities.FreeCapacityGB = q.Capabilities.FreeCapacityGB.(float64)
+		case int:
+			r.Capabilities.FreeCapacityGB = q.Capabilities.FreeCapacityGB.(float64)


That will panic. Type-assert to int, then cast to float64. Better would be writing a unit test for it.

jrperritt · 2017-06-03T03:06:03Z

+		case float64:
+			r.Capabilities.TotalCapacityGB = q.Capabilities.TotalCapacityGB.(float64)
+		case int:
+			r.Capabilities.TotalCapacityGB = q.Capabilities.TotalCapacityGB.(float64)


Same here as above (type-assert to int, then cast to float64).

Cleans up wine-induced typos. Updated the unmarshall code, seems the integer case is unnecessarily as go when unmarshalling a number defaults to float64, so has been deleted. As we default to 0.0 for total and free capacities I've folded the unknown case into the default. Testing code has been updated to include an 'integer' in the JSON to ensure the assumptions hold.

Rather than unmarshal the entire storage pool struct we only need cater for the capabilities structure, the JSON unmarshalling code auto-magically takes care of invoking the correct routines for us.

coveralls · 2017-06-05T10:12:11Z

Coverage increased (+0.2%) to 70.165% when pulling 5a6d3cc on spjmurray:blockstorage_pool_stats into 11fd706 on gophercloud:master.

Renamed to scheduler stats, relocated to v2 extension as it's not part of v1.

coveralls · 2017-06-05T10:28:11Z

Coverage increased (+0.2%) to 70.165% when pulling c8f2a5d on spjmurray:blockstorage_pool_stats into 11fd706 on gophercloud:master.

spjmurray · 2017-06-05T10:43:25Z

@jtopjian @jrperritt Should be much better now...

jtopjian

@spjmurray Thanks for making all of these changes.

I left a comment about the remaining default switch clauses.

In addition, can you move this to blockstorage/extensions rather than blockstorage/v2/extensions? This works in both v1 and v2 (and I suspect v3).

Points for blaming wine on the typos :)

jtopjian · 2017-06-07T02:52:45Z

+				r.FreeCapacityGB = 0.0
+			}
+		default:
+			return fmt.Errorf("capabilities.free_capacity_gb: unexpected type %v", t)


I think all default actions should be to just set to 0.0 rather than return an error.

Makes all defaults silently return 0.0 if something is amiss. Also thought it best to share the duplicated code in the form of a separate function.

coveralls · 2017-06-07T10:25:29Z

Coverage increased (+0.6%) to 70.565% when pulling e799df0 on spjmurray:blockstorage_pool_stats into 11fd706 on gophercloud:master.

jtopjian · 2017-06-07T15:05:21Z

Thanks, Simon :)

jrperritt · 2017-06-07T16:27:59Z


+// Generic function to parse a capacity value which may be a numeric
+// value, "unknown", or "infinite"
+func ParseCapacity(capacity interface{}) float64 {


I'd prefer to just put this logic in UnmarshalJSON, but if you want to use a function, it should to be unexported (lowercase).

Unbeknownst to me symbol visibility across compilation units is controlled by capitalization, guess I'm too old and used to C's static :) This removes the export. Jrperritt preferred the functionality in the actual JSON code, I wanted to reduce code duplication so what I gather is the most idiomatic go is function literals/lambdas.

coveralls · 2017-06-08T09:17:55Z

Coverage increased (+0.6%) to 70.583% when pulling 34dcdd5 on spjmurray:blockstorage_pool_stats into 11fd706 on gophercloud:master.

* Cinder Storage Pools API Adds in the ability to probe storage backends to get the current and maximum pool utilization. Useful for capacity planning based on committed-to metrics and those as a result of thin-provisioning. * jtopjian's comments from 25/11/16 * Update field source documentation Additionally makes the distinction between required and optional fields based on said documentation, adding in the optional fields * Fix up CI header format failures * Move Storage Pools to Block Storage Extension * Jrperritt's Review Comments Cleans up wine-induced typos. Updated the unmarshall code, seems the integer case is unnecessarily as go when unmarshalling a number defaults to float64, so has been deleted. As we default to 0.0 for total and free capacities I've folded the unknown case into the default. Testing code has been updated to include an 'integer' in the JSON to ensure the assumptions hold. * Add Pagination * Tweak Unmarshalling Rather than unmarshal the entire storage pool struct we only need cater for the capabilities structure, the JSON unmarshalling code auto-magically takes care of invoking the correct routines for us. * Update Name and Location Renamed to scheduler stats, relocated to v2 extension as it's not part of v1. * Move Back To Generic Extension * Alter JSON Unmarshalling Makes all defaults silently return 0.0 if something is amiss. Also thought it best to share the duplicated code in the form of a separate function. * Remove Capacity Parser Global Unbeknownst to me symbol visibility across compilation units is controlled by capitalization, guess I'm too old and used to C's static :) This removes the export. Jrperritt preferred the functionality in the actual JSON code, I wanted to reduce code duplication so what I gather is the most idiomatic go is function literals/lambdas.

Acc Tests: Enable DNS test to run with other tests

jtopjian reviewed Nov 25, 2016

View reviewed changes

jtopjian mentioned this pull request May 21, 2017

Cinder Scheduler API #194

Closed

spjmurray added 3 commits June 2, 2017 12:26

Cinder Storage Pools API

df1d14f

Adds in the ability to probe storage backends to get the current and maximum pool utilization. Useful for capacity planning based on committed-to metrics and those as a result of thin-provisioning.

jtopjian's comments from 25/11/16

fa637bc

Update field source documentation

3c4a174

Additionally makes the distinction between required and optional fields based on said documentation, adding in the optional fields

spjmurray force-pushed the blockstorage_pool_stats branch from 9f0e2b0 to 3c4a174 Compare June 2, 2017 11:32

spjmurray changed the title ~~[wip] Cinder Storage Pools API~~ Cinder Storage Pools API Jun 2, 2017

Fix up CI header format failures

970fc29

jtopjian requested changes Jun 3, 2017

View reviewed changes

jrperritt suggested changes Jun 3, 2017

View reviewed changes

spjmurray added 4 commits June 5, 2017 09:43

Move Storage Pools to Block Storage Extension

77e981b

Add Pagination

420de13

Tweak Unmarshalling

5a6d3cc

Rather than unmarshal the entire storage pool struct we only need cater for the capabilities structure, the JSON unmarshalling code auto-magically takes care of invoking the correct routines for us.

Update Name and Location

c8f2a5d

Renamed to scheduler stats, relocated to v2 extension as it's not part of v1.

spjmurray mentioned this pull request Jun 5, 2017

OpenStack Input Plugin influxdata/telegraf#2013

Closed

jtopjian requested changes Jun 7, 2017

View reviewed changes

spjmurray added 2 commits June 7, 2017 11:09

Move Back To Generic Extension

3d1d557

Alter JSON Unmarshalling

e799df0

Makes all defaults silently return 0.0 if something is amiss. Also thought it best to share the duplicated code in the form of a separate function.

jtopjian approved these changes Jun 7, 2017

View reviewed changes

jrperritt reviewed Jun 7, 2017

View reviewed changes

jrperritt approved these changes Jun 8, 2017

View reviewed changes

jrperritt merged commit 36954b3 into gophercloud:master Jun 8, 2017

cardoe pushed a commit to cardoe/gophercloud that referenced this pull request Aug 27, 2020

Merge pull request gophercloud#205 from jtopjian/acc-enable-combined-dns

0cf15ab

Acc Tests: Enable DNS test to run with other tests

Conversation

spjmurray commented Nov 25, 2016

Uh oh!

coveralls commented Nov 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtopjian commented Nov 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spjmurray commented Nov 29, 2016

Uh oh!

coveralls commented Nov 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrperritt commented Mar 10, 2017

Uh oh!

coveralls commented Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtopjian left a comment • 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!

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!

coveralls commented Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spjmurray commented Jun 5, 2017

Uh oh!

jtopjian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtopjian commented Jun 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Nov 25, 2016 •

edited

Loading

jtopjian commented Nov 25, 2016 •

edited

Loading

coveralls commented Nov 29, 2016 •

edited

Loading

coveralls commented Jun 2, 2017 •

edited

Loading

jtopjian left a comment •

edited

Loading

coveralls commented Jun 5, 2017 •

edited

Loading

coveralls commented Jun 5, 2017 •

edited

Loading

coveralls commented Jun 7, 2017 •

edited

Loading

coveralls commented Jun 8, 2017 •

edited

Loading