diff --git a/common_utils/context.go b/common_utils/context.go index 09bff486..b1dd293e 100644 --- a/common_utils/context.go +++ b/common_utils/context.go @@ -54,6 +54,7 @@ const ( DBUS_STOP_SERVICE DBUS_RESTART_SERVICE DBUS_FILE_STAT + DBUS_VALIDATE_YANG DBUS_HALT_SYSTEM COUNTER_SIZE ) diff --git a/gnmi_server/gnoi.go b/gnmi_server/gnoi.go index 4c3aad6c..bb34bf35 100644 --- a/gnmi_server/gnoi.go +++ b/gnmi_server/gnoi.go @@ -2,8 +2,6 @@ package gnmi import ( "context" - "errors" - "os" "strconv" "strings" gnoi_system_pb "github.com/openconfig/gnoi/system" @@ -12,10 +10,8 @@ import ( "time" spb "github.com/sonic-net/sonic-gnmi/proto/gnoi" transutil "github.com/sonic-net/sonic-gnmi/transl_utils" - io "io/ioutil" ssc "github.com/sonic-net/sonic-gnmi/sonic_service_client" spb_jwt "github.com/sonic-net/sonic-gnmi/proto/gnoi/jwt" - "github.com/sonic-net/sonic-gnmi/common_utils" "google.golang.org/grpc/status" "google.golang.org/grpc/codes" "os/user" @@ -153,19 +149,7 @@ func HaltSystem() error { return err } -func RebootSystem(fileName string) error { - log.V(2).Infof("Rebooting with %s...", fileName) - sc, err := ssc.NewDbusClient() - if err != nil { - return err - } - err = sc.ConfigReload(fileName) - return err -} - func (srv *SystemServer) Reboot(ctx context.Context, req *gnoi_system_pb.RebootRequest) (*gnoi_system_pb.RebootResponse, error) { - fileName := common_utils.GNMI_WORK_PATH + "/config_db.json.tmp" - _, err := authenticate(srv.config, ctx) if err != nil { return nil, err @@ -182,18 +166,7 @@ func (srv *SystemServer) Reboot(ctx context.Context, req *gnoi_system_pb.RebootR return nil, err } default: - log.V(1).Info("Reboot system now, delay is ignored...") - // TODO: Support GNOI reboot delay - // Delay in nanoseconds before issuing reboot. - // https://github.com/openconfig/gnoi/blob/master/system/system.proto#L102-L115 - config_db_json, err := io.ReadFile(fileName) - if errors.Is(err, os.ErrNotExist) { - fileName = "" - } - err = RebootSystem(string(config_db_json)) - if err != nil { - return nil, err - } + return nil, status.Errorf(codes.Unimplemented, "") } var resp gnoi_system_pb.RebootResponse diff --git a/gnmi_server/server_test.go b/gnmi_server/server_test.go index bee65d08..cfa899ef 100644 --- a/gnmi_server/server_test.go +++ b/gnmi_server/server_test.go @@ -3839,14 +3839,6 @@ func TestRecoverFromJSONSerializationPanic(t *testing.T) { } func TestGnmiSetBatch(t *testing.T) { - mockCode := - ` -print('No Yang validation for test mode...') -print('%s') -` - mock1 := gomonkey.ApplyGlobalVar(&sdc.PyCodeForYang, mockCode) - defer mock1.Reset() - sdcfg.Init() s := createServer(t, 8090) go runServer(t, s) @@ -3925,8 +3917,6 @@ func TestGNMINative(t *testing.T) { return &dbus.Call{} }) defer mock2.Reset() - mock3 := gomonkey.ApplyFunc(sdc.RunPyCode, func(text string) error {return nil}) - defer mock3.Reset() sdcfg.Init() s := createServer(t, 8080) diff --git a/sonic_data_client/mixed_db_client.go b/sonic_data_client/mixed_db_client.go index 4fe1bb22..bcce2b32 100644 --- a/sonic_data_client/mixed_db_client.go +++ b/sonic_data_client/mixed_db_client.go @@ -1,9 +1,5 @@ package client -// #cgo pkg-config: python3-embed -// #include -import "C" - import ( "bytes" "encoding/json" @@ -16,7 +12,6 @@ import ( "strings" "sync" "time" - "unsafe" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" log "github.com/golang/glog" @@ -1177,37 +1172,6 @@ func (c *MixedDbClient) ConvertToJsonPatch(prefix *gnmipb.Path, path *gnmipb.Pat return nil } -func RunPyCode(text string) error { - defer C.Py_Finalize() - C.Py_Initialize() - PyCodeInC := C.CString(text) - defer C.free(unsafe.Pointer(PyCodeInC)) - CRet := C.PyRun_SimpleString(PyCodeInC) - if int(CRet) != 0 { - return fmt.Errorf("Python failure") - } - return nil -} - -var PyCodeForYang string = -` -import sonic_yang -import json - -yang_parser = sonic_yang.SonicYang("/usr/local/yang-models") -yang_parser.loadYangModel() -filename = "%s" -with open(filename, 'r') as fp: - text = fp.read() - - try: - yang_parser.loadData(configdbJson=json.loads(text)) - yang_parser.validate_data_tree() - except sonic_yang.SonicYangException as e: - print("Yang validation error: {}".format(str(e))) - raise -` - func (c *MixedDbClient) SetIncrementalConfig(delete []*gnmipb.Path, replace []*gnmipb.Update, update []*gnmipb.Update) error { var err error @@ -1362,17 +1326,28 @@ func (c *MixedDbClient) SetFullConfig(delete []*gnmipb.Path, replace []*gnmipb.U if len(ietf_json_val) == 0 { return fmt.Errorf("Value encoding is not IETF JSON") } - content := []byte(ietf_json_val) - fileName := c.workPath + "/config_db.json.tmp" - err := ioutil.WriteFile(fileName, content, 0644) + + var sc ssc.Service + sc, err := ssc.NewDbusClient() if err != nil { return err } - - PyCodeInGo := fmt.Sprintf(PyCodeForYang, fileName) - err = RunPyCode(PyCodeInGo) + log.V(2).Infof("Run yang validation") + err = sc.ValidateYang(string(ietf_json_val)) + if err != nil { + return err + } + log.V(2).Infof("Run config reload") + err = sc.ConfigReload(string(ietf_json_val), "gnmi") if err != nil { - return fmt.Errorf("Yang validation failed!") + log.V(2).Infof("Recover default config") + sc.ConfigReloadForce("", "gnmi") + return err + } + log.V(2).Infof("Run config save") + err = sc.ConfigSave("") + if err != nil { + return err } return nil diff --git a/sonic_service_client/dbus_client.go b/sonic_service_client/dbus_client.go index 51777099..b196001c 100644 --- a/sonic_service_client/dbus_client.go +++ b/sonic_service_client/dbus_client.go @@ -10,7 +10,8 @@ import ( ) type Service interface { - ConfigReload(fileName string) error + ConfigReload(config string, caller string) error + ConfigReloadForce(config string, caller string) error ConfigSave(fileName string) error ApplyPatchYang(fileName string) error ApplyPatchDb(fileName string) error @@ -19,6 +20,7 @@ type Service interface { StopService(service string) error RestartService(service string) error GetFileStat(path string) (map[string]string, error) + ValidateYang(config string) error HaltSystem() error } @@ -99,13 +101,23 @@ func DbusApi(busName string, busPath string, intName string, timeout int, args . } } -func (c *DbusClient) ConfigReload(config string) error { +func (c *DbusClient) ConfigReload(config string, caller string) error { common_utils.IncCounter(common_utils.DBUS_CONFIG_RELOAD) modName := "config" busName := c.busNamePrefix + modName busPath := c.busPathPrefix + modName intName := c.intNamePrefix + modName + ".reload" - _, err := DbusApi(busName, busPath, intName, 60, config) + _, err := DbusApi(busName, busPath, intName, 240, config, caller) + return err +} + +func (c *DbusClient) ConfigReloadForce(config string, caller string) error { + common_utils.IncCounter(common_utils.DBUS_CONFIG_RELOAD) + modName := "config" + busName := c.busNamePrefix + modName + busPath := c.busPathPrefix + modName + intName := c.intNamePrefix + modName + ".reload_force" + _, err := DbusApi(busName, busPath, intName, 240, config, caller) return err } @@ -193,6 +205,16 @@ func (c *DbusClient) GetFileStat(path string) (map[string]string, error) { return data, nil } +func (c *DbusClient) ValidateYang(config string) error { + common_utils.IncCounter(common_utils.DBUS_VALIDATE_YANG) + modName := "yang" + busName := c.busNamePrefix + modName + busPath := c.busPathPrefix + modName + intName := c.intNamePrefix + modName + ".validate" + _, err := DbusApi(busName, busPath, intName, 60, config) + return err +} + func (c *DbusClient) HaltSystem() error { // Increment the counter for the DBUS_HALT_SYSTEM event common_utils.IncCounter(common_utils.DBUS_HALT_SYSTEM) diff --git a/sonic_service_client/dbus_client_test.go b/sonic_service_client/dbus_client_test.go index aae99608..33f90d6e 100644 --- a/sonic_service_client/dbus_client_test.go +++ b/sonic_service_client/dbus_client_test.go @@ -13,7 +13,7 @@ func TestSystemBusNegative(t *testing.T) { if err != nil { t.Errorf("NewDbusClient failed: %v", err) } - err = client.ConfigReload("abc") + err = client.ConfigReload("abc", "") if err == nil { t.Errorf("SystemBus should fail") } @@ -121,7 +121,7 @@ func TestConfigReload(t *testing.T) { if err != nil { t.Errorf("NewDbusClient failed: %v", err) } - err = client.ConfigReload("abc") + err = client.ConfigReload("abc", "") if err != nil { t.Errorf("ConfigReload should pass: %v", err) } @@ -151,7 +151,7 @@ func TestConfigReloadNegative(t *testing.T) { if err != nil { t.Errorf("NewDbusClient failed: %v", err) } - err = client.ConfigReload("abc") + err = client.ConfigReload("abc", "") if err == nil { t.Errorf("ConfigReload should fail") } @@ -160,15 +160,20 @@ func TestConfigReloadNegative(t *testing.T) { } } -func TestConfigReloadTimeout(t *testing.T) { +func TestConfigReloadForce(t *testing.T) { mock1 := gomonkey.ApplyFunc(dbus.SystemBus, func() (conn *dbus.Conn, err error) { return &dbus.Conn{}, nil }) defer mock1.Reset() mock2 := gomonkey.ApplyMethod(reflect.TypeOf(&dbus.Object{}), "Go", func(obj *dbus.Object, method string, flags dbus.Flags, ch chan *dbus.Call, args ...interface{}) *dbus.Call { - if method != "org.SONiC.HostService.config.reload" { + if method != "org.SONiC.HostService.config.reload_force" { t.Errorf("Wrong method: %v", method) } + ret := &dbus.Call{} + ret.Err = nil + ret.Body = make([]interface{}, 2) + ret.Body[0] = int32(0) + ch <- ret return &dbus.Call{} }) defer mock2.Reset() @@ -177,9 +182,42 @@ func TestConfigReloadTimeout(t *testing.T) { if err != nil { t.Errorf("NewDbusClient failed: %v", err) } - err = client.ConfigReload("abc") + err = client.ConfigReloadForce("abc", "") + if err != nil { + t.Errorf("ConfigReload should pass: %v", err) + } +} + +func TestConfigReloadForceNegative(t *testing.T) { + err_msg := "This is the mock error message" + mock1 := gomonkey.ApplyFunc(dbus.SystemBus, func() (conn *dbus.Conn, err error) { + return &dbus.Conn{}, nil + }) + defer mock1.Reset() + mock2 := gomonkey.ApplyMethod(reflect.TypeOf(&dbus.Object{}), "Go", func(obj *dbus.Object, method string, flags dbus.Flags, ch chan *dbus.Call, args ...interface{}) *dbus.Call { + if method != "org.SONiC.HostService.config.reload_force" { + t.Errorf("Wrong method: %v", method) + } + ret := &dbus.Call{} + ret.Err = nil + ret.Body = make([]interface{}, 2) + ret.Body[0] = int32(1) + ret.Body[1] = err_msg + ch <- ret + return &dbus.Call{} + }) + defer mock2.Reset() + + client, err := NewDbusClient() + if err != nil { + t.Errorf("NewDbusClient failed: %v", err) + } + err = client.ConfigReloadForce("abc", "") if err == nil { - t.Errorf("ConfigReload should timeout: %v", err) + t.Errorf("ConfigReload should fail") + } + if err.Error() != err_msg { + t.Errorf("Wrong error: %v", err) } } @@ -244,6 +282,29 @@ func TestConfigSaveNegative(t *testing.T) { } } +func TestConfigSaveTimeout(t *testing.T) { + mock1 := gomonkey.ApplyFunc(dbus.SystemBus, func() (conn *dbus.Conn, err error) { + return &dbus.Conn{}, nil + }) + defer mock1.Reset() + mock2 := gomonkey.ApplyMethod(reflect.TypeOf(&dbus.Object{}), "Go", func(obj *dbus.Object, method string, flags dbus.Flags, ch chan *dbus.Call, args ...interface{}) *dbus.Call { + if method != "org.SONiC.HostService.config.save" { + t.Errorf("Wrong method: %v", method) + } + return &dbus.Call{} + }) + defer mock2.Reset() + + client, err := NewDbusClient() + if err != nil { + t.Errorf("NewDbusClient failed: %v", err) + } + err = client.ConfigSave("") + if err == nil { + t.Errorf("ConfigSave should timeout: %v", err) + } +} + func TestApplyPatchYang(t *testing.T) { mock1 := gomonkey.ApplyFunc(dbus.SystemBus, func() (conn *dbus.Conn, err error) { return &dbus.Conn{}, nil @@ -487,3 +548,64 @@ func TestDeleteCheckPointNegative(t *testing.T) { t.Errorf("Wrong error: %v", err) } } + +func TestValidateYang(t *testing.T) { + mock1 := gomonkey.ApplyFunc(dbus.SystemBus, func() (conn *dbus.Conn, err error) { + return &dbus.Conn{}, nil + }) + defer mock1.Reset() + mock2 := gomonkey.ApplyMethod(reflect.TypeOf(&dbus.Object{}), "Go", func(obj *dbus.Object, method string, flags dbus.Flags, ch chan *dbus.Call, args ...interface{}) *dbus.Call { + if method != "org.SONiC.HostService.yang.validate" { + t.Errorf("Wrong method: %v", method) + } + ret := &dbus.Call{} + ret.Err = nil + ret.Body = make([]interface{}, 2) + ret.Body[0] = int32(0) + ch <- ret + return &dbus.Call{} + }) + defer mock2.Reset() + + client, err := NewDbusClient() + if err != nil { + t.Errorf("NewDbusClient failed: %v", err) + } + err = client.ValidateYang("abc") + if err != nil { + t.Errorf("ValidateYang should pass: %v", err) + } +} + +func TestValidateYangNegative(t *testing.T) { + err_msg := "This is the mock error message" + mock1 := gomonkey.ApplyFunc(dbus.SystemBus, func() (conn *dbus.Conn, err error) { + return &dbus.Conn{}, nil + }) + defer mock1.Reset() + mock2 := gomonkey.ApplyMethod(reflect.TypeOf(&dbus.Object{}), "Go", func(obj *dbus.Object, method string, flags dbus.Flags, ch chan *dbus.Call, args ...interface{}) *dbus.Call { + if method != "org.SONiC.HostService.yang.validate" { + t.Errorf("Wrong method: %v", method) + } + ret := &dbus.Call{} + ret.Err = nil + ret.Body = make([]interface{}, 2) + ret.Body[0] = int32(1) + ret.Body[1] = err_msg + ch <- ret + return &dbus.Call{} + }) + defer mock2.Reset() + + client, err := NewDbusClient() + if err != nil { + t.Errorf("NewDbusClient failed: %v", err) + } + err = client.ValidateYang("abc") + if err == nil { + t.Errorf("ValidateYang should fail") + } + if err.Error() != err_msg { + t.Errorf("Wrong error: %v", err) + } +} diff --git a/test/test_gnmi_configdb.py b/test/test_gnmi_configdb.py index d933faff..033a3096 100644 --- a/test/test_gnmi_configdb.py +++ b/test/test_gnmi_configdb.py @@ -286,10 +286,6 @@ def test_gnmi_full(self): ret, msg = gnmi_set(delete_list, update_list, []) assert ret == 0, msg - assert os.path.exists(config_file), "No config file" - with open(config_file,'r') as cf: - config_json = json.load(cf) - assert test_data == config_json, "Wrong config file" def test_gnmi_full_negative(self): delete_list = ['/sonic-db:CONFIG_DB/localhost/'] diff --git a/test/test_gnoi.py b/test/test_gnoi.py index f8ceccf3..4d9076b3 100644 --- a/test/test_gnoi.py +++ b/test/test_gnoi.py @@ -10,17 +10,6 @@ def test_gnoi_time(self): assert ret == 0, msg assert 'time' in msg, 'Invalid response: %s'%msg - def test_gnoi_reboot(self): - ret, old_cnt = gnmi_dump('DBUS config reload') - assert ret == 0, 'Fail to read counter' - - ret, msg = gnoi_reboot(1, 0, 'Test reboot') - assert ret == 0, msg - - ret, new_cnt = gnmi_dump('DBUS config reload') - assert ret == 0, 'Fail to read counter' - assert new_cnt == old_cnt+1, 'DBUS API is not invoked' - def test_gnoi_reboot_halt(self): ret, old_cnt = gnmi_dump('DBUS halt system') assert ret == 0, 'Fail to read counter'