Skip to content

Commit

Permalink
Use 'defer' to simplify handling of AppNetworkStatus pending flags
Browse files Browse the repository at this point in the history
Currently, we have to remember to set PendingAdd/PendingModify inside
AppNetworkStatus to false just before the corresponding handler returns.
However, there are few error branches where we forgot to clear
the pending flag. This causes zedmanager to just wait and not report
the error published inside the AppNetworkStatus.
Instead of fixing this case-by-case, let's take advantage of 'defer'
to make sure that we will never forget to clear the Pending flag,
even if a new branch with return statement is added in the future.

Signed-off-by: Milan Lenco <milan@zededa.com>
(cherry picked from commit fbacc17)
  • Loading branch information
milan-zededa committed Sep 28, 2023
1 parent be13bbc commit 2476df9
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 22 deletions.
5 changes: 0 additions & 5 deletions pkg/pillar/cmd/zedrouter/appnetwork.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,6 @@ func (z *zedrouter) doActivateAppNetwork(config types.AppNetworkConfig,

// Update AppNetwork and NetworkInstance status.
status.Activated = true
status.PendingAdd = false
status.PendingModify = false
z.publishAppNetworkStatus(status)
z.updateNIStatusAfterAppNetworkActivate(status)

Expand Down Expand Up @@ -307,7 +305,6 @@ func (z *zedrouter) doUpdateActivatedAppNetwork(oldConfig, newConfig types.AppNe
// Update app network status as well as status of connected network instances.
z.processAppConnReconcileStatus(appConnRecStatus, status)
z.reloadStatusOfAssignedIPs(status)
status.PendingModify = false
z.publishAppNetworkStatus(status)
z.updateNIStatusAfterAppNetworkActivate(status)
}
Expand Down Expand Up @@ -335,8 +332,6 @@ func (z *zedrouter) doInactivateAppNetwork(config types.AppNetworkConfig,

// Update AppNetwork and NetworkInstance status.
status.Activated = false
status.PendingModify = false
status.PendingDelete = false
z.updateNIStatusAfterAppNetworkInactivate(status)
z.removeAssignedIPsFromAppNetStatus(status)
z.publishAppNetworkStatus(status)
Expand Down
29 changes: 12 additions & 17 deletions pkg/pillar/cmd/zedrouter/pubsubhandlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -439,15 +439,19 @@ func (z *zedrouter) handleAppNetworkCreate(ctxArg interface{}, key string,
// Start by marking with PendingAdd
status := types.AppNetworkStatus{
UUIDandVersion: config.UUIDandVersion,
PendingAdd: true,
DisplayName: config.DisplayName,
}
z.doCopyAppNetworkConfigToStatus(config, &status)
status.PendingAdd = true
z.publishAppNetworkStatus(&status)
defer func() {
status.PendingAdd = false
z.publishAppNetworkStatus(&status)
}()

if err := z.validateAppNetworkConfig(config); err != nil {
z.log.Errorf("handleAppNetworkCreate(%v): validation failed: %v",
config.UUIDandVersion.UUID, err)
status.PendingAdd = false
z.addAppNetworkError(&status, "handleAppNetworkCreate", err)
return
}
Expand All @@ -460,7 +464,6 @@ func (z *zedrouter) handleAppNetworkCreate(ctxArg interface{}, key string,
err = fmt.Errorf("failed to allocate appNum for %s/%s: %v",
config.UUIDandVersion.UUID, config.DisplayName, err)
z.log.Errorf("handleAppNetworkCreate(%v): %v", config.UUIDandVersion.UUID, err)
status.PendingAdd = false
z.addAppNetworkError(&status, "handleAppNetworkCreate", err)
return
}
Expand All @@ -474,7 +477,6 @@ func (z *zedrouter) handleAppNetworkCreate(ctxArg interface{}, key string,
err = fmt.Errorf("failed to allocate numbers for VIFs of the app %s/%s: %v",
config.UUIDandVersion.UUID, config.DisplayName, err)
z.log.Errorf("handleAppNetworkCreate(%v): %v", config.UUIDandVersion.UUID, err)
status.PendingAdd = false
z.addAppNetworkError(&status, "handleAppNetworkCreate", err)
return
}
Expand All @@ -485,20 +487,14 @@ func (z *zedrouter) handleAppNetworkCreate(ctxArg interface{}, key string,
if err != nil {
z.log.Errorf("handleAppNetworkCreate(%v): %v", config.UUIDandVersion.UUID, err)
status.AwaitNetworkInstance = true
status.PendingAdd = false
if netInErrState {
z.addAppNetworkError(&status, "handleAppNetworkCreate", err)
} else {
z.publishAppNetworkStatus(&status)
}
return
}

if config.Activate {
z.doActivateAppNetwork(config, &status)
} else {
status.PendingAdd = false
z.publishAppNetworkStatus(&status)
}

z.maybeScheduleRetry()
Expand All @@ -519,12 +515,15 @@ func (z *zedrouter) handleAppNetworkModify(ctxArg interface{}, key string,
status.ClearError()
status.PendingModify = true
z.publishAppNetworkStatus(status)
defer func() {
status.PendingModify = false
z.publishAppNetworkStatus(status)
}()

// Check for unsupported/invalid changes.
if err := z.validateAppNetworkConfigForModify(newConfig, oldConfig); err != nil {
z.log.Errorf("handleAppNetworkModify(%v): validation failed: %v",
newConfig.UUIDandVersion.UUID, err)
status.PendingModify = false
z.addAppNetworkError(status, "handleAppNetworkModify", err)
return
}
Expand All @@ -538,27 +537,21 @@ func (z *zedrouter) handleAppNetworkModify(ctxArg interface{}, key string,
if err != nil {
z.log.Errorf("handleAppNetworkModify(%v): %v", newConfig.UUIDandVersion.UUID, err)
status.AwaitNetworkInstance = true
status.PendingModify = false
if netInErrState {
z.addAppNetworkError(status, "handleAppNetworkModify", err)
} else {
z.publishAppNetworkStatus(status)
}
return
}

if !newConfig.Activate && status.Activated {
z.doInactivateAppNetwork(newConfig, status)
z.doCopyAppNetworkConfigToStatus(newConfig, status)
z.publishAppNetworkStatus(status)
} else if newConfig.Activate && !status.Activated {
z.doCopyAppNetworkConfigToStatus(newConfig, status)
z.doActivateAppNetwork(newConfig, status)
} else if !status.Activated {
// Just copy in newConfig
z.doCopyAppNetworkConfigToStatus(newConfig, status)
status.PendingModify = false
z.publishAppNetworkStatus(status)
} else { // Config change while application network is active.
z.doUpdateActivatedAppNetwork(oldConfig, newConfig, status)
}
Expand All @@ -584,6 +577,8 @@ func (z *zedrouter) handleAppNetworkDelete(ctxArg interface{}, key string,

// Deactivate app network if it is currently activated.
if status.Activated {
// No need to clear PendingDelete later. Instead, we un-publish
// the status completely few lines below.
status.PendingDelete = true
z.publishAppNetworkStatus(status)
z.doInactivateAppNetwork(config, status)
Expand Down

0 comments on commit 2476df9

Please sign in to comment.