From 4010afd85d3b94bbdf9e905f8adb43969b54ad83 Mon Sep 17 00:00:00 2001 From: Alexander North Date: Fri, 24 Jul 2026 15:00:13 +0200 Subject: [PATCH 1/2] add new not connected error type and grpc unavailable mapping --- pkg/datastore/intent_rpc.go | 2 +- pkg/datastore/target/gnmi/gnmi.go | 2 +- pkg/datastore/target/netconf/nc.go | 4 +- pkg/datastore/target/types/targetstatus.go | 6 ++ pkg/server/transaction.go | 9 ++- pkg/server/transaction_test.go | 70 ++++++++++++++++++++++ 6 files changed, 87 insertions(+), 6 deletions(-) create mode 100644 pkg/server/transaction_test.go diff --git a/pkg/datastore/intent_rpc.go b/pkg/datastore/intent_rpc.go index 974384d5..f9a3b48b 100644 --- a/pkg/datastore/intent_rpc.go +++ b/pkg/datastore/intent_rpc.go @@ -46,7 +46,7 @@ func (d *Datastore) applyIntent(ctx context.Context, source targettypes.TargetSo } if d.sbi == nil { - return nil, fmt.Errorf("%s is not connected", d.config.Name) + return nil, fmt.Errorf("%s: %w", d.config.Name, targettypes.ErrNotConnected) } rsp, err = d.sbi.Set(ctx, source) diff --git a/pkg/datastore/target/gnmi/gnmi.go b/pkg/datastore/target/gnmi/gnmi.go index a174af9a..7df85f5f 100644 --- a/pkg/datastore/target/gnmi/gnmi.go +++ b/pkg/datastore/target/gnmi/gnmi.go @@ -161,7 +161,7 @@ func (t *gnmiTarget) Set(ctx context.Context, source targetTypes.TargetSource) ( var err error if t == nil { - return nil, fmt.Errorf("%s", "not connected") + return nil, targetTypes.ErrNotConnected } // deletes from protos diff --git a/pkg/datastore/target/netconf/nc.go b/pkg/datastore/target/netconf/nc.go index e04d9775..064eda2b 100644 --- a/pkg/datastore/target/netconf/nc.go +++ b/pkg/datastore/target/netconf/nc.go @@ -99,7 +99,7 @@ func (t *ncTarget) internalGet(ctx context.Context, req *sdcpb.GetDataRequest) ( ctx = logf.IntoContext(ctx, log) if !t.Status().IsConnected() { - return nil, fmt.Errorf("%s", types.TargetStatusNotConnected) + return nil, fmt.Errorf("%s: %w", t.name, types.ErrNotConnected) } source := "running" @@ -163,7 +163,7 @@ func (t *ncTarget) Set(ctx context.Context, source types.TargetSource) (*sdcpb.S log := logf.FromContext(ctx).WithName("Set") ctx = logf.IntoContext(ctx, log) if !t.Status().IsConnected() { - return nil, fmt.Errorf("%s", types.TargetStatusNotConnected) + return nil, fmt.Errorf("%s: %w", t.name, types.ErrNotConnected) } switch t.sbiConfig.NetconfOptions.CommitDatastore { diff --git a/pkg/datastore/target/types/targetstatus.go b/pkg/datastore/target/types/targetstatus.go index 249f8767..6fb903a6 100644 --- a/pkg/datastore/target/types/targetstatus.go +++ b/pkg/datastore/target/types/targetstatus.go @@ -1,5 +1,11 @@ package types +import "errors" + +// ErrNotConnected indicates the southbound interface (device connection) of a +// datastore is not established +var ErrNotConnected = errors.New("not connected") + type TargetStatus struct { Status TargetConnectionStatus Details string diff --git a/pkg/server/transaction.go b/pkg/server/transaction.go index 4e8be390..89e9f25f 100644 --- a/pkg/server/transaction.go +++ b/pkg/server/transaction.go @@ -7,6 +7,7 @@ import ( "time" "github.com/sdcio/data-server/pkg/datastore" + targettypes "github.com/sdcio/data-server/pkg/datastore/target/types" "github.com/sdcio/data-server/pkg/datastore/types" "github.com/sdcio/data-server/pkg/tree/consts" "github.com/sdcio/data-server/pkg/utils" @@ -153,8 +154,12 @@ func (s *Server) TransactionCancel(ctx context.Context, req *sdcpb.TransactionCa // translateInternalToGrpcError central function to map internal errors to grpc error codes func translateInternalToGrpcError(err error) error { - if errors.Is(err, datastore.ErrDatastoreLocked) { + switch { + case errors.Is(err, datastore.ErrDatastoreLocked): return status.Error(codes.Aborted, err.Error()) + case errors.Is(err, targettypes.ErrNotConnected): + return status.Error(codes.Unavailable, err.Error()) + default: + return err } - return err } diff --git a/pkg/server/transaction_test.go b/pkg/server/transaction_test.go new file mode 100644 index 00000000..44361dcc --- /dev/null +++ b/pkg/server/transaction_test.go @@ -0,0 +1,70 @@ +package server + +import ( + "errors" + "fmt" + "testing" + + "github.com/sdcio/data-server/pkg/datastore" + targettypes "github.com/sdcio/data-server/pkg/datastore/target/types" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +func TestTranslateInternalToGrpcError(t *testing.T) { + cases := map[string]struct { + err error + wantNil bool + wantSame bool // returned unchanged (non-status passthrough) + wantCode codes.Code + }{ + "nil": { + err: nil, + wantNil: true, + }, + "datastore locked -> Aborted": { + err: datastore.ErrDatastoreLocked, + wantCode: codes.Aborted, + }, + "not connected -> Unavailable": { + err: targettypes.ErrNotConnected, + wantCode: codes.Unavailable, + }, + "wrapped not connected -> Unavailable": { + err: fmt.Errorf("some.datastore: %w", targettypes.ErrNotConnected), + wantCode: codes.Unavailable, + }, + "other error -> passthrough": { + err: errors.New("boom"), + wantSame: true, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got := translateInternalToGrpcError(tc.err) + + if tc.wantNil { + if got != nil { + t.Fatalf("expected nil, got %v", got) + } + return + } + + if tc.wantSame { + if got != tc.err { + t.Fatalf("expected passthrough of the same error, got %v", got) + } + return + } + + st, ok := status.FromError(got) + if !ok { + t.Fatalf("expected a gRPC status error, got %v", got) + } + if st.Code() != tc.wantCode { + t.Fatalf("expected code %v, got %v (%v)", tc.wantCode, st.Code(), got) + } + }) + } +} From f15c4f1f50b3bc5f78e28d13afe5d8d2871e9fee Mon Sep 17 00:00:00 2001 From: Alexander North Date: Thu, 13 Aug 2026 12:57:35 +0200 Subject: [PATCH 2/2] TargetStatus refactoring --- pkg/datastore/datastore_rpc.go | 2 +- pkg/datastore/intent_rpc.go | 2 +- pkg/datastore/target/gnmi/gnmi.go | 20 +++--- pkg/datastore/target/netconf/nc.go | 12 ++-- pkg/datastore/target/netconf/status_test.go | 53 ++++++++++++++ pkg/datastore/target/noop/noop.go | 2 +- pkg/datastore/target/types/targetstatus.go | 34 ++++++--- .../target/types/targetstatus_test.go | 72 +++++++++++++++++++ pkg/server/datastore.go | 13 +--- 9 files changed, 171 insertions(+), 39 deletions(-) create mode 100644 pkg/datastore/target/netconf/status_test.go create mode 100644 pkg/datastore/target/types/targetstatus_test.go diff --git a/pkg/datastore/datastore_rpc.go b/pkg/datastore/datastore_rpc.go index da0cb61e..856efa0e 100644 --- a/pkg/datastore/datastore_rpc.go +++ b/pkg/datastore/datastore_rpc.go @@ -207,7 +207,7 @@ func (d *Datastore) Delete(ctx context.Context) error { func (d *Datastore) ConnectionState() *targettypes.TargetStatus { if d.sbi == nil { - return targettypes.NewTargetStatus(targettypes.TargetStatusNotConnected) + return targettypes.NewTargetStatus(sdcpb.TargetStatus_NOT_CONNECTED) } return d.sbi.Status() } diff --git a/pkg/datastore/intent_rpc.go b/pkg/datastore/intent_rpc.go index f9a3b48b..016ec1f1 100644 --- a/pkg/datastore/intent_rpc.go +++ b/pkg/datastore/intent_rpc.go @@ -51,7 +51,7 @@ func (d *Datastore) applyIntent(ctx context.Context, source targettypes.TargetSo rsp, err = d.sbi.Set(ctx, source) if err != nil { - return nil, err + return nil, fmt.Errorf("%s: %w", d.config.Name, err) } log.V(logf.VDebug).Info("got SetResponse from SBI", "raw-response", utils.FormatProtoJSON(rsp)) diff --git a/pkg/datastore/target/gnmi/gnmi.go b/pkg/datastore/target/gnmi/gnmi.go index 7df85f5f..14fbde81 100644 --- a/pkg/datastore/target/gnmi/gnmi.go +++ b/pkg/datastore/target/gnmi/gnmi.go @@ -160,8 +160,8 @@ func (t *gnmiTarget) Set(ctx context.Context, source targetTypes.TargetSource) ( var deletes []*sdcpb.Path var err error - if t == nil { - return nil, targetTypes.ErrNotConnected + if err := t.Status().Err(); err != nil { + return nil, err } // deletes from protos @@ -251,19 +251,19 @@ func (t *gnmiTarget) Set(ctx context.Context, source targetTypes.TargetSource) ( } func (t *gnmiTarget) Status() *targetTypes.TargetStatus { - result := targetTypes.NewTargetStatus(targetTypes.TargetStatusNotConnected) + result := targetTypes.NewTargetStatus(sdcpb.TargetStatus_NOT_CONNECTED) if t == nil || t.target == nil { result.Details = "connection not initialized" return result } - switch t.target.ConnState() { - case connectivity.Ready.String(), connectivity.Idle.String(): - result.Status = targetTypes.TargetStatusConnected - result.Details = t.target.ConnState() - case connectivity.Connecting.String(), connectivity.Shutdown.String(), connectivity.TransientFailure.String(): - result.Status = targetTypes.TargetStatusNotConnected - result.Details = t.target.ConnState() + state := t.target.ConnectivityState() + result.Details = state.String() + switch state { + case connectivity.Ready, connectivity.Idle: + result.Status = sdcpb.TargetStatus_CONNECTED + default: + result.Status = sdcpb.TargetStatus_NOT_CONNECTED } return result diff --git a/pkg/datastore/target/netconf/nc.go b/pkg/datastore/target/netconf/nc.go index 064eda2b..c2ce441f 100644 --- a/pkg/datastore/target/netconf/nc.go +++ b/pkg/datastore/target/netconf/nc.go @@ -98,8 +98,8 @@ func (t *ncTarget) internalGet(ctx context.Context, req *sdcpb.GetDataRequest) ( log := logf.FromContext(ctx).WithName("Get") ctx = logf.IntoContext(ctx, log) - if !t.Status().IsConnected() { - return nil, fmt.Errorf("%s: %w", t.name, types.ErrNotConnected) + if err := t.Status().Err(); err != nil { + return nil, err } source := "running" @@ -162,8 +162,8 @@ func (t *ncTarget) Get(ctx context.Context, req *sdcpb.GetDataRequest) (*sdcpb.G func (t *ncTarget) Set(ctx context.Context, source types.TargetSource) (*sdcpb.SetDataResponse, error) { log := logf.FromContext(ctx).WithName("Set") ctx = logf.IntoContext(ctx, log) - if !t.Status().IsConnected() { - return nil, fmt.Errorf("%s: %w", t.name, types.ErrNotConnected) + if err := t.Status().Err(); err != nil { + return nil, err } switch t.sbiConfig.NetconfOptions.CommitDatastore { @@ -175,13 +175,13 @@ func (t *ncTarget) Set(ctx context.Context, source types.TargetSource) (*sdcpb.S } func (t *ncTarget) Status() *types.TargetStatus { - result := types.NewTargetStatus(types.TargetStatusNotConnected) + result := types.NewTargetStatus(sdcpb.TargetStatus_NOT_CONNECTED) if t == nil || t.driver == nil { result.Details = "connection not initialized" return result } if t.driver.IsAlive() { - result.Status = types.TargetStatusConnected + result.Status = sdcpb.TargetStatus_CONNECTED } return result } diff --git a/pkg/datastore/target/netconf/status_test.go b/pkg/datastore/target/netconf/status_test.go new file mode 100644 index 00000000..19e429ce --- /dev/null +++ b/pkg/datastore/target/netconf/status_test.go @@ -0,0 +1,53 @@ +// Copyright 2024 Nokia +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package netconf + +import ( + "errors" + "strings" + "testing" + + "github.com/sdcio/data-server/pkg/datastore/target/types" + sdcpb "github.com/sdcio/sdc-protos/sdcpb" +) + +// Get and Set reject requests via Status().Err() before touching any other +// field, so both must survive a nil receiver rather than panicking. +func Test_ncTarget_Status_NilReceiver(t *testing.T) { + var target *ncTarget + + st := target.Status() + if st.Status != sdcpb.TargetStatus_NOT_CONNECTED { + t.Errorf("expected NOT_CONNECTED, got %v", st.Status) + } + if st.IsConnected() { + t.Error("expected a nil target to report not connected") + } + if err := st.Err(); !errors.Is(err, types.ErrNotConnected) { + t.Errorf("expected ErrNotConnected, got %v", err) + } +} + +func Test_ncTarget_Status_UninitializedDriver(t *testing.T) { + target := &ncTarget{name: "dev1"} + + err := target.Status().Err() + if !errors.Is(err, types.ErrNotConnected) { + t.Fatalf("expected ErrNotConnected, got %v", err) + } + if !strings.Contains(err.Error(), "connection not initialized") { + t.Errorf("expected the status details to reach the error, got %q", err.Error()) + } +} diff --git a/pkg/datastore/target/noop/noop.go b/pkg/datastore/target/noop/noop.go index ccc69006..555bb0de 100644 --- a/pkg/datastore/target/noop/noop.go +++ b/pkg/datastore/target/noop/noop.go @@ -96,7 +96,7 @@ func (t *noopTarget) Set(ctx context.Context, source types.TargetSource) (*sdcpb func (t *noopTarget) Status() *types.TargetStatus { return &types.TargetStatus{ - Status: types.TargetStatusConnected, + Status: sdcpb.TargetStatus_CONNECTED, } } diff --git a/pkg/datastore/target/types/targetstatus.go b/pkg/datastore/target/types/targetstatus.go index 6fb903a6..3d386c38 100644 --- a/pkg/datastore/target/types/targetstatus.go +++ b/pkg/datastore/target/types/targetstatus.go @@ -1,28 +1,42 @@ package types -import "errors" +import ( + "errors" + "fmt" + + sdcpb "github.com/sdcio/sdc-protos/sdcpb" +) // ErrNotConnected indicates the southbound interface (device connection) of a // datastore is not established var ErrNotConnected = errors.New("not connected") +// TargetStatus is the southbound connection state of a target. The zero value +// is sdcpb.TargetStatus_UNKNOWN type TargetStatus struct { - Status TargetConnectionStatus + Status sdcpb.TargetStatus Details string } -func NewTargetStatus(status TargetConnectionStatus) *TargetStatus { +func NewTargetStatus(status sdcpb.TargetStatus) *TargetStatus { return &TargetStatus{ Status: status, } } func (ts *TargetStatus) IsConnected() bool { - return ts.Status == TargetStatusConnected + return ts.Status == sdcpb.TargetStatus_CONNECTED } -type TargetConnectionStatus string - -const ( - TargetStatusConnected TargetConnectionStatus = "connected" - TargetStatusNotConnected TargetConnectionStatus = "not connected" -) +// Err reports the connection state as an error, so callers can propagate it and +// match it with errors.Is. It returns nil when the target is connected. This is +// the single place where a connection state turns into ErrNotConnected, keeping +// the Details a target collected attached to the error. +func (ts *TargetStatus) Err() error { + if ts.IsConnected() { + return nil + } + if ts.Details != "" { + return fmt.Errorf("%w: %s", ErrNotConnected, ts.Details) + } + return ErrNotConnected +} diff --git a/pkg/datastore/target/types/targetstatus_test.go b/pkg/datastore/target/types/targetstatus_test.go new file mode 100644 index 00000000..acc41bf2 --- /dev/null +++ b/pkg/datastore/target/types/targetstatus_test.go @@ -0,0 +1,72 @@ +package types + +import ( + "errors" + "strings" + "testing" + + sdcpb "github.com/sdcio/sdc-protos/sdcpb" +) + +func TestTargetStatusErr(t *testing.T) { + cases := map[string]struct { + status *TargetStatus + wantNil bool + wantDetails string + }{ + "connected": { + status: NewTargetStatus(sdcpb.TargetStatus_CONNECTED), + wantNil: true, + }, + "connected with details": { + status: &TargetStatus{Status: sdcpb.TargetStatus_CONNECTED, Details: "READY"}, + wantNil: true, + }, + "not connected": { + status: NewTargetStatus(sdcpb.TargetStatus_NOT_CONNECTED), + }, + "not connected with details": { + status: &TargetStatus{Status: sdcpb.TargetStatus_NOT_CONNECTED, Details: "connection not initialized"}, + wantDetails: "connection not initialized", + }, + "unknown": { + status: NewTargetStatus(sdcpb.TargetStatus_UNKNOWN), + }, + "zero value defaults to unknown": { + status: &TargetStatus{}, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + err := tc.status.Err() + + if tc.wantNil { + if err != nil { + t.Fatalf("expected nil, got %v", err) + } + return + } + + if !errors.Is(err, ErrNotConnected) { + t.Fatalf("expected error matching ErrNotConnected, got %v", err) + } + if tc.wantDetails != "" && !strings.Contains(err.Error(), tc.wantDetails) { + t.Fatalf("expected error to contain %q, got %q", tc.wantDetails, err.Error()) + } + }) + } +} + +// The zero value must not read as connected, otherwise a status that was never +// populated would let a write through to a device. +func TestTargetStatusZeroValueIsNotConnected(t *testing.T) { + var ts TargetStatus + + if ts.Status != sdcpb.TargetStatus_UNKNOWN { + t.Fatalf("expected zero value to be UNKNOWN, got %v", ts.Status) + } + if ts.IsConnected() { + t.Fatal("expected zero value to report not connected") + } +} diff --git a/pkg/server/datastore.go b/pkg/server/datastore.go index ddc43cd7..d7e9de93 100644 --- a/pkg/server/datastore.go +++ b/pkg/server/datastore.go @@ -23,7 +23,6 @@ import ( "github.com/sdcio/data-server/pkg/config" "github.com/sdcio/data-server/pkg/datastore" - targettypes "github.com/sdcio/data-server/pkg/datastore/target/types" "github.com/sdcio/data-server/pkg/utils" logf "github.com/sdcio/logger" sdcpb "github.com/sdcio/sdc-protos/sdcpb" @@ -312,15 +311,9 @@ func (s *Server) datastoreToRsp(ctx context.Context, ds *datastore.Datastore) (* if err != nil { return nil, err } - // map datastore sbi conn state to sdcpb.TargetStatus - switch ds.ConnectionState().Status { - case targettypes.TargetStatusConnected: - rsp.Target.Status = sdcpb.TargetStatus_CONNECTED - case targettypes.TargetStatusNotConnected: - rsp.Target.Status = sdcpb.TargetStatus_NOT_CONNECTED - default: - rsp.Target.Status = sdcpb.TargetStatus_UNKNOWN - } + connState := ds.ConnectionState() + rsp.Target.Status = connState.Status + rsp.Target.StatusDetails = connState.Details rsp.Schema = ds.Config().Schema.GetSchema() return rsp, nil