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

app number free on underlay network #2285

Closed
wants to merge 1 commit into from
Closed

app number free on underlay network #2285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 8 additions & 10 deletions pkg/pillar/cmd/zedrouter/appnumonunet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,9 +255,8 @@ func appNumOnUNetBaseCreate(baseID uuid.UUID) *types.Bitmap {

// Delete the application number Base for a given UUID
func appNumOnUNetBaseDelete(ctx *zedrouterContext, baseID uuid.UUID) {
appNumMap := appNumOnUNetBaseGet(baseID)
if appNumMap == nil {
log.Fatalf("appNumOnUNetBaseDelete: non-existent")
if baseMap := appNumOnUNetBaseGet(baseID); baseMap == nil {
log.Fatalf("appNumOnUNetBaseDelete: non-existent map")
}
// check whether there are still some apps on
// this network
Expand All @@ -279,10 +278,9 @@ func appNumOnUNetBaseDelete(ctx *zedrouterContext, baseID uuid.UUID) {
}

// appNumOnUNetRefCount returns the number of (remaining) references to the network
func appNumOnUNetRefCount(ctx *zedrouterContext, networkID uuid.UUID) int {
appNumMap := appNumOnUNetBaseGet(networkID)
if appNumMap == nil {
log.Fatalf("appNumOnUNetRefCount: non map")
func appNumOnUNetRefCount(ctx *zedrouterContext, baseID uuid.UUID) int {
if baseMap := appNumOnUNetBaseGet(baseID); baseMap == nil {
log.Fatalf("appNumOnUNetRefCount: non-existent map")
}
pub := ctx.pubUUIDPairToNum
numType := appNumOnUNetType
Expand All @@ -293,12 +291,12 @@ func appNumOnUNetRefCount(ctx *zedrouterContext, networkID uuid.UUID) int {
if appNumMap.NumType != numType {
continue
}
if appNumMap.BaseID == networkID {
if appNumMap.BaseID == baseID {
log.Functionf("appNumOnUNetRefCount(%s): found: %v",
networkID, appNumMap)
baseID, appNumMap)
count++
}
}
log.Functionf("appNumOnUNetRefCount(%s) found %d", networkID, count)
log.Functionf("appNumOnUNetRefCount(%s) found %d", baseID, count)
return count
}
19 changes: 16 additions & 3 deletions pkg/pillar/cmd/zedrouter/ipaddronunet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"errors"
"fmt"
"github.com/lf-edge/eve/pkg/pillar/types"
uuid "github.com/satori/go.uuid"
)

func appNumsOnUNetAllocate(ctx *zedrouterContext,
Expand Down Expand Up @@ -44,9 +45,21 @@ func appNumsOnUNetFree(ctx *zedrouterContext,
ulStatus := &status.UnderlayNetworkList[ulNum]
networkID := ulStatus.Network
// release the app number
_, err := appNumOnUNetGet(ctx, networkID, appID)
if err == nil {
appNumOnUNetFree(ctx, networkID, appID)
appNumOnNetworkInstanceRelease(ctx, networkID, appID)
}
}

// frees an app number for network instance/ application number pair
// also tries to delete the referred network instance
func appNumOnNetworkInstanceRelease(ctx *zedrouterContext,
networkID uuid.UUID, appID uuid.UUID) {
if _, err := appNumOnUNetGet(ctx, networkID, appID); err == nil {
appNumOnUNetFree(ctx, networkID, appID)
}
netstatus := lookupNetworkInstanceStatus(ctx, networkID.String())
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to do this when you didn't call appNumOnUNetFree?

if netstatus != nil {
if maybeNetworkInstanceDelete(ctx, netstatus) {
log.Noticef("deleted network instance %s", netstatus.Key())
}
}
}
159 changes: 79 additions & 80 deletions pkg/pillar/cmd/zedrouter/zedrouter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -896,6 +896,7 @@ func handleAppNetworkCreate(ctxArg interface{}, key string, configArg interface{

// allocate application numbers on underlay network
if err := appNumsOnUNetAllocate(ctx, &config); err != nil {
status.PendingAdd = false
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bugfix. Please do as separate PR.

addError(ctx, &status, "handleAppNetworkCreate", err)
return
}
Expand Down Expand Up @@ -1486,67 +1487,30 @@ func handleAppNetworkModify(ctxArg interface{}, key string,
// We need to make sure we update any network instance references which
// have changed, but we need to do that when the status is not Activated,
// hence the order depends on the state.
if !config.Activate && status.Activated {
doInactivateAppNetwork(ctx, status)
checkAppNetworkModifyUNetAppNum(ctx, config, status)
appNetworkDoCopyNetworksToStatus(ctx, config, status)
} else if config.Activate && !status.Activated {
checkAppNetworkModifyUNetAppNum(ctx, config, status)
appNetworkDoCopyNetworksToStatus(ctx, config, status)
doActivate(ctx, config, status) // We check any error below
} else if !status.Activated {
if !status.Activated {
// need not handle ACL changes in deactivated state
Comment on lines +1490 to +1491
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how these changes make the code any easier to read and understand.

checkAppNetworkModifyUNetAppNum(ctx, config, status)
// Just copy in config
appNetworkDoCopyNetworksToStatus(ctx, config, status)
} else {
// during modify, copy the collect stats IP to status and
// check to see if need to launch the process
appCheckStatsCollect(ctx, &config, status)

// Check which underlays have changes while active
// that require bringing them down and up
for i := range config.UnderlayNetworkList {
ulNum := i + 1
log.Tracef("handleModify ulNum %d\n", ulNum)
ulConfig := &config.UnderlayNetworkList[i]
oldulConfig := &oldConfig.UnderlayNetworkList[i]
ulStatus := &status.UnderlayNetworkList[i]
if ulConfig.Network == ulStatus.Network &&
ulConfig.AppIPAddr.Equal(ulStatus.AppIPAddr) &&
ulConfig.AppMacAddr.String() == ulStatus.AppMacAddr.String() {
// Save new config then process any ACL changes
ulStatus.UnderlayNetworkConfig = *ulConfig
doAppNetworkModifyUNetAcls(ctx, status, ulConfig,
oldulConfig, ulStatus, ipsets, false)
continue
}
appNetworkDoInactivateUnderlayNetwork(ctx, status,
ulStatus, ipsets)

if ulConfig.Network != ulStatus.Network {
log.Functionf("checkAppNetworkModifyUNetAppNum(%v) for %s: change from %s to %s",
config.UUIDandVersion, config.DisplayName,
ulStatus.Network, ulConfig.Network)
// update the reference to the network instance
err := doAppNetworkModifyUNetAppNum(ctx,
status.UUIDandVersion.UUID,
ulConfig, ulStatus)
if err != nil {
log.Errorf("handleAppNetworkModify: AppNum failed for %s: %v",
config.DisplayName, err)
addError(ctx, status, "handleModify", err)
}
}
// Save new config
ulStatus.UnderlayNetworkConfig = *ulConfig

err := appNetworkDoActivateUnderlayNetwork(
ctx, config, status, ipsets, ulConfig, ulNum)
if err != nil {
// addError already done
log.Errorf("handleAppNetworkModify: Underlay Network activation failed for %s: %v",
config.DisplayName, err)
}
if config.Activate {
doActivate(ctx, config, status) // We check any error below
}
} else { // activated
if config.Activate {
// during modify, copy the collect stats IP to status and
// check to see if need to launch the process
appCheckStatsCollect(ctx, &config, status)

// handle any underlay specific change, may need to
// deactivate/activate a specific underlay network for
// network/addr changes
doAppNetworkModifyUnderlayNetworks(ctx, status,
config, oldConfig, ipsets, false)
} else {
// deactivate will clear ACLs also
doInactivateAppNetwork(ctx, status)
checkAppNetworkModifyUNetAppNum(ctx, config, status)
appNetworkDoCopyNetworksToStatus(ctx, config, status)
}
}

Expand Down Expand Up @@ -1621,6 +1585,59 @@ func doAppNetworkSanityCheckForModify(ctx *zedrouterContext,
return nil
}

// for activated/active case
func doAppNetworkModifyUnderlayNetworks(
ctx *zedrouterContext,
status *types.AppNetworkStatus,
config types.AppNetworkConfig,
oldConfig types.AppNetworkConfig,
ipsets []string, force bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The forice argument is unused. Are there other unused arguments?

for i := range config.UnderlayNetworkList {
ulNum := i + 1
log.Tracef("handleModify(%s), ulNum %d\n", config.DisplayName, ulNum)
ulConfig := &config.UnderlayNetworkList[i]
oldulConfig := &oldConfig.UnderlayNetworkList[i]
ulStatus := &status.UnderlayNetworkList[i]
if uuid.Equal(ulConfig.Network, ulStatus.Network) &&
ulConfig.AppIPAddr.Equal(ulStatus.AppIPAddr) &&
ulConfig.AppMacAddr.String() == ulStatus.AppMacAddr.String() {
// Save new config then process any ACL changes
ulStatus.UnderlayNetworkConfig = *ulConfig
doAppNetworkModifyUNetAcls(ctx, status, ulConfig,
oldulConfig, ulStatus, ipsets, false)
continue
}
appNetworkDoInactivateUnderlayNetwork(ctx, status,
ulStatus, ipsets)

//TBD:XXX may be also trigger app num changes for static/dynamic ipAddr
if !uuid.Equal(ulConfig.Network, ulStatus.Network) {
log.Functionf("doAppNetworkModifyUnderlayNetworks(%v) for %s: change from %s to %s",
config.UUIDandVersion, config.DisplayName,
ulStatus.Network, ulConfig.Network)
// update the reference to the network instance
err := doAppNetworkModifyUNetAppNum(ctx,
status.UUIDandVersion.UUID,
ulConfig, ulStatus)
if err != nil {
log.Errorf("handleAppNetworkModify: AppNum failed for %s: %v",
config.DisplayName, err)
addError(ctx, status, "handleModify", err)
}
}
// Save new config
ulStatus.UnderlayNetworkConfig = *ulConfig

err := appNetworkDoActivateUnderlayNetwork(
ctx, config, status, ipsets, ulConfig, ulNum)
if err != nil {
// addError already done
log.Errorf("handleAppNetworkModify: Underlay Network activation failed for %s: %v",
config.DisplayName, err)
}
}
}

func doAppNetworkModifyUNetAcls(
ctx *zedrouterContext,
status *types.AppNetworkStatus,
Expand Down Expand Up @@ -1719,9 +1736,7 @@ func doAppNetworkModifyUNetAppNum(
networkID := ulConfig.Network
oldNetworkID := ulStatus.Network
// release the app number on old network
if _, err := appNumOnUNetGet(ctx, oldNetworkID, appID); err == nil {
appNumOnUNetFree(ctx, oldNetworkID, appID)
}
appNumOnNetworkInstanceRelease(ctx, oldNetworkID, appID)
// allocate an app number on new network
isStatic := (ulConfig.AppIPAddr != nil)
if _, err := appNumOnUNetAllocate(ctx, networkID, appID,
Expand Down Expand Up @@ -1771,19 +1786,6 @@ func handleDelete(ctx *zedrouterContext, key string,

appNumFree(ctx, status.UUIDandVersion.UUID)
appNumsOnUNetFree(ctx, status)
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to explain to me the difference between the abstraction provided by ipaddronunet.go and appnumsonunet.go.
Presumably the former is layered on top of the later, in which case zedrouter.go should only call the former abstraction.

With your changes it is extremely hard to see where the freeing of various resources happen since there is no consistent abstraction. And if there is no such abstraction the easiest to read is the have the indivual calls at the top layer and not introduce intermediate functions; the functions should follow an intentional abstraction as supposed to just being collections of code.

// Did this free up any last references against any Network Instance Status?
for ulNum := 0; ulNum < len(status.UnderlayNetworkList); ulNum++ {
ulStatus := &status.UnderlayNetworkList[ulNum]
netstatus := lookupNetworkInstanceStatus(ctx, ulStatus.Network.String())
if netstatus != nil {
if maybeNetworkInstanceDelete(ctx, netstatus) {
log.Functionf("post appNumsOnUNetFree(%v) for %s deleted %s",
status.UUIDandVersion, status.DisplayName,
netstatus.Key())
}
}
}

log.Functionf("handleDelete done for %s\n", status.DisplayName)
}

Expand Down Expand Up @@ -1922,12 +1924,6 @@ func appNetworkDoInactivateUnderlayNetwork(

netstatus.RemoveVif(log, ulStatus.Vif)
publishNetworkInstanceStatus(ctx, netstatus)
if maybeNetworkInstanceDelete(ctx, netstatus) {
log.Noticef("deleted network instance %s", netstatus.Key())
} else {
// publish the changes to network instance status
publishNetworkInstanceStatus(ctx, netstatus)
}
Comment on lines -1925 to -1930
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you know this isn't required?

}

func pkillUserArgs(userName string, match string, printOnError bool) {
Expand Down Expand Up @@ -2358,6 +2354,9 @@ func checkAppNetworkErrorAndStartTimer(ctx *zedrouterContext) {
// When hit error while creating, set a timer for 60 sec and come back to retry
log.Functionf("checkAppNetworkErrorAndStartTimer: set timer\n")
ctx.appNetCreateTimer = time.NewTimer(60 * time.Second)
// may be multiple config entries, need not try scheduling again
// just return
return
Comment on lines +2357 to +2359
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this change do in this PR?

}
}

Expand Down