app number free on underlay network #2285
Conversation
srinibas-zededa
force-pushed
the
sticky-ip
branch
4 times, most recently
from
ba7e02b to
e3ffb35
Compare
…e delete try Signed-off-by: Srinibas Maharana <srinibas@zededa.com>
srinibas-zededa
force-pushed
the
sticky-ip
branch
from
0e07a6e to
000d4b4
Compare
| @@ -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 | |||
There was a problem hiding this comment.
This is a bugfix. Please do as separate PR.
There was a problem hiding this comment.
99% of these changes are unneccesary and I don't see how they improve readability.
Two bug fixes are needed: the PendingCreate = false fix and making sure that when a Modify releases an appnum the maybeNetworkInstanceDelete() gets called.
The latter only makes sense from the top as I said before.
zedrouter.go should always call a new function which does what appNumOnNetworkInstanceRelease() does, but I'm not sure about the name for this function. (The abstraction hence naming is a bit of a mess IMHO.)
Thus the code in appnumonunet.go should have no idea what a network instance is.
It merely operates on network UUIDs and app instance UUID. (Yes, the arguments should be a networkID; the caller has no idea what a baseid is; the notion of a baseID belongs in uuidpairtonum. As I said, the abstraction hence naming is a bit of a mess.)
So I
| if _, err := appNumOnUNetGet(ctx, networkID, appID); err == nil { | ||
| appNumOnUNetFree(ctx, networkID, appID) | ||
| } | ||
| netstatus := lookupNetworkInstanceStatus(ctx, networkID.String()) |
There was a problem hiding this comment.
Why do you need to do this when you didn't call appNumOnUNetFree?
| if !status.Activated { | ||
| // need not handle ACL changes in deactivated state |
There was a problem hiding this comment.
I don't see how these changes make the code any easier to read and understand.
| if maybeNetworkInstanceDelete(ctx, netstatus) { | ||
| log.Noticef("deleted network instance %s", netstatus.Key()) | ||
| } else { | ||
| // publish the changes to network instance status | ||
| publishNetworkInstanceStatus(ctx, netstatus) | ||
| } |
There was a problem hiding this comment.
How do you know this isn't required?
| // may be multiple config entries, need not try scheduling again | ||
| // just return | ||
| return |
There was a problem hiding this comment.
What does this change do in this PR?
| @@ -1771,19 +1786,6 @@ func handleDelete(ctx *zedrouterContext, key string, | |||
|
|
|||
| appNumFree(ctx, status.UUIDandVersion.UUID) | |||
| appNumsOnUNetFree(ctx, status) | |||
There was a problem hiding this comment.
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.
| status *types.AppNetworkStatus, | ||
| config types.AppNetworkConfig, | ||
| oldConfig types.AppNetworkConfig, | ||
| ipsets []string, force bool) { |
There was a problem hiding this comment.
The forice argument is unused. Are there other unused arguments?
|
@eriknordmark can we please close? |
|
This PR is obsolete - number allocators have been refactored and the rest of the zedrouter will be soon as well. |
From lf-edge#2285 Signed-off-by: eriknordmark <erik@zededa.com>
From comments in lf-edge#2285 Signed-off-by: eriknordmark <erik@zededa.com>
From lf-edge#2285 Signed-off-by: eriknordmark <erik@zededa.com>
From comments in lf-edge#2285 Signed-off-by: eriknordmark <erik@zededa.com>
From #2285 Signed-off-by: eriknordmark <erik@zededa.com>
From comments in #2285 Signed-off-by: eriknordmark <erik@zededa.com>
Signed-off-by: Srinibas Maharana srinibas@zededa.com