/juju

state: Multple interfaces can be added to machines; more tests/methods. #4481

Merged
merged 9 commits into from Feb 24, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@dimitern @dooferlad @jujubot
@dimitern
Contributor

dimitern commented Feb 19, 2016

Replaced singular Machine.AddInterface() by bulk Machine.AddInterfaces() method, which adds
multiple interfaces to the same machine, in a single transaction. Test coverage now is as good as
it can reasonably get, without resorting to ugly patching. Added jujutxn.TestHook tests and a speculative highly concurrent test for AddInterfaces.

New methods added and tested for all corner cases:

  • Machine.AllInterfaces()
  • Machine.RemoveAllInterfaces()
  • Interface.Remove()
  • Interface.Machine()

Defined two new concrete error types:

  • ErrProviderIDNotUnique, which will be used for Spaces and Subnets later.
  • ErrParentInterfaceHasChildren, returned when trying to remove an interface with children.

Still left to do in follow-ups:

  • Interface links state model - one or more links per interface, each having a unique ID and usable instead of a "naked" address across the board;
  • Machine.UpdateInterfaces() taking variable InterfaceArgs structs like AddInterfaces();
  • Removing interfaces on machine destruction;
  • Adding interfaces during at provisioning time;
  • Updating / adding interfaces to match what's observed by MA at startup;
  • more..
state/interfaces_test.go
+}
+
+func (s *interfacesStateSuite) TestInterfaceRefreshUpdatesStaleDocData(c *gc.C) {
+ fooInterface := s.addSimpleInterface(c)
@dooferlad

dooferlad Feb 22, 2016

Contributor

This test is unclear. Please add a comment telling us how fooInterface.Refresh() is able to change hardware address. It seems that fooInterface.Refresh is looking up some details by interface name.

@dimitern

dimitern Feb 23, 2016

Contributor

Indeed, now looking at it seems so. What it's trying to test is 2 copies of state.Interface with the same docID can hold different field values and Refresh() updates the doc and values. Unfortunately, there's no way to change anything of an interface - only add and remove. Other similar tests in state do things like: get original, copy with the same docID, copy.EnsureDead(), check original.Life() == Alive && copy.Life() == Dead, original.Refresh() && original.Life() == Dead. So this test "cheats" by removing the doc and adding a new one with the same docID but set hardware address and relies on Refresh() to get the new doc by ID. Too many assumptions, will simplify or just drop perhaps.
EDIT: Decided to drop Interface.Refresh() entirely along with its tests, as since nothing can change yet on the doc, there's no need for Refresh()ing.

state/interfaces_test.go
+ case successfulArgs <- testArgs:
+ default:
+ // successfulArgs is buffered, so if we can't send there was
+ // more than once success.
@dooferlad

dooferlad Feb 22, 2016

Contributor

s/once/one/

@dimitern

dimitern Feb 23, 2016

Contributor

Will fix.

state/interfaces_test.go
+ default:
+ // successfulArgs is buffered, so if we can't send there was
+ // more than once success.
+ c.Fatalf("unexpected more than one success for args %+v", testArgs)
@dooferlad

dooferlad Feb 22, 2016

Contributor

unexpected: more than one...

@dimitern

dimitern Feb 23, 2016

Contributor

Will do.

@dimitern
Contributor

dimitern commented Feb 23, 2016

Thanks for the detailed reviews! I'll start replying shortly in order.
@dimitern
Contributor

dimitern commented Feb 23, 2016

@frobware @dooferlad Updated as described and rebased onto maas-spaces2 tip, please have another look and see if it's good to land.
@dooferlad
Contributor

dooferlad commented Feb 23, 2016

LGTM
@dimitern
Contributor

dimitern commented Feb 24, 2016

$$merge$$
@jujubot
Contributor

jujubot commented Feb 24, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

jujubot added a commit that referenced this pull request Feb 24, 2016

@jujubot
Merge pull request #4481 from dimitern/maas-spaces2-state-interfaces2 
state: Multple interfaces can be added to machines; more tests/methods.

Replaced singular Machine.AddInterface() by bulk Machine.AddInterfaces() method, which adds
multiple interfaces to the same machine, in a single transaction. Test coverage now is as good as
it can reasonably get, without resorting to ugly patching. Added jujutxn.TestHook tests and a speculative highly concurrent test for AddInterfaces.

New methods added and tested for all corner cases:
- Machine.AllInterfaces()
- Machine.RemoveAllInterfaces()
- Interface.Remove()
- Interface.Machine()

Defined two new concrete error types:
- ErrProviderIDNotUnique, which will be used for Spaces and Subnets later.
- ErrParentInterfaceHasChildren, returned when trying to remove an interface with children.  

Still left to do in follow-ups:
- Interface links state model - one or more links per interface, each having a unique ID and usable instead of a "naked" address across the board; 
- Machine.UpdateInterfaces() taking variable InterfaceArgs structs like AddInterfaces();
- Removing interfaces on machine destruction;
- Adding interfaces during at provisioning time;
- Updating / adding interfaces to match what's observed by MA at startup;
- more..
d9da8d4

@jujubot jujubot merged commit d9da8d4 into juju:maas-spaces2 Feb 24, 2016

@dimitern dimitern deleted the dimitern:maas-spaces2-state-interfaces2 branch Feb 24, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment