Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

centralize resource allocation #23

Merged
merged 2 commits into from
Mar 6, 2015
+1,416 −568
Merged

centralize resource allocation #23

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
35377a7
centralize resource allocation
Mar 3, 2015
1aa88a0
create master state, publish driver state with master's operational p…
Mar 5, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
220 changes: 8 additions & 212 deletions drivers/ovsdriver.go
View file
Original file line number Diff line number Diff line change
@@ -24,8 +24,6 @@ import (

"github.com/contiv/libovsdb"
"github.com/contiv/netplugin/core"
"github.com/contiv/netplugin/gstate"
"github.com/contiv/netplugin/netutils"
)

// implements the NetworkDriver and EndpointDriver interface for an vlan based
@@ -238,7 +236,7 @@ func (d *OvsDriver) createDeletePort(portName, intfName, intfType, id string,
}

if intfOptions != nil {
log.Printf("using options %v \n", intfOptions)
// log.Printf("using options %v \n", intfOptions)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest to remove the commented code instead.

Copy link
Author

Choose a reason for hiding this comment

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

will do.

intf["options"], err = libovsdb.NewOvsMap(intfOptions)
if err != nil {
log.Printf("error '%s' creating options from %v \n", err, intfOptions)
@@ -426,169 +424,35 @@ func (d *OvsDriver) Deinit() {
}

func (d *OvsDriver) CreateNetwork(id string) error {
var err error
var extPktTag, pktTag uint
var subnetLen uint
var subnetIp string
var gOper gstate.Oper

operNwState := OvsOperNetworkState{StateDriver: d.stateDriver}
err = operNwState.Read(id)
if err == nil {
return err
}

cfgNetState := OvsCfgNetworkState{StateDriver: d.stateDriver}
err = cfgNetState.Read(id)
if err != nil {
return err
}

err = gOper.Read(d.stateDriver, cfgNetState.Tenant)
if err != nil {
return err
}

operNwState = OvsOperNetworkState{StateDriver: d.stateDriver,
Id: cfgNetState.Id, Tenant: cfgNetState.Tenant,
PktTagType: cfgNetState.PktTagType,
DefaultGw: cfgNetState.DefaultGw}

if cfgNetState.PktTag == "auto" {
operNwState.PktTagType = gOper.DefaultNetType
if gOper.DefaultNetType == "vlan" {
pktTag, err = gOper.AllocVlan()
if err != nil {
return err
}
} else if gOper.DefaultNetType == "vxlan" {
extPktTag, pktTag, err = gOper.AllocVxlan()
if err != nil {
return err
}
}

err = gOper.Update(d.stateDriver)
if err != nil {
return err
}

log.Printf("allocated vlan %d vxlan %d \n", pktTag, extPktTag)
operNwState.ExtPktTag = int(extPktTag)
operNwState.PktTag = int(pktTag)
} else {
if cfgNetState.PktTagType == "vxlan" {
pktTag, err = gOper.AllocLocalVlan()
if err != nil {
return err
}

err = gOper.Update(d.stateDriver)
if err != nil {
log.Printf("error updating the global state - %s \n", err)
return err
}

operNwState.PktTag = int(pktTag)
operNwState.ExtPktTag, err = strconv.Atoi(cfgNetState.PktTag)
} else if cfgNetState.PktTagType == "vlan" {
operNwState.PktTag, err = strconv.Atoi(cfgNetState.PktTag)
if err != nil {
return err
}
}
}

if cfgNetState.SubnetIp == "auto" {
subnetLen = gOper.AllocSubnetLen
subnetIp, err = gOper.AllocSubnet()
if err != nil {
return err
}

err = gOper.Update(d.stateDriver)
if err != nil {
return err
}
log.Printf("allocated subnet %s/%d \n", subnetIp, subnetLen)
} else {
subnetLen = cfgNetState.SubnetLen
subnetIp = cfgNetState.SubnetIp
}
operNwState.SubnetIp = subnetIp
operNwState.SubnetLen = subnetLen

netutils.InitSubnetBitset(&operNwState.IpAllocMap, subnetLen)
err = operNwState.Write()
err := cfgNetState.Read(id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need a read operation here, since the CreateNetwork() is a noop? I am thinking why add extra error paths if we can live without them.

I also see that we have stopped creating OvsNwOperState in this function altogether. It was mostly used to track number of endpoints in a network. It might be a useful stats to keep and maintain for debugging from netplugin perspective, albeit it is much coarser stats and needs multiple distributed instance of the netplugin to synchronize updates to OvsNwOperState (I was able to achieve the synchronization in my other branch 'deleteApiCleanup' without an explicit lock). But not sure if we loose much by removing this state. I think we can start off with current change as it keeps things simple.

Copy link
Author

Choose a reason for hiding this comment

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

The reason for read operation was to suggest that we can continue to perform any OVS operations based on that.
The reason for not updating OVsNwOperState is because OvsNwOperState is not a node specific event, rather something cluster wide. For that reason I also kept the EpCount in the NwOperState as updated by netmaster.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand NwOperState is cluster wide and EpCount was cluster wide (we can achieve it's update with serialized writes as I mentioned in my comment). I was just mentioning it could be updated by Driver for debuggabilty. Example, it could be used to indicate some failure when master thinks they have placed X EPs in a network but the Driver state would indicate say (X'). I agree it's a coarser stats though and won't narrow down to failing minion but still indicates some failure.

Also, looks like we will be getting rid of OvsNwOperState for now based on rest of the comments so not a concern right now 😉

if err != nil {
log.Printf("Failed to read net %s \n", cfgNetState.Id)
return err
}
log.Printf("create net %s \n", cfgNetState.Id)

return nil
}

func (d *OvsDriver) DeleteNetwork(value string) error {

// no driver operation for network delete
var err error
var gOper gstate.Oper

cfgNetState := OvsCfgNetworkState{}
err = cfgNetState.Unmarshal(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest to remove the dependence on the 'value' being passed in this function since it is going to be a noop. This will take us closer to cleaning up the Delete API to revert them to take keys instead, which I am trying to get to in my deleteApiCleanup branch.

Copy link
Author

Choose a reason for hiding this comment

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

I haven't changed this code of passing the value; rather my change is removing gOper reference from it. I'd expect this to be cleaned up as part of your changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, will do.

if err != nil {
log.Printf("Failed to unmarshal network config, err '%s' \n", err)
return err
}

operNwState := OvsOperNetworkState{StateDriver: d.stateDriver}
err = operNwState.Read(cfgNetState.Id)
if err != nil {
return core.ErrIfKeyExists(err)
}

err = gOper.Read(d.stateDriver, operNwState.Tenant)
if err != nil {
return err
}

if operNwState.PktTagType == "vlan" {
err = gOper.FreeVlan(uint(operNwState.PktTag))
if err != nil {
return err
}
} else if operNwState.PktTagType == "vxlan" {
err = gOper.FreeVxlan(uint(operNwState.ExtPktTag),
uint(operNwState.PktTag))
if err != nil {
return err
}
}

if cfgNetState.SubnetIp == "auto" {
log.Printf("freeing subnet %s/%d \n",
operNwState.SubnetIp, operNwState.SubnetLen)
err = gOper.FreeSubnet(operNwState.SubnetIp)
if err != nil {
log.Printf("error '%s' freeing the subnet \n", err)
}
}

err = gOper.Update(d.stateDriver)
if err != nil {
log.Printf("error updating the global state - %s \n", err)
return err
}

err = operNwState.Clear()
if err != nil {
return core.ErrIfKeyExists(err)
}
log.Printf("delete net %s \n", cfgNetState.Id)

return nil
}

func (d *OvsDriver) CreateEndpoint(id string) error {
var err error
var ipAddrBit uint = 0
var found bool

// add an internal ovs port with vlan-tag information from the state
portName := d.getPortName()
@@ -621,14 +485,6 @@ func (d *OvsDriver) CreateEndpoint(id string) error {
intfType = ""
}

cfgNetState := OvsCfgNetworkState{StateDriver: d.stateDriver}
err = cfgNetState.Read(epCfg.NetId)
if err != nil {
return err
}

// TODO: use etcd distributed lock to ensure atomicity of this operation
// this is valid for EpCount, defer the unlocking for intermediate returns
operNwState := OvsOperNetworkState{StateDriver: d.stateDriver}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not be using OvsOperNetworkState but rather OvsConfigNetworkState, since oper state is owned and created by the (ovs-)driver (if it needs to), while the config state is owned by the management function and is just consumed by the driver.

Just to clarify, I am talking something different wrt intent v/s realized-intent. I see each module/interface (driver; resource allocator etc) to own their own config(consumed) and oper(generated) states. One module's oper state may become other module's config state (assuming no need for state translation). This helps keep state boundaries clear and provide flexibility of reuse of module's functionality.

Example 1, netmaster can consume MasterNwConfig and generate OvsNwConfig or ACINwConfig based on state translation function.

Example 2, I will be able to mix IP-allocator with ovs as well as Linux-bridge driver, if I define IP-allocator's own config and oper state and then translate it's oper state to config state of underlying driver). This is inline with what we put in the design document around intermixing independent functionalities through state and state translation.

err = operNwState.Read(epCfg.NetId)
if err != nil {
@@ -647,52 +503,13 @@ func (d *OvsDriver) CreateEndpoint(id string) error {
}
}()

ipAddress := epCfg.IpAddress
if ipAddress == "auto" {
if ipAddrBit, found = operNwState.IpAllocMap.NextClear(0); !found {
log.Printf("auto allocation failed - address exhaustion in subnet %s/%d \n",
operNwState.SubnetIp, operNwState.SubnetLen)
return err
}
ipAddress, err = netutils.GetSubnetIp(
operNwState.SubnetIp, operNwState.SubnetLen, 32, ipAddrBit)
if err != nil {
log.Printf("error acquiring subnet ip '%s' \n", err)
return err
}
log.Printf("Ep %s was allocated ip address %s \n", id, ipAddress)
} else if ipAddress != "" && operNwState.SubnetIp != "" {
ipAddrBit, err = netutils.GetIpNumber(
operNwState.SubnetIp, operNwState.SubnetLen, 32, ipAddress)
if err != nil {
log.Printf("error getting host id from hostIp %s Subnet %s/%d err '%s'\n",
ipAddress, operNwState.SubnetIp, operNwState.SubnetLen, err)
return err
}
}
operNwState.IpAllocMap.Set(ipAddrBit)

// deprecate - bitset.WordCount gives the following value
operNwState.EpCount += 1
err = operNwState.Write()
if err != nil {
return err
}
defer func() {
if err != nil {
//XXX: need to return allocated ip back to the pool in case of failure
operNwState.EpCount -= 1
operNwState.Write()
}
}()

operEpState := OvsOperEndpointState{
StateDriver: d.stateDriver,
Id: id,
PortName: portName,
NetId: epCfg.NetId,
ContName: epCfg.ContName,
IpAddress: ipAddress,
IpAddress: epCfg.IpAddress,
IntfName: intfName,
HomingHost: epCfg.HomingHost,
VtepIp: epCfg.VtepIp}
@@ -752,27 +569,6 @@ func (d *OvsDriver) DeleteEndpoint(value string) (err error) {
return err
}

operNwState := OvsOperNetworkState{StateDriver: d.stateDriver}
err = operNwState.Read(epOper.NetId)
if err != nil {
return err
}

ipAddrBit, err := netutils.GetIpNumber(
operNwState.SubnetIp, operNwState.SubnetLen, 32, epOper.IpAddress)
if err != nil {
log.Printf("error getting host id from %s subnet %s/%d err '%s'\n",
epOper.IpAddress, operNwState.SubnetIp, operNwState.SubnetLen, err)
} else {
operNwState.IpAllocMap.Clear(ipAddrBit)
}

operNwState.EpCount -= 1
err = operNwState.Write()
if err != nil {
return err
}

return nil
}

27 changes: 22 additions & 5 deletions gstate/gstate.go
View file
Original file line number Diff line number Diff line change
@@ -262,12 +262,12 @@ func (g *Oper) AllocVxlan() (vxlan uint, localVlan uint, err error) {

func (g *Oper) FreeVxlan(vxlan uint, localVlan uint) error {
if !g.FreeLocalVlans.Test(localVlan) {
g.FreeLocalVlans.Clear(localVlan)
g.FreeLocalVlans.Set(localVlan)
}

vxlan = vxlan - g.FreeVxlansStart
if !g.FreeVxlans.Test(vxlan) {
g.FreeVxlans.Clear(vxlan)
g.FreeVxlans.Set(vxlan)
}

return nil
@@ -279,15 +279,15 @@ func (g *Oper) AllocLocalVlan() (uint, error) {
return 0, errors.New("no vlans available ")
}

g.FreeLocalVlans.Set(vlan)
g.FreeLocalVlans.Clear(vlan)

return vlan, nil
}

// be idempotent, don't complain if vlan is already freed
func (g *Oper) FreeLocalVlan(vlan uint) error {
if !g.FreeLocalVlans.Test(vlan) {
g.FreeLocalVlans.Clear(vlan)
g.FreeLocalVlans.Set(vlan)
}
return nil
}
@@ -333,11 +333,28 @@ func (g *Oper) AllocVlan() (uint, error) {
// be idempotent, don't complain if vlan is already freed
func (g *Oper) FreeVlan(vlan uint) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a minor nit. I was hoping we could remodel gstate into a resource allocator interface, so that we don't need to have explicit APIs like Set/FreeVlan, Set/FreeVxlan etc. A resource allocator interface can potentially look something like this:

type Resource interface {
    Alloc () interface{}, error
    Dealloc(interface{}) error
}

type ResourceAllocator interface {
    // initialize a resource like setup a IP-pool etc and add it to allocator
    DefResource (Resource) error
    // deinit a resource
    UndefResource(Resource) error
    // allocate the resource in logically centralized manner, eventually calls Resource.Alloc()
    AllocResource(Resource) interface{}, error
    // de-allocate the resource in logically centralized manner, eventually calls Resource.Dealloc()
    DeallocResource(Resource, interface{}) error
}

I just wanted to point this out so that we are on same page wrt resource-allocation design but don't need to change it now. Can safely be addressed later in case we start identifying more resources to deal with.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for putting out the resource allocation abstraction API; Will do it as a separate change, but a generic nature of interface makes a lot of sense to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, thanks Vipin.

if !g.FreeVlans.Test(vlan) {
g.FreeVlans.Clear(vlan)
g.FreeVlans.Set(vlan)
}
return nil
}

func (g *Oper) CheckVlanInUse(vlan uint) error {
if !g.FreeVlans.Test(vlan) {
return errors.New("specified vlan not available")
}

return nil
}

func (g *Oper) SetVlan(vlan uint) (err error) {
err = g.CheckVlanInUse(vlan)
if err == nil {
g.FreeVlans.Clear(vlan)
}

return
}

func (g *Oper) AllocSubnet() (string, error) {

subnetId, found := g.FreeSubnets.NextSet(0)
30 changes: 30 additions & 0 deletions gstate/gstate_test.go
View file
Original file line number Diff line number Diff line change
@@ -99,6 +99,21 @@ func TestGlobalConfigSpecificVlans(t *testing.T) {
t.Fatalf("error - expecting vlan %d but allocated %d \n", 200, vlan)
}

err = g.CheckVlanInUse(vlan)
if err == nil {
t.Fatalf("error - vlan %d should have returned in use \n", vlan)
}

err = g.SetVlan(vlan + 1)
if err != nil {
t.Fatalf("error - vlan %d should be allowed for allocation \n", vlan+1)
}

err = g.FreeVlan(vlan + 1)
if err != nil {
t.Fatalf("error freeing allocated vlan %d - err '%s' \n", vlan, err)
}

err = g.FreeVlan(vlan)
if err != nil {
t.Fatalf("error freeing allocated vlan %d - err '%s' \n", vlan, err)
@@ -257,6 +272,21 @@ func TestGlobalConfigDefaultVxlanWithVlans(t *testing.T) {
t.Fatalf("error - invalid vlan allocated %d \n", localVlan)
}

err = g.CheckVlanInUse(vlan)
if err == nil {
t.Fatalf("error - vlan %d should have returned in use \n", vlan)
}

err = g.SetVlan(vlan + 1)
if err != nil {
t.Fatalf("error - vlan %d should be allowed for allocation \n", vlan+1)
}

err = g.FreeVlan(vlan + 1)
if err != nil {
t.Fatalf("error freeing allocated vlan %d - err '%s' \n", vlan, err)
}

err = g.FreeVlan(vlan)
if err != nil {
t.Fatalf("error freeing allocated vlan %d - err '%s' \n", vlan, err)
Loading