From a78cc048d932e34f690ab99b5726547d1fec3a09 Mon Sep 17 00:00:00 2001
From: Malte Bauch <malte.bauch@stud.h-da.de>
Date: Fri, 5 May 2023 12:31:52 +0000
Subject: [PATCH] Resolve "A change confirm updates the network element
multiple times"
See merge request danet/gosdn!464
---
controller/interfaces/change/change.go | 3 +
controller/interfaces/transport/transport.go | 3 +-
controller/mocks/Change.go | 22 ++++++-
controller/mocks/Transport.go | 12 ++--
.../northbound/server/networkElement.go | 26 ++++----
controller/nucleus/change.go | 11 +++-
controller/nucleus/gnmi_transport.go | 46 ++-------------
controller/nucleus/gnmi_transport_test.go | 59 +++++++------------
controller/nucleus/util/gnmi/notification.go | 14 +++++
9 files changed, 94 insertions(+), 102 deletions(-)
create mode 100644 controller/nucleus/util/gnmi/notification.go
diff --git a/controller/interfaces/change/change.go b/controller/interfaces/change/change.go
index bde850fe7..489b42303 100644
--- a/controller/interfaces/change/change.go
+++ b/controller/interfaces/change/change.go
@@ -6,6 +6,7 @@ import (
mnepb "code.fbi.h-da.de/danet/gosdn/api/go/gosdn/networkelement"
"github.com/google/uuid"
+ "github.com/openconfig/gnmi/proto/gnmi"
)
// Change is an intended change to an MNE. It is unique and immutable.
@@ -20,6 +21,7 @@ type Change interface {
Confirm() error
PreviousState() []byte
IntendedState() []byte
+ Diff() *gnmi.Notification
AssociatedDeviceID() uuid.UUID
}
@@ -28,4 +30,5 @@ type Change interface {
type Payload struct {
Original []byte
Modified []byte
+ Diff *gnmi.Notification
}
diff --git a/controller/interfaces/transport/transport.go b/controller/interfaces/transport/transport.go
index c93927873..586115cda 100644
--- a/controller/interfaces/transport/transport.go
+++ b/controller/interfaces/transport/transport.go
@@ -4,7 +4,6 @@ import (
"context"
"code.fbi.h-da.de/danet/gosdn/controller/interfaces/change"
- "code.fbi.h-da.de/danet/gosdn/controller/interfaces/plugin"
gpb "github.com/openconfig/gnmi/proto/gnmi"
)
@@ -13,7 +12,7 @@ import (
// like RESTCONF or gnmi.
type Transport interface {
Get(ctx context.Context, params ...string) (any, error)
- Set(ctx context.Context, payload change.Payload, path string, plugin plugin.Plugin) error
+ Set(ctx context.Context, payload change.Payload) error
CustomSet(ctx context.Context, req *gpb.SetRequest) (*gpb.SetResponse, error)
Subscribe(ctx context.Context, params ...string) error
ControlPlaneSubscribe(ctx context.Context, subscribeCallbackFunc HandleSubscribeResponse,
diff --git a/controller/mocks/Change.go b/controller/mocks/Change.go
index a9b23b912..5faf6ff7b 100644
--- a/controller/mocks/Change.go
+++ b/controller/mocks/Change.go
@@ -3,10 +3,12 @@
package mocks
import (
- time "time"
+ gnmi "github.com/openconfig/gnmi/proto/gnmi"
+ mock "github.com/stretchr/testify/mock"
networkelement "code.fbi.h-da.de/danet/gosdn/api/go/gosdn/networkelement"
- mock "github.com/stretchr/testify/mock"
+
+ time "time"
uuid "github.com/google/uuid"
)
@@ -74,6 +76,22 @@ func (_m *Change) Confirm() error {
return r0
}
+// Diff provides a mock function with given fields:
+func (_m *Change) Diff() *gnmi.Notification {
+ ret := _m.Called()
+
+ var r0 *gnmi.Notification
+ if rf, ok := ret.Get(0).(func() *gnmi.Notification); ok {
+ r0 = rf()
+ } else {
+ if ret.Get(0) != nil {
+ r0 = ret.Get(0).(*gnmi.Notification)
+ }
+ }
+
+ return r0
+}
+
// ID provides a mock function with given fields:
func (_m *Change) ID() uuid.UUID {
ret := _m.Called()
diff --git a/controller/mocks/Transport.go b/controller/mocks/Transport.go
index 5e1615014..b3a01d0af 100644
--- a/controller/mocks/Transport.go
+++ b/controller/mocks/Transport.go
@@ -11,8 +11,6 @@ import (
mock "github.com/stretchr/testify/mock"
- plugin "code.fbi.h-da.de/danet/gosdn/controller/interfaces/plugin"
-
transport "code.fbi.h-da.de/danet/gosdn/controller/interfaces/transport"
)
@@ -122,13 +120,13 @@ func (_m *Transport) ProcessResponse(resp interface{}) error {
return r0
}
-// Set provides a mock function with given fields: ctx, payload, path, _a3
-func (_m *Transport) Set(ctx context.Context, payload change.Payload, path string, _a3 plugin.Plugin) error {
- ret := _m.Called(ctx, payload, path, _a3)
+// Set provides a mock function with given fields: ctx, payload
+func (_m *Transport) Set(ctx context.Context, payload change.Payload) error {
+ ret := _m.Called(ctx, payload)
var r0 error
- if rf, ok := ret.Get(0).(func(context.Context, change.Payload, string, plugin.Plugin) error); ok {
- r0 = rf(ctx, payload, path, _a3)
+ if rf, ok := ret.Get(0).(func(context.Context, change.Payload) error); ok {
+ r0 = rf(ctx, payload)
} else {
r0 = ret.Error(0)
}
diff --git a/controller/northbound/server/networkElement.go b/controller/northbound/server/networkElement.go
index 6a433b7de..479291617 100644
--- a/controller/northbound/server/networkElement.go
+++ b/controller/northbound/server/networkElement.go
@@ -18,6 +18,7 @@ import (
"code.fbi.h-da.de/danet/gosdn/controller/metrics"
"code.fbi.h-da.de/danet/gosdn/controller/nucleus"
"code.fbi.h-da.de/danet/gosdn/controller/nucleus/types"
+ util "code.fbi.h-da.de/danet/gosdn/controller/nucleus/util/gnmi"
"code.fbi.h-da.de/danet/gosdn/controller/store"
aGNMI "code.fbi.h-da.de/danet/gosdn/forks/goarista/gnmi"
"github.com/google/uuid"
@@ -854,22 +855,27 @@ func (n *NetworkElementServer) ChangeMNE(duid uuid.UUID, operation mnepb.ApiOper
}
}
- callback := func(original, modified []byte) error {
- ctx := context.WithValue(context.Background(), types.CtxKeyOperation, operation) // nolint
- payload := change.Payload{Original: original, Modified: modified}
- pathToSet := path
- if err := mne.Transport().Set(ctx, payload, pathToSet, plugin); err != nil {
- return err
- }
- return n.mneService.Update(mne)
+ currentModel, err := mne.GetModelAsFilteredCopy()
+ if err != nil {
+ return uuid.Nil, err
}
- currentModel, err := mne.GetModelAsFilteredCopy()
+ diff, err := plugin.Diff(currentModel, filteredMarshaledModel)
if err != nil {
return uuid.Nil, err
}
- ch := nucleus.NewChange(duid, currentModel, filteredMarshaledModel, callback)
+ if util.IsGNMINotificationEmpty(diff) {
+ return uuid.Nil, customerrs.NoNewChangesError{Original: string(currentModel), Modified: string(filteredMarshaledModel)}
+ }
+
+ callback := func(original, modified []byte) error {
+ ctx := context.WithValue(context.Background(), types.CtxKeyOperation, operation) // nolint
+ payload := change.Payload{Original: original, Modified: modified, Diff: diff}
+ return mne.Transport().Set(ctx, payload)
+ }
+
+ ch := nucleus.NewChange(duid, currentModel, filteredMarshaledModel, diff, callback)
if err := n.changeStore.Add(ch); err != nil {
return uuid.Nil, err
diff --git a/controller/nucleus/change.go b/controller/nucleus/change.go
index b1bc268a5..0cdecdcc2 100644
--- a/controller/nucleus/change.go
+++ b/controller/nucleus/change.go
@@ -10,6 +10,7 @@ import (
mnepb "code.fbi.h-da.de/danet/gosdn/api/go/gosdn/networkelement"
"github.com/google/uuid"
+ "github.com/openconfig/gnmi/proto/gnmi"
log "github.com/sirupsen/logrus"
)
@@ -33,13 +34,14 @@ func init() {
// a callback function, and returns a *Change.
// The callback function is used by the Commit() and Confirm() functions. It
// must define how the change is carried out.
-func NewChange(mne uuid.UUID, currentState, change []byte, callback func([]byte, []byte) error) *Change {
+func NewChange(mne uuid.UUID, currentState, change []byte, diff *gnmi.Notification, callback func([]byte, []byte) error) *Change {
c := &Change{
cuid: uuid.New(),
duid: mne,
state: mnepb.ChangeState_CHANGE_STATE_PENDING,
timestamp: time.Now(),
previousState: currentState,
+ diff: diff,
intendedState: change,
callback: callback,
}
@@ -63,6 +65,7 @@ type Change struct {
timestamp time.Time
previousState []byte
intendedState []byte
+ diff *gnmi.Notification
callback func([]byte, []byte) error
stateMu sync.RWMutex
errChan <-chan error
@@ -132,6 +135,12 @@ func (c *Change) IntendedState() []byte {
return c.intendedState
}
+// Diff returns the differences between the previous and the intended state as
+// gnmi.Notification.
+func (c *Change) Diff() *gnmi.Notification {
+ return c.diff
+}
+
func stateManager(ctx context.Context, ch *Change, timeout time.Duration) (chan<- mnepb.ChangeState, <-chan mnepb.ChangeState, <-chan error) {
stateIn := make(chan mnepb.ChangeState)
stateOut := make(chan mnepb.ChangeState)
diff --git a/controller/nucleus/gnmi_transport.go b/controller/nucleus/gnmi_transport.go
index 20176b1f1..6494520e3 100644
--- a/controller/nucleus/gnmi_transport.go
+++ b/controller/nucleus/gnmi_transport.go
@@ -5,7 +5,6 @@ import (
"fmt"
"code.fbi.h-da.de/danet/gosdn/controller/interfaces/change"
- "code.fbi.h-da.de/danet/gosdn/controller/interfaces/plugin"
tpInterface "code.fbi.h-da.de/danet/gosdn/controller/interfaces/transport"
"code.fbi.h-da.de/danet/gosdn/controller/plugin/shared"
@@ -13,7 +12,6 @@ import (
"code.fbi.h-da.de/danet/gosdn/controller/nucleus/types"
"code.fbi.h-da.de/danet/gosdn/forks/goarista/gnmi"
gpb "github.com/openconfig/gnmi/proto/gnmi"
- "github.com/openconfig/ygot/ygot"
log "github.com/sirupsen/logrus"
tpb "code.fbi.h-da.de/danet/gosdn/api/go/gosdn/transport"
@@ -87,39 +85,17 @@ func (g *Gnmi) Get(ctx context.Context, params ...string) (interface{}, error) {
}
// Set takes a change.Payload struct.
-func (g *Gnmi) Set(ctx context.Context, payload change.Payload, path string, plugin plugin.Plugin) error {
- p, err := ygot.StringToStructuredPath(path)
- if err != nil {
- return err
- }
+func (g *Gnmi) Set(ctx context.Context, payload change.Payload) error {
if g.client == nil {
return &customerrs.NilClientError{}
}
ctx = gnmi.NewContext(ctx, g.config)
- return g.applyDiff(ctx, payload, p, plugin)
-}
-
-// isGNMINotificationEmpty checks if the given gnmi.Notification does not
-// contain any updates or deletes.
-func isGNMINotificationEmpty(n *gpb.Notification) bool {
- if n.Update == nil || len(n.Update) == 0 {
- if n.Delete == nil || len(n.Delete) == 0 {
- return true
- }
- }
- return false
+ return g.applyDiff(ctx, payload)
}
-func (g *Gnmi) applyDiff(ctx context.Context, payload change.Payload, path *gpb.Path, plugin plugin.Plugin) error {
- diff, err := plugin.Diff(payload.Original, payload.Modified)
- if err != nil {
- return err
- }
-
- if isGNMINotificationEmpty(diff) {
- return customerrs.NoNewChangesError{Original: string(payload.Original), Modified: string(payload.Modified)}
- }
+func (g *Gnmi) applyDiff(ctx context.Context, payload change.Payload) error {
+ diff := payload.Diff
updates := diff.GetUpdate()
deletes := diff.GetDelete()
@@ -136,20 +112,6 @@ func (g *Gnmi) applyDiff(ctx context.Context, payload change.Payload, path *gpb.
}
log.Info(resp)
- // apply diffs to the plugin of the managed network element.
- for _, update := range updates {
- err := plugin.SetNode(update.GetPath(), update.GetVal())
- if err != nil {
- return err
- }
- }
- for _, del := range deletes {
- err := plugin.DeleteNode(del)
- if err != nil {
- return err
- }
- }
-
return nil
}
diff --git a/controller/nucleus/gnmi_transport_test.go b/controller/nucleus/gnmi_transport_test.go
index 901275d63..e7230b1af 100644
--- a/controller/nucleus/gnmi_transport_test.go
+++ b/controller/nucleus/gnmi_transport_test.go
@@ -3,7 +3,6 @@ package nucleus
import (
"context"
"errors"
- "fmt"
"reflect"
"testing"
@@ -265,33 +264,10 @@ func TestGnmi_ProcessResponse(t *testing.T) {
}
func TestGnmi_Set(t *testing.T) {
- mockPlugin := mockPlugin(t)
- mockCall := mockPlugin.(*mocks.Plugin).On("Diff", mock.IsType([]byte{}), mock.IsType([]byte{}))
- mockCall.RunFn = func(args mock.Arguments) {
- path, err := ygot.StringToStructuredPath("/system/config/hostname")
- if err != nil {
- mockCall.ReturnArguments = mock.Arguments{nil, fmt.Errorf("StringToStructuredPath failed in ReturnArguments")}
- }
- if string(args.Get(0).([]byte)) == "" {
- mockCall.ReturnArguments = mock.Arguments{&gpb.Notification{}, nil}
- } else if string(args.Get(0).([]byte)) == "update" {
- mockCall.ReturnArguments = mock.Arguments{&gpb.Notification{
- Update: []*gpb.Update{{Path: path, Val: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "newName"}}}},
- },
- nil,
- }
- } else if string(args.Get(0).([]byte)) == "delete" {
- mockCall.ReturnArguments = mock.Arguments{&gpb.Notification{
- Delete: []*gpb.Path{path},
- },
- nil,
- }
- }
+ hostnamePath, err := ygot.StringToStructuredPath("system/config/hostname")
+ if err != nil {
+ t.Fatal(err)
}
- mockPlugin.(*mocks.Plugin).On("Unmarshal", mock.Anything, mock.Anything).Return(nil)
- mockPlugin.(*mocks.Plugin).On("SetNode", mock.Anything, mock.Anything).Return(nil)
- mockPlugin.(*mocks.Plugin).On("DeleteNode", mock.Anything).Return(nil)
-
setResponse := &gpb.SetResponse{}
type fields struct {
@@ -309,16 +285,6 @@ func TestGnmi_Set(t *testing.T) {
args args
wantErr bool
}{
- {
- name: "uninitialised",
- fields: fields{transport: mockTransport(t)},
- args: args{
- payload: change.Payload{},
- path: "/",
- ctx: context.WithValue(context.Background(), types.CtxKeyOperation, mnepb.ApiOperation_API_OPERATION_UPDATE), // nolint
- },
- wantErr: true,
- },
{
name: "updateValue",
fields: fields{
@@ -343,6 +309,18 @@ func TestGnmi_Set(t *testing.T) {
payload: change.Payload{
Original: []byte("update"),
Modified: []byte("update2"),
+ Diff: &gpb.Notification{
+ Update: []*gpb.Update{
+ {
+ Path: hostnamePath,
+ Val: &gpb.TypedValue{
+ Value: &gpb.TypedValue_StringVal{
+ StringVal: "newName",
+ },
+ },
+ },
+ },
+ },
},
path: "/system/config/hostname",
ctx: context.WithValue(context.Background(), types.CtxKeyOperation, mnepb.ApiOperation_API_OPERATION_UPDATE), // nolint
@@ -365,6 +343,11 @@ func TestGnmi_Set(t *testing.T) {
payload: change.Payload{
Original: []byte("delete"),
Modified: []byte("delete2"),
+ Diff: &gpb.Notification{
+ Delete: []*gpb.Path{
+ hostnamePath,
+ },
+ },
},
path: "/system/config/hostname",
ctx: context.WithValue(context.Background(), types.CtxKeyOperation, mnepb.ApiOperation_API_OPERATION_DELETE), // nolint
@@ -376,7 +359,7 @@ func TestGnmi_Set(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
tt.fields.transport.client.(*mocks.GNMIClient).
On("Set", mockContext, tt.fields.mockArgumentMatcher).Return(setResponse, nil)
- err := tt.fields.transport.Set(tt.args.ctx, tt.args.payload, tt.args.path, mockPlugin)
+ err := tt.fields.transport.Set(tt.args.ctx, tt.args.payload)
if (err != nil) != tt.wantErr {
t.Errorf("Set() error = %v, wantErr %v", err, tt.wantErr)
}
diff --git a/controller/nucleus/util/gnmi/notification.go b/controller/nucleus/util/gnmi/notification.go
new file mode 100644
index 000000000..407faf3b1
--- /dev/null
+++ b/controller/nucleus/util/gnmi/notification.go
@@ -0,0 +1,14 @@
+package gnmi
+
+import "github.com/openconfig/gnmi/proto/gnmi"
+
+// IsGNMINotificationEmpty checks if the given gnmi.Notification does not
+// contain any updates or deletes.
+func IsGNMINotificationEmpty(n *gnmi.Notification) bool {
+ if n.Update == nil || len(n.Update) == 0 {
+ if n.Delete == nil || len(n.Delete) == 0 {
+ return true
+ }
+ }
+ return false
+}
--
GitLab