From bba4eb4d813a0d48836601eedd6af944ce821986 Mon Sep 17 00:00:00 2001 From: Benjamin Wang Date: Mon, 15 Aug 2022 16:59:39 +0800 Subject: [PATCH] Improve functional test code Get duplicated code wraped in common functions, and simplify error handling. Signed-off-by: Benjamin Wang --- tests/functional/agent/handler.go | 150 ++++++------------ tests/functional/agent/server.go | 6 - tests/functional/agent/utils.go | 24 +++ .../cmd/etcd-tester/etcd_tester_test.go | 6 +- 4 files changed, 73 insertions(+), 113 deletions(-) diff --git a/tests/functional/agent/handler.go b/tests/functional/agent/handler.go index 7909d67bd2b..057d539dc8c 100644 --- a/tests/functional/agent/handler.go +++ b/tests/functional/agent/handler.go @@ -86,8 +86,7 @@ func (srv *Server) handleTesterRequest(req *rpcpb.Request) (resp *rpcpb.Response // just archive the first file func (srv *Server) createEtcdLogFile() error { var err error - srv.etcdLogFile, err = os.Create(srv.Member.Etcd.LogOutputs[0]) - if err != nil { + if srv.etcdLogFile, err = os.Create(srv.Member.Etcd.LogOutputs[0]); err != nil { return err } srv.lg.Info("created etcd log file", zap.String("path", srv.Member.Etcd.LogOutputs[0])) @@ -172,8 +171,7 @@ func (srv *Server) stopEtcd(sig os.Signal) error { zap.String("signal", sig.String()), ) - err := srv.etcdCmd.Process.Signal(sig) - if err != nil { + if err := srv.etcdCmd.Process.Signal(sig); err != nil { return err } @@ -191,7 +189,7 @@ func (srv *Server) stopEtcd(sig os.Signal) error { return e } - err = <-errc + err := <-errc srv.lg.Info( "stopped etcd command", @@ -306,29 +304,16 @@ func (srv *Server) stopProxy() { // if started with manual TLS, stores TLS assets // from tester/client to disk before starting etcd process func (srv *Server) saveTLSAssets() error { - if srv.Member.PeerCertPath != "" { - if srv.Member.PeerCertData == "" { - return fmt.Errorf("got empty data for %q", srv.Member.PeerCertPath) - } - if err := os.WriteFile(srv.Member.PeerCertPath, []byte(srv.Member.PeerCertData), 0644); err != nil { - return err - } + const defaultFileMode os.FileMode = 0644 + + if err := safeDataToFile(srv.Member.PeerCertPath, []byte(srv.Member.PeerCertData), defaultFileMode); err != nil { + return err } - if srv.Member.PeerKeyPath != "" { - if srv.Member.PeerKeyData == "" { - return fmt.Errorf("got empty data for %q", srv.Member.PeerKeyPath) - } - if err := os.WriteFile(srv.Member.PeerKeyPath, []byte(srv.Member.PeerKeyData), 0644); err != nil { - return err - } + if err := safeDataToFile(srv.Member.PeerKeyPath, []byte(srv.Member.PeerKeyData), defaultFileMode); err != nil { + return err } - if srv.Member.PeerTrustedCAPath != "" { - if srv.Member.PeerTrustedCAData == "" { - return fmt.Errorf("got empty data for %q", srv.Member.PeerTrustedCAPath) - } - if err := os.WriteFile(srv.Member.PeerTrustedCAPath, []byte(srv.Member.PeerTrustedCAData), 0644); err != nil { - return err - } + if err := safeDataToFile(srv.Member.PeerTrustedCAPath, []byte(srv.Member.PeerTrustedCAData), defaultFileMode); err != nil { + return err } if srv.Member.PeerCertPath != "" && srv.Member.PeerKeyPath != "" && @@ -341,29 +326,14 @@ func (srv *Server) saveTLSAssets() error { ) } - if srv.Member.ClientCertPath != "" { - if srv.Member.ClientCertData == "" { - return fmt.Errorf("got empty data for %q", srv.Member.ClientCertPath) - } - if err := os.WriteFile(srv.Member.ClientCertPath, []byte(srv.Member.ClientCertData), 0644); err != nil { - return err - } + if err := safeDataToFile(srv.Member.ClientCertPath, []byte(srv.Member.ClientCertData), defaultFileMode); err != nil { + return err } - if srv.Member.ClientKeyPath != "" { - if srv.Member.ClientKeyData == "" { - return fmt.Errorf("got empty data for %q", srv.Member.ClientKeyPath) - } - if err := os.WriteFile(srv.Member.ClientKeyPath, []byte(srv.Member.ClientKeyData), 0644); err != nil { - return err - } + if err := safeDataToFile(srv.Member.ClientKeyPath, []byte(srv.Member.ClientKeyData), defaultFileMode); err != nil { + return err } - if srv.Member.ClientTrustedCAPath != "" { - if srv.Member.ClientTrustedCAData == "" { - return fmt.Errorf("got empty data for %q", srv.Member.ClientTrustedCAPath) - } - if err := os.WriteFile(srv.Member.ClientTrustedCAPath, []byte(srv.Member.ClientTrustedCAData), 0644); err != nil { - return err - } + if err := safeDataToFile(srv.Member.ClientTrustedCAPath, []byte(srv.Member.ClientTrustedCAData), defaultFileMode); err != nil { + return err } if srv.Member.ClientCertPath != "" && srv.Member.ClientKeyPath != "" && @@ -390,24 +360,18 @@ func (srv *Server) loadAutoTLSAssets() error { zap.String("dir", fdir), zap.String("endpoint", srv.EtcdClientEndpoint), ) - + // load peer cert.pem certPath := filepath.Join(fdir, "cert.pem") - if !fileutil.Exist(certPath) { - return fmt.Errorf("cannot find %q", certPath) - } - certData, err := os.ReadFile(certPath) + certData, err := loadFileData(certPath) if err != nil { - return fmt.Errorf("cannot read %q (%v)", certPath, err) + return err } srv.Member.PeerCertData = string(certData) - + // load peer key.pem keyPath := filepath.Join(fdir, "key.pem") - if !fileutil.Exist(keyPath) { - return fmt.Errorf("cannot find %q", keyPath) - } - keyData, err := os.ReadFile(keyPath) + keyData, err := loadFileData(keyPath) if err != nil { - return fmt.Errorf("cannot read %q (%v)", keyPath, err) + return err } srv.Member.PeerKeyData = string(keyData) @@ -431,24 +395,18 @@ func (srv *Server) loadAutoTLSAssets() error { zap.String("dir", fdir), zap.String("endpoint", srv.EtcdClientEndpoint), ) - + // load client cert.pem certPath := filepath.Join(fdir, "cert.pem") - if !fileutil.Exist(certPath) { - return fmt.Errorf("cannot find %q", certPath) - } - certData, err := os.ReadFile(certPath) + certData, err := loadFileData(certPath) if err != nil { - return fmt.Errorf("cannot read %q (%v)", certPath, err) + return err } srv.Member.ClientCertData = string(certData) - + // load client key.pem keyPath := filepath.Join(fdir, "key.pem") - if !fileutil.Exist(keyPath) { - return fmt.Errorf("cannot find %q", keyPath) - } - keyData, err := os.ReadFile(keyPath) + keyData, err := loadFileData(keyPath) if err != nil { - return fmt.Errorf("cannot read %q (%v)", keyPath, err) + return err } srv.Member.ClientKeyData = string(keyData) @@ -473,28 +431,27 @@ func (srv *Server) handle_INITIAL_START_ETCD(req *rpcpb.Request) (*rpcpb.Respons }, nil } - err := fileutil.TouchDirAll(srv.lg, srv.Member.BaseDir) - if err != nil { + if err := fileutil.TouchDirAll(srv.lg, srv.Member.BaseDir); err != nil { return nil, err } srv.lg.Info("created base directory", zap.String("path", srv.Member.BaseDir)) if srv.etcdServer == nil { - if err = srv.createEtcdLogFile(); err != nil { + if err := srv.createEtcdLogFile(); err != nil { return nil, err } } - if err = srv.saveTLSAssets(); err != nil { + if err := srv.saveTLSAssets(); err != nil { return nil, err } - if err = srv.createEtcd(false, req.Member.Failpoints); err != nil { + if err := srv.createEtcd(false, req.Member.Failpoints); err != nil { return nil, err } - if err = srv.runEtcd(); err != nil { + if err := srv.runEtcd(); err != nil { return nil, err } - if err = srv.loadAutoTLSAssets(); err != nil { + if err := srv.loadAutoTLSAssets(); err != nil { return nil, err } @@ -508,8 +465,7 @@ func (srv *Server) handle_INITIAL_START_ETCD(req *rpcpb.Request) (*rpcpb.Respons func (srv *Server) handle_RESTART_ETCD(req *rpcpb.Request) (*rpcpb.Response, error) { var err error if !fileutil.Exist(srv.Member.BaseDir) { - err = fileutil.TouchDirAll(srv.lg, srv.Member.BaseDir) - if err != nil { + if err = fileutil.TouchDirAll(srv.lg, srv.Member.BaseDir); err != nil { return nil, err } } @@ -552,8 +508,7 @@ func (srv *Server) handle_SIGTERM_ETCD() (*rpcpb.Response, error) { } func (srv *Server) handle_SIGQUIT_ETCD_AND_REMOVE_DATA() (*rpcpb.Response, error) { - err := srv.stopEtcd(syscall.SIGQUIT) - if err != nil { + if err := srv.stopEtcd(syscall.SIGQUIT); err != nil { return nil, err } @@ -565,10 +520,10 @@ func (srv *Server) handle_SIGQUIT_ETCD_AND_REMOVE_DATA() (*rpcpb.Response, error } // for debugging purposes, rename instead of removing - if err = os.RemoveAll(srv.Member.BaseDir + ".backup"); err != nil { + if err := os.RemoveAll(srv.Member.BaseDir + ".backup"); err != nil { return nil, err } - if err = os.Rename(srv.Member.BaseDir, srv.Member.BaseDir+".backup"); err != nil { + if err := os.Rename(srv.Member.BaseDir, srv.Member.BaseDir+".backup"); err != nil { return nil, err } srv.lg.Info( @@ -579,8 +534,7 @@ func (srv *Server) handle_SIGQUIT_ETCD_AND_REMOVE_DATA() (*rpcpb.Response, error // create a new log file for next new member restart if !fileutil.Exist(srv.Member.BaseDir) { - err = fileutil.TouchDirAll(srv.lg, srv.Member.BaseDir) - if err != nil { + if err := fileutil.TouchDirAll(srv.lg, srv.Member.BaseDir); err != nil { return nil, err } } @@ -592,8 +546,7 @@ func (srv *Server) handle_SIGQUIT_ETCD_AND_REMOVE_DATA() (*rpcpb.Response, error } func (srv *Server) handle_SAVE_SNAPSHOT() (*rpcpb.Response, error) { - err := srv.Member.SaveSnapshot(srv.lg) - if err != nil { + if err := srv.Member.SaveSnapshot(srv.lg); err != nil { return nil, err } return &rpcpb.Response{ @@ -604,8 +557,7 @@ func (srv *Server) handle_SAVE_SNAPSHOT() (*rpcpb.Response, error) { } func (srv *Server) handle_RESTORE_RESTART_FROM_SNAPSHOT(req *rpcpb.Request) (resp *rpcpb.Response, err error) { - err = srv.Member.RestoreSnapshot(srv.lg) - if err != nil { + if err = srv.Member.RestoreSnapshot(srv.lg); err != nil { return nil, err } resp, err = srv.handle_RESTART_FROM_SNAPSHOT(req) @@ -637,8 +589,7 @@ func (srv *Server) handle_RESTART_FROM_SNAPSHOT(req *rpcpb.Request) (resp *rpcpb } func (srv *Server) handle_SIGQUIT_ETCD_AND_ARCHIVE_DATA() (*rpcpb.Response, error) { - err := srv.stopEtcd(syscall.SIGQUIT) - if err != nil { + if err := srv.stopEtcd(syscall.SIGQUIT); err != nil { return nil, err } @@ -650,18 +601,13 @@ func (srv *Server) handle_SIGQUIT_ETCD_AND_ARCHIVE_DATA() (*rpcpb.Response, erro } // TODO: support separate WAL directory - if err = archive( - srv.lg, - srv.Member.BaseDir, - srv.Member.Etcd.LogOutputs[0], - srv.Member.Etcd.DataDir, - ); err != nil { + if err := archive(srv.lg, srv.Member.BaseDir, srv.Member.Etcd.LogOutputs[0], srv.Member.Etcd.DataDir); err != nil { return nil, err } srv.lg.Info("archived data", zap.String("base-dir", srv.Member.BaseDir)) if srv.etcdServer == nil { - if err = srv.createEtcdLogFile(); err != nil { + if err := srv.createEtcdLogFile(); err != nil { return nil, err } } @@ -681,8 +627,7 @@ func (srv *Server) handle_SIGQUIT_ETCD_AND_ARCHIVE_DATA() (*rpcpb.Response, erro // stop proxy, etcd, delete data directory func (srv *Server) handle_SIGQUIT_ETCD_AND_REMOVE_DATA_AND_STOP_AGENT() (*rpcpb.Response, error) { - err := srv.stopEtcd(syscall.SIGQUIT) - if err != nil { + if err := srv.stopEtcd(syscall.SIGQUIT); err != nil { return nil, err } @@ -693,8 +638,7 @@ func (srv *Server) handle_SIGQUIT_ETCD_AND_REMOVE_DATA_AND_STOP_AGENT() (*rpcpb. srv.etcdLogFile.Close() } - err = os.RemoveAll(srv.Member.BaseDir) - if err != nil { + if err := os.RemoveAll(srv.Member.BaseDir); err != nil { return nil, err } srv.lg.Info("removed base directory", zap.String("dir", srv.Member.BaseDir)) diff --git a/tests/functional/agent/server.go b/tests/functional/agent/server.go index bfd5e018d5c..81bdec5ab20 100644 --- a/tests/functional/agent/server.go +++ b/tests/functional/agent/server.go @@ -138,12 +138,6 @@ func (srv *Server) Transport(stream rpcpb.Transport_TransportServer) (reterr err // TODO: handle error and retry return } - if req.Member != nil { - srv.Member = req.Member - } - if req.Tester != nil { - srv.Tester = req.Tester - } var resp *rpcpb.Response resp, err = srv.handleTesterRequest(req) diff --git a/tests/functional/agent/utils.go b/tests/functional/agent/utils.go index 2cd888ed0e0..16931a2cbf9 100644 --- a/tests/functional/agent/utils.go +++ b/tests/functional/agent/utils.go @@ -15,6 +15,7 @@ package agent import ( + "fmt" "io" "net" "net/url" @@ -102,6 +103,29 @@ func copyFile(src, dst string) error { return w.Sync() } +func safeDataToFile(filePath string, fileData []byte, mode os.FileMode) error { + if filePath != "" { + if len(fileData) == 0 { + return fmt.Errorf("got empty data for %q", filePath) + } + if err := os.WriteFile(filePath, fileData, mode); err != nil { + return fmt.Errorf("writing file %q failed, %w", filePath, err) + } + } + return nil +} + +func loadFileData(filePath string) ([]byte, error) { + if !fileutil.Exist(filePath) { + return nil, fmt.Errorf("cannot find %q", filePath) + } + data, err := os.ReadFile(filePath) + if err != nil { + return nil, fmt.Errorf("read file %q failed, %w", filePath, err) + } + return data, nil +} + func cleanPageCache() error { // https://www.kernel.org/doc/Documentation/sysctl/vm.txt // https://github.com/torvalds/linux/blob/master/fs/drop_caches.c diff --git a/tests/functional/cmd/etcd-tester/etcd_tester_test.go b/tests/functional/cmd/etcd-tester/etcd_tester_test.go index 0e1bd029a2f..39cdd830363 100644 --- a/tests/functional/cmd/etcd-tester/etcd_tester_test.go +++ b/tests/functional/cmd/etcd-tester/etcd_tester_test.go @@ -38,15 +38,13 @@ func TestFunctional(t *testing.T) { t.Fatalf("failed to create a cluster: %v", err) } - err = clus.Send_INITIAL_START_ETCD() - if err != nil { + if err = clus.Send_INITIAL_START_ETCD(); err != nil { t.Fatal("Bootstrap failed", zap.Error(err)) } defer clus.Send_SIGQUIT_ETCD_AND_REMOVE_DATA_AND_STOP_AGENT() t.Log("wait health after bootstrap") - err = clus.WaitHealth() - if err != nil { + if err = clus.WaitHealth(); err != nil { t.Fatal("WaitHealth failed", zap.Error(err)) }