Resource allocator interface changes #36
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| package core | ||
|
|
||
| type State interface { | ||
| Read(id string) error | ||
| ReadAll() ([]State, error) | ||
| Write() error | ||
| Clear() error | ||
| } | ||
|
|
||
| // CommonState defines the fields common to all core.State implementations. | ||
| // This struct shall be embedded as anonymous field in all structs that | ||
| // implement core.State | ||
| type CommonState struct { | ||
| StateDriver StateDriver `json:"-"` | ||
| Id string `json:"id"` | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| package drivers | ||
|
|
||
| import ( | ||
| "log" | ||
| "strings" | ||
|
|
||
| "github.com/contiv/netplugin/core" | ||
| ) | ||
|
|
||
| // The FakeStateDriver implements core.StateDriver interface for use with | ||
| // unit-tests | ||
|
|
||
| type ValueData struct { | ||
| value []byte | ||
| } | ||
|
|
||
| type FakeStateDriver struct { | ||
| TestState map[string]ValueData | ||
| } | ||
|
|
||
| func (d *FakeStateDriver) Init(config *core.Config) error { | ||
| d.TestState = make(map[string]ValueData) | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (d *FakeStateDriver) Deinit() { | ||
| d.TestState = nil | ||
| } | ||
|
|
||
| func (d *FakeStateDriver) Write(key string, value []byte) error { | ||
| val := ValueData{value: value} | ||
| d.TestState[key] = val | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (d *FakeStateDriver) Read(key string) ([]byte, error) { | ||
| if val, ok := d.TestState[key]; ok { | ||
| return val.value, nil | ||
| } | ||
|
|
||
| return []byte{}, &core.Error{Desc: "Key not found!"} | ||
| } | ||
|
|
||
| func (d *FakeStateDriver) ReadAll(baseKey string) ([][]byte, error) { | ||
| values := [][]byte{} | ||
|
|
||
| for key, val := range d.TestState { | ||
| if strings.Contains(key, baseKey) { | ||
| values = append(values, val.value) | ||
| } | ||
| } | ||
| return values, nil | ||
| } | ||
|
|
||
| func (d *FakeStateDriver) ClearState(key string) error { | ||
| if _, ok := d.TestState[key]; ok { | ||
| delete(d.TestState, key) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (d *FakeStateDriver) ReadState(key string, value core.State, | ||
| unmarshal func([]byte, interface{}) error) error { | ||
| encodedState, err := d.Read(key) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| err = unmarshal(encodedState, value) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (d *FakeStateDriver) ReadAllState(baseKey string, sType core.State, | ||
| unmarshal func([]byte, interface{}) error) ([]core.State, error) { | ||
| return ReadAllStateCommon(d, baseKey, sType, unmarshal) | ||
| } | ||
|
|
||
| func (d *FakeStateDriver) WriteState(key string, value core.State, | ||
| marshal func(interface{}) ([]byte, error)) error { | ||
| encodedState, err := marshal(value) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| err = d.Write(key, encodedState) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (d *FakeStateDriver) DumpState() { | ||
| for key, _ := range d.TestState { | ||
| log.Printf("key: %q\n", key) | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resource Interface:
State DriverandState Configduring Init() and then just call allocate/de-allocate?State Configcan define the path/hierarchy given to a specific resource to manage its own hierarchy there in. IfState Driveris passed duringInitthen we'd not needSetStateDriver()call.SetId()seem to suggest that I can allocate a resource first and then set its id, so then how do I key the resource in between? Should we just say that keep the Id must be specified duringNewResource()type of function?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if I understood. I model Resource interface as a (config) State along with associated logic for allocation and de-allocation. So in that sense it seem reasonable to reuse the same State interface instead of defining Read/Write methods of this own. Does the code seem to imply otherwise?
IMO moving knowledge about hierachy/path outside the State exposes the low level details of the state organization to the caller. This information is better kept inside the State. However, there are some common parts to all the State like Id and StateDriver, which I think can be moved to the State interface itself (as I also point to in my comment). I don't want to have Init() method for State since it seems similar to having a constructor for entire State which will differ between different implementations.
Since the resource get's defined through ResourceAllocator so this is avoided. However, I realize that same effect might appear when I move this method to the State interface. I think to make it more clear we can call it InitId(), InitStateDriver() instead.
Yes, that is one case. I think ReadAll is needed for all State(s) (even though we need to figure how to address it for other data stores). But use ReadAll kind of function else where as well. Example, we depend on ReadAll() for reading existing configurations to compute deltas etc. We can also use it for dumping (in pretty format) all system-state for debugging etc.
The way I have defined, each resource is defined independently and the information passed during it's definition shall be enough to initialize it. So, in this case the IP resource will require a Subnet to be provided in it's definition, which in turn will require that Subnet to have been allocated.
Yes, the ResourceAllocator shall be usable for both local and global resource. The difference really comes from which implementation is called where. Example, a local ResourceAllocator need not implement state synchronization algorithms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maintaining state is a natural effect of resource allocation/deallocation/init, etc. Then resource is merely an object that can be allocated, deallocated, updated, etc. The associated logic, its state state need not be made as part of the interface. However it needs to know the stateDriver therefore it can be passed during Init()
There is a logical hierarchy of various objects, now whether state interprets and maintains the same hierarchy can be left out of the logical hierarchy. Right now the key hierarchy is split among various files and no central place to know the central hierarchy. I 'd not be very concerned if we create the logical hierarchy using dir/file syntax or some other notation.
Sure. I was hoping that resource allocation can happen in a nested manner i.e. one resource allocation can trigger another resource to be allocated. For example if an load-balancer resource is allocated, it may automatically suggest allocating an IP address from the pool.
ok; let me ask the question differently. If I use vlanResource on the host for local vlan pool management, do I need a different implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to reuse the State as a underlying interface for whatever can hold state. This has certain benefits like it reuses a lot of interfaces without having to define new ones; it provides a uniform way to access the information in the system; it promotes a uniform (State based) design for the major constructs in our system i.e we think about our constructs as State and associated behaviors.
Are you concerned about overlapping paths between different states (as they are defined in different files) in etcd? I guess as of now that hasn't been an issue since the states we have defined have been for different functions like driver, resources etc and that might be the case going forward as well. In my view, the paths are not first order citizens (since we hide them in State). We should make sure that higher level functions that build on these states don't make assumptions on this hierarchy.
hmm, may be the definition of resource needs to be clarified. What does a load-balancer resource implies in your mind, is it just like an endpoint with IP address derived from a subnet pool allocated for it's network previously (at time of network definition) Or does it also involves auto defining a network and then grabbing an IP in that network? In either case there is a stage of defining the network and subnet allocation can occur at that point, right?
I think it depends on where the ResourceAllocator logic can be run to manage these vlans. If the local vlans are an independent set that can be managed locally then a different implementation of ResourceAllocator (running at netplugin level) might be needed. But if the vlanResource needs knowledge of centrally allocated vlans (as is the case for us today), then this ResourceAllocator will be same as the one for the cluster (i.e. it runs at netmaster level). An example of local/host-level resources that will need a different ResourceAllocator implementation (run at netplugin level) I can think of are network queues/buffers. Vxlan-only networking (i.e. no mixed mode at all) can potentially make use of a host-only ResourceAllocator as well for just allocating local-vlans per host but in that case I don't see benefit of having two implementations since single implementation can allocate same vlans on all hosts without having to worry about overlaps.