Delete ovs bridge when forwarder stop

[ljkiraly]

Description

Cleanup the bridge used to connect NSM endpoints.

Issue link

Related issue: #276

How Has This Been Tested?

• Added unit testing to cover
• Tested manually
• Tested by integration testing
• Have not tested

Reproduction steps described in the issue

Types of changes

• Bug fix
• New functionality
• Documentation
• Refactoring
• CI

[denis-tingaikin]

I think we may convert this function to private and do next:

1. Create a new chain element or edit one of the existing chain elements if possible.
2. Add application context to the chain element from step 1. (Note: the application context is https://github.com/networkservicemesh/cmd-forwarder-ovs/blob/main/main.go#L109)
3. Simply start a new gorouite once that waits for the application context to be done and removes the bridge.

With this change, DeleteBridge will be a private thing, and we will prevent unexpected usage and API leaks that may increase the complexity of the system.

Also, if we edit exist chain element in step1 we could close networkservicemesh/cmd-forwarder-ovs#305

[ljkiraly]

Hello @denis-tingaikin,
In case when OVS is running locally indside forwarder-ovs container it is supervised and started by 'supervisord':
https://github.com/networkservicemesh/cmd-forwarder-ovs/blob/0798082507c276e1904d5cc40bea16c05d71e61e/internal/ovsinit/start.go#L64

I've just tried the following change:

diff --git a/pkg/networkservice/chains/forwarder/server.go b/pkg/networkservice/chains/forwarder/server.go
...
+       go func() { // Cleanup the main nsm bridge
+               <-ctx.Done()
+               exec := kexec.New()
+               if err := util.SetExec(exec); err == nil {
+                       _, stdErr, err := util.RunOVSVsctl("del-br", bridgeName)
+                       if stdErr != "" {
+                               fmt.Printf("Failed to delete interfaces %s", stdErr)
+                               return
+                       }
+                       if err != nil {
+                               fmt.Printf("Failed to run ovsvsctl %v", err)
+                               return
+                       }
+               } else {
+                       fmt.Printf("Failed to init exec %v", err)
+                       return
+               }
+               fmt.Println("deleted nsm-br")
+       }()

Since the context already done, the 'supervisord' and the ovs daemons are stopped, this function will fail on github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util.SetExec. In short: can not execute ovs commands after context was canceled or done. Interesting that seemingly the 'defer' is called before context.Done is sent.
If we decide to implement the proposed way the remainders (nsm-bridge and ovs-system) must be deleted by netlink in case of this (init exec) failure. If the nsm bridge is deleted before ovs stops the 'ovs-system' interface is deleted automatically by ovs.

[ljkiraly]

Another option should be to delete the bridge interface in cmd-forwarder-ovs main without creating additional API. Updated networkservicemesh/cmd-forwarder-ovs#305.

[denis-tingaikin]

Another option should be to delete the bridge interface in cmd-forwarder-ovs main without creating an additional API. Updated networkservicemesh/cmd-forwarder-ovs#305.

Woot! It sounds smart and easy. I think we may go with networkservicemesh/cmd-forwarder-ovs#305.

[ljkiraly]

networkservicemesh/cmd-forwarder-ovs#305.merged, closing this PR