Add ability to handle partition match with the machine allocation constraint map response. #75

blakerouse · 2018-09-13T13:38:36Z

MAAS 2.5 has added the ability for tags to match a partition and not just a block device when performing allocation with storage constraints.

This change allows this to on MAAS 2.5, while also not breaking versions before. This adds a new interface StorageDevice that can be either a partition or a block device.

…straint map response.

wallyworld

A few initial thoughts before lunch - I'm not sure of the need to change the map values from int to interface{}/int/float64

wallyworld · 2018-09-17T10:10:54Z

blockdevice.go

@@ -77,6 +89,14 @@ func (b *blockdevice) Size() uint64 {
 	return b.size
 }

+// FileSystem implements BlockDevice.
+func (b *blockdevice) FileSystem() FileSystem {
+	if b.filesystem == nil {


you can just return b.filesystem here
no need for if {}

wallyworld · 2018-09-17T10:15:48Z

blockdevice.go

 		"partitions": schema.List(schema.StringMap(schema.Any())),
 	}
-	checker := schema.FieldMap(fields, nil)
+	defaults := schema.Defaults{
+		"uuid": "",


Do you even need to specify a default given that above you have:
"uuid": schema.OneOf(schema.Nil(""), schema.String()),

eg model and id_path are processed the same was as uuid and don't need a default to be set

wallyworld · 2018-09-17T10:21:20Z

blockdevice.go

@@ -157,17 +182,26 @@ func blockdevice_2_0(source map[string]interface{}) (*blockdevice, error) {
 	// From here we know that the map returned from the schema coercion
 	// contains fields of the right type.

+	var filesystem *filesystem
+	if fsSource := valid["filesystem"]; fsSource != nil {


This would be better:

if fsSource, ok := valid["filesystem"].(map[string]interface{}); ok {
filesystem, err = filesystem2_0(fsSource)

wallyworld · 2018-09-17T10:23:51Z

blockdevice_test.go

@@ -41,6 +42,11 @@ func (*blockdeviceSuite) TestReadBlockDevices(c *gc.C) {
 	partition := partitions[0]
 	c.Check(partition.ID(), gc.Equals, 1)
 	c.Check(partition.UsedFor(), gc.Equals, "ext4 formatted filesystem mounted at /")
+
+	fs := blockdevice.FileSystem()
+	c.Assert(fs, gc.NotNil)


Can you add a check for the actual content

The Type() and MountPoint() are checked below? Is there something different your looking for?

Yeah, what's below does the job

wallyworld · 2018-09-17T10:24:37Z

blockdevice_test.go

@@ -89,7 +95,13 @@ var blockdevicesResponse = `
                "size": 8581545984
            }
        ],
-        "filesystem": null,
+        "filesystem": {


We should retain this "WithNulls" test and add a new one without

This diff is confusing I still have the filesystem is nil in the WithNulls. I added another check to ensure that filesystem stays nil in the WithNulls.

wallyworld · 2018-09-17T10:26:51Z

controller.go

 	}

 	if interfaceMatches, found := constraintsMap["interfaces"]; found {
 		matches := convertConstraintMatches(interfaceMatches)
 		for label, ids := range matches {
 			interfaces := make([]Interface, len(ids))
 			for index, id := range ids {
-				iface := machine.Interface(id)
+				iface := machine.Interface(id.(int))


This is dangerous given the change above to schema.Any() as it could panic
Why was the change to schema.Any() needed?

wallyworld · 2018-09-17T10:28:59Z

controller.go

-					return empty, NewDeserializationError("constraint match storage %q: %d does not match a block device for the machine", label, id)
+			storageDevices := make([]StorageDevice, len(ids))
+			for index, storageId := range ids {
+				if id, ok := storageId.(float64); ok {


Be very careful about type asserts like this as they will panic if the value is the wrong type.
We had int previously and now flot64 here. It seems like you are catering for a json marshall/unmarshal use case?
Maybe you want to use a cast float64(storageId)

I now understand that if we have a number it's a block device and a string is a partition. Can we add a comment to explain this concept.

wallyworld · 2018-09-18T14:01:30Z

blockdevice.go

 		"partitions": schema.List(schema.StringMap(schema.Any())),
 	}
-	checker := schema.FieldMap(fields, nil)
+	defaults := schema.Defaults{}


We can just revert to the previous code here

wallyworld · 2018-09-18T14:12:11Z

controller.go

@@ -931,15 +932,15 @@ func parseAllocateConstraintsResponse(source interface{}, machine *machine) (Con
 	constraintsMap := valid["constraints_by_type"].(map[string]interface{})
 	result := ConstraintMatches{
 		Interfaces: make(map[string][]Interface),
-		Storage:    make(map[string][]BlockDevice),
+		Storage:    make(map[string][]StorageDevice),
 	}

 	if interfaceMatches, found := constraintsMap["interfaces"]; found {
 		matches := convertConstraintMatches(interfaceMatches)


So we are processing the "interfaces" map here.
The "interfaces" schema defines a map of string-> int list.
We should have a version of convertConstraintMatches() which returns the correct type so we don't need to use id.(int)

wallyworld · 2018-09-18T14:45:01Z

controller.go

+				} else if id, ok := storageId.(string); ok {
+					// Should link to a partition.
+					const partPrefix = "partition:"
+					if strings.HasPrefix(id, partPrefix) {


To save indentation, do the check for the expected prefix and exit.
Then the rest of the stuff can be done without indenting

wallyworld · 2018-09-18T14:46:16Z

controller.go

 		}
 	}
 	return result, nil
 }

-func convertConstraintMatches(source interface{}) map[string][]int {
+func convertConstraintMatches(source interface{}) map[string][]interface{} {


As stated above, maybe 2 versions of this to return the expected typed map and error if the values don't conform

wallyworld · 2018-09-18T14:47:03Z

interfaces.go

@@ -234,6 +234,9 @@ type Machine interface {
 	// BlockDevice returns the block device for the machine that matches the
 	// id specified. If there is no match, nil is returned.
 	BlockDevice(id int) BlockDevice
+	// Partition returns the partition for the machine that matches the


nit pic - blank line before comments

wallyworld

Looking good, main issue is tweaking how the data is parsed to better handle the different type values

blakerouse · 2018-09-19T12:14:07Z

Thanks for the review.

I have updated the branch to cleanup the areas you called out. The Any and the .(int) casting has been removed for interfaces as request. Hopefully its good enough to land now, ;-)

wallyworld · 2018-09-19T13:49:12Z

$$merge$$

Veebers · 2018-09-19T14:40:26Z

!!build!!

Veebers · 2018-09-19T14:46:12Z

!!build!!

Veebers · 2018-09-19T14:52:12Z

!!build!!

Veebers · 2018-09-19T14:57:06Z

$$merge$$

Add ability to handle partition match with the machine allocation con…

fb4219d

…straint map response.

wallyworld reviewed Sep 17, 2018

View reviewed changes

Improve code from review.

9a70fcc

wallyworld reviewed Sep 18, 2018

View reviewed changes

Fixes from code review.

f688dd4

wallyworld approved these changes Sep 19, 2018

View reviewed changes

jujubot merged commit 8a8cec7 into juju:master Sep 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to handle partition match with the machine allocation constraint map response. #75

Add ability to handle partition match with the machine allocation constraint map response. #75

blakerouse commented Sep 13, 2018

wallyworld left a comment

wallyworld Sep 17, 2018

wallyworld Sep 17, 2018

wallyworld Sep 17, 2018

wallyworld Sep 17, 2018

blakerouse Sep 17, 2018

wallyworld Sep 18, 2018

wallyworld Sep 17, 2018

blakerouse Sep 17, 2018

wallyworld Sep 17, 2018

wallyworld Sep 17, 2018

wallyworld Sep 18, 2018

wallyworld Sep 18, 2018

wallyworld Sep 18, 2018

wallyworld Sep 18, 2018

wallyworld Sep 18, 2018

wallyworld Sep 18, 2018

wallyworld left a comment

blakerouse commented Sep 19, 2018

wallyworld commented Sep 19, 2018

Veebers commented Sep 19, 2018

Veebers commented Sep 19, 2018

Veebers commented Sep 19, 2018

Veebers commented Sep 19, 2018

Add ability to handle partition match with the machine allocation constraint map response. #75

Add ability to handle partition match with the machine allocation constraint map response. #75

Conversation

blakerouse commented Sep 13, 2018

wallyworld 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

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

wallyworld left a comment

Choose a reason for hiding this comment

blakerouse commented Sep 19, 2018

wallyworld commented Sep 19, 2018

Veebers commented Sep 19, 2018

Veebers commented Sep 19, 2018

Veebers commented Sep 19, 2018

Veebers commented Sep 19, 2018