Skip to content
This repository has been archived by the owner on Jul 16, 2020. It is now read-only.

configuration: fix configure scheme #425

Merged
merged 4 commits into from
Aug 12, 2016
Merged

configuration: fix configure scheme #425

merged 4 commits into from
Aug 12, 2016

Conversation

mrkz
Copy link
Contributor

@mrkz mrkz commented Aug 1, 2016

Both compute_net and mgmt_net can only represent one
subnet. This commit changes the configure scheme for
compute_net and mgmt_net to accept a list of subnets.

both values should be now a YAML list of subnets like:

  launcher:
    compute_net: [192.168.1.110]
    mgmt_net: [192.168.1.0/24, 192.168.2.0/24]

or

  launcher:
    compute_net:
      - 192.168.1.110
    mgmt_net:
      - 192.168.1.0/24
      - 192.168.2.0/24

Fixes #232

Signed-off-by: Simental Magana, Marcos [email protected]

@mrkz
Copy link
Contributor Author

mrkz commented Aug 1, 2016

missing code coverage, created early PR for reviewing purposes

@coveralls
Copy link

coveralls commented Aug 2, 2016

Coverage Status

Coverage increased (+0.007%) to 64.714% when pulling 2f83fe8 on mrkz:fix_conf_scheme into dd98c89 on 01org:master.

@mrkz mrkz changed the title [DONOTMERGE] configuration: fix configure scheme [RFC] configuration: fix configure scheme Aug 2, 2016
@mrkz
Copy link
Contributor Author

mrkz commented Aug 2, 2016

There's a duplicated function ( equalNetSlice(slice1, slice2 []string) bool ) which is duplicated in payloads/configure_test.go and configuration/configuration_test.go as it wasn't clear enough where this could be placed to be used in both places, any suggestion here would be appreciated.

@mrkz
Copy link
Contributor Author

mrkz commented Aug 2, 2016

Also, please notice that by the nature of the Issue (#232) this PR fixes, the YAML schema is changed; some data which was previously a YAML string type will be now a YAML list type, breaking previous CIAO clusters setup, in order to fix it, both compute_net and mgmt_net values in yaml should be changed into a list format. e.g:

  • Previous PR format:
  launcher:
    compute_net: 192.168.1.110
    mgmt_net: 192.168.1.0/24
  • New PR format:
  launcher:
    compute_net:
    -  192.168.1.110
    mgmt_net:
    -  192.168.1.0/24

@@ -102,6 +107,36 @@ func TestBlobCorrectPayload(t *testing.T) {
testBlob(t, &payload, []byte(fullValidConf), true)
}

//equalNetSlice creates copies of slices received
// sort them and then check elements are equal
func equalNetSlice(slice1, slice2 []string) bool {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrkz So you want to compare if the 2 subnets slices are equal, right ?
2 options here:

  1. The order of the subnets matters, i.e [a, b] != [b, a] for us. In which case you can simply call reflect.DeepEqual(slice1, slice2)
  2. The order of the subnets does not matter, i.e. [a, b] and [b, a] are equivalent for us. In that case you can sort and copy the 2 slices and then call reflect.DeepEqual(sortedSlice1, sortedSlice2).

@mcastelino When building the mgmt and compute list of subnets for NetworkConfig, does the order of the subnets matter ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sameo so I sorted those slices to be able to compare slices element by element, will take a look at this approach. For other matters, I don't know if slices should be sorted or not, that's why I create copies of them and work with those copies instead of modifying the ones read from the configuration.

Other issue I have is having this func equalNetSlice function duplicated, any hint on where I could drop this in order to avoid code duplication?

@mcastelino
Copy link
Contributor

mcastelino commented Aug 4, 2016

@sameo the order of the subnets does not really matter. We scan all the network interfaces and see if they belong to one or more of the subnets. To run traffic we choose the first network interface that matched any of the subnets.

You do not need to check for overlaps. All you need to do is ensure they are valid CIDR subnets.

So the interface discovery order matters (not the subnet order).

@sameo
Copy link

sameo commented Aug 5, 2016

@mrkz So we're in case #2, where order does not matter. I think you can order them without copying, remove duplicates and then call reflect.DeepEqual(slice1, slice2) on them.

Since this really is a subnet comparison routine, it probably makes sense to have it exported from libsnnet.

@coveralls
Copy link

coveralls commented Aug 5, 2016

Coverage Status

Coverage decreased (-0.09%) to 65.265% when pulling 3f90027 on mrkz:fix_conf_scheme into 56930ea on 01org:master.

@mrkz mrkz changed the title [RFC] configuration: fix configure scheme configuration: fix configure scheme Aug 8, 2016
@coveralls
Copy link

coveralls commented Aug 8, 2016

Coverage Status

Coverage decreased (-0.07%) to 65.356% when pulling dcc21dc on mrkz:fix_conf_scheme into 981bb66 on 01org:master.

@mrkz mrkz changed the title configuration: fix configure scheme [DONOTMERGE] configuration: fix configure scheme Aug 8, 2016
if (slice1 == nil && slice2 != nil) ||
(slice2 == nil && slice1 != nil) ||
(len(slice1) != len(slice2)) {
equal = false
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return false

@mrkz mrkz changed the title [DONOTMERGE] configuration: fix configure scheme configuration: fix configure scheme Aug 9, 2016
@coveralls
Copy link

coveralls commented Aug 10, 2016

Coverage Status

Coverage decreased (-0.02%) to 65.439% when pulling db86e63 on mrkz:fix_conf_scheme into d6cef89 on 01org:master.

mrkz added 4 commits August 12, 2016 09:20
Both `compute_net` and `mgmt_net` can only represent one
subnet. This commit changes the configure scheme for
`compute_net` and `mgmt_net` to accept a list of subnets.

both values should be now a YAML list of subnets like:

  launcher:
    compute_net: [192.168.1.0/24]
    mgmt_net: [192.168.1.0/24, 192.168.2.0/24]

or

  launcher:
    compute_net:
      - 192.168.1.0/24
    mgmt_net:
      - 192.168.1.0/24
      - 192.168.2.0/24

Fixes #232

Signed-off-by: Simental Magana, Marcos <[email protected]>
Add `EqualNetSlice`, handy for checking slices of CIDR strings

Signed-off-by: Simental Magana, Marcos <[email protected]>
Previous commit (3ffdd0a) breaks tests as YAML
scheme was changed. this commit fix tests for the
configuration component.

Signed-off-by: Simental Magana, Marcos <[email protected]>
Previous commit (3ffdd0a) breaks tests as YAML
scheme was changed. this commit fix tests for the
payloads component.

Signed-off-by: Simental Magana, Marcos <[email protected]>
@coveralls
Copy link

coveralls commented Aug 12, 2016

Coverage Status

Coverage increased (+0.1%) to 65.561% when pulling c49e3af on mrkz:fix_conf_scheme into d6cef89 on 01org:master.

@sameo sameo merged commit d3c5489 into ciao-project:master Aug 12, 2016
@sameo sameo removed the in progress label Aug 12, 2016
@mrkz mrkz deleted the fix_conf_scheme branch September 19, 2016 16:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants