Further refactoring zedrouter to make it much more robust

[milan-zededa]

Testing of refactored zedrouter showed that while it can handle incremental changes (one change at a time, e.g.: add network instance; connect new app; etc.), there are some cracks when it comes to applying new device config containing multiple changes at once (e.g. the set of currently deployed network instances is replaced with a completely different set). However unlikely it is to encounter such complex sudden device config changes in practice, it would be nice if zedrouter was robust enough to handle anything.

An eden test was prepared to stress zedrouter under complex config changes: lf-edge/eden#870
This test would fail even before already merged zedrouter refactoring. Zedrouter was never robust enough to handle any config change (even if both the previous and the new config are valid). This PR changes that and makes the test pass.

This PR continues (and hopefully ends!) with zedrouter refactoring and adds some commits. The three main topics are:

• bring the latest reconciler improvements related to async operations (Reconciler: improvements for processing of async operations #3311) to pillar
• several changes made to LinuxNIReconciler for more robustness
• better reporting of the network instance status

More detailed info can be found in descriptions of individual commits.

[uncleDecart]

LGTM, thanks for structuring commits. Was easy to read your changes. And you have a typo in this commit

[eriknordmark]

I see you changed the sanitycheck functions for the network instance to operate on the config and not the status. Is there ever a case when we keep a status around after the config has been deleted by zedagent, or is the delete of the status always immediate when the config is gone?
(I don't know why we we previously checking this on status.)

[eriknordmark]

@milan-zededa I see a golang panic when running ztests.
{"file":"/pillar/cmd/zedagent/zedagent.go:2136","func":"github.com/lf-edge/eve/pkg/pillar/cmd/zedagent.handleDPCLImpl","level":"info","msg":"handleDPCLImpl: has real change.","pid":1825,"source":"zedagent","time":"2023-07-07T23:46:32.510430094Z"}
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0x2276290]

goroutine 242 [running]:
github.com/lf-edge/eve/pkg/pillar/nireconciler.(*LinuxNIReconciler).niBridgeIsCreatedByNIM(...)
/pillar/nireconciler/linux_config.go:1011
github.com/lf-edge/eve/pkg/pillar/nireconciler.(*LinuxNIReconciler).updateCurrentNIBridge(0xc000ae2800, {0xa5, 0x69, 0xd8, 0xa8, 0x21, 0x87, 0x4a, 0x1e, 0x95, ...})
/pillar/nireconciler/linux_currentstate.go:179 +0x210
github.com/lf-edge/eve/pkg/pillar/nireconciler.(*LinuxNIReconciler).runWatcher(0xc000ae2800, 0xc000cbe420)
/pillar/nireconciler/linux_reconciler.go:308 +0xb4e
created by github.com/lf-edge/eve/pkg/pillar/nireconciler.(*LinuxNIReconciler).init
/pillar/nireconciler/linux_reconciler.go:191 +0x50f

I've verified that this panic is due to niBridgeIsCreatedByNIM() being called with a nil argument. Guarding against that makes the ztests pass.

[milan-zededa]

I see a golang panic when running ztests.

@eriknordmark Should be fixed now. The place in the code and variable being nil should no longer be reachable.

[milan-zededa]

I see you changed the sanitycheck functions for the network instance to operate on the config and not the status. Is there ever a case when we keep a status around after the config has been deleted by zedagent, or is the delete of the status always immediate when the config is gone?
(I don't know why we we previously checking this on status.)

Yes, the status lives a bit longer than config when delete of network instance is performed. Config is removed immediately by zedagent, but status disappears only after zedrouter completed removal of a NI.
Using config instead of status for validation avoids including NI that is being deleted in subnet overlap checks (against newly submitted NIs). But that's a rather special case - even more importantly it just makes more sense to validate config I think.

[eriknordmark]

Yes, the status lives a bit longer than config when delete of network instance is performed. Config is removed immediately by zedagent, but status disappears only after zedrouter completed removal of a NI. Using config instead of status for validation avoids including NI that is being deleted in subnet overlap checks (against newly submitted NIs). But that's a rather special case - even more importantly it just makes more sense to validate config I think.

Could there be cases where this causes the conflict check to pass (on config), but then there is an error deep in the code due to the status and related kernel state not being cleaned up yet?

[milan-zededa]

Yes, the status lives a bit longer than config when delete of network instance is performed. Config is removed immediately by zedagent, but status disappears only after zedrouter completed removal of a NI. Using config instead of status for validation avoids including NI that is being deleted in subnet overlap checks (against newly submitted NIs). But that's a rather special case - even more importantly it just makes more sense to validate config I think.

Could there be cases where this causes the conflict check to pass (on config), but then there is an error deep in the code due to the status and related kernel state not being cleaned up yet?

These complexities have now been moved to the reconciler layer. It handles config items of all network instances and their dependencies. If something is still being deleted for an obsolete network instance, it may delay creation of a new network instance (if some config items overlap), but eventually it will iterate into the intended state. Error from one network instance, e.g. a failed delete, may negatively affect creation/modification of another network instance and the error will in some form propagate to the status of the other NI. Or at least the other NI will show as still being in progress.

Checking status errors of network instances at this higher level would not work very well. This is because it is harder to tell at that level if there is actually any overlap between config items of NIs and if the error relates to the overlap. So I think it is OK to let the validation pass and fail later, but only if there is an actual conflict with another NI.

[eriknordmark]

Ignoring the three yetus complains on existing unmodified code.