From e1845eb168a7db0708e3ba307904d666847ea00c Mon Sep 17 00:00:00 2001 From: Calvin Chan Date: Tue, 28 Nov 2023 19:08:00 -0800 Subject: [PATCH 1/4] Add utils for service under SansShell services --- services/service/client/client.go | 71 ++++++++++++++++--------------- services/service/client/utils.go | 67 +++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 34 deletions(-) diff --git a/services/service/client/client.go b/services/service/client/client.go index 1dc87358..96054968 100644 --- a/services/service/client/client.go +++ b/services/service/client/client.go @@ -218,19 +218,22 @@ func (s *statusCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interf return subcommands.ExitUsageError } - req := &pb.StatusRequest{ - SystemType: system, - ServiceName: serviceName, - } - c := pb.NewServiceClientProxy(state.Conn) + err = StopManyRemoteService(ctx, state.Conn, system, serviceName) - respChan, err := c.StatusOneMany(ctx, req) + // Comment out the following code temporarily for testing utils + //req := &pb.StatusRequest{ + // SystemType: system, + // ServiceName: serviceName, + //} + // + //c := pb.NewServiceClientProxy(state.Conn) + //respChan, err := c.StatusOneMany(ctx, req) if err != nil { // Emit this to every error file as it's not specific to a given target. - for _, e := range state.Err { - fmt.Fprintf(e, "All targets - error executing 'status' for service %s: %v\n", serviceName, err) - } + //for _, e := range state.Err { + // fmt.Fprintf(e, "All targets - error executing 'status' for service %s: %v\n", serviceName, err) + //} return subcommands.ExitFailure } @@ -241,24 +244,24 @@ func (s *statusCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interf // return early here. // Note that this is only the last non-nil error, and previous // error values may be lost. - var lastErr error - for resp := range respChan { - out := state.Out[resp.Index] - system, status := resp.Resp.GetSystemType(), resp.Resp.GetServiceStatus().GetStatus() - output := fmt.Sprintf("[%s] %s : %s", systemTypeString(system), serviceName, statusString(status)) - if resp.Error != nil { - lastErr = fmt.Errorf("target %s [%d] error: %w\n", resp.Target, resp.Index, resp.Error) - fmt.Fprint(state.Err[resp.Index], lastErr) - continue - } - if _, err := fmt.Fprintln(out, output); err != nil { - lastErr = fmt.Errorf("target %s [%d] write error: %w\n", resp.Target, resp.Index, err) - fmt.Fprint(state.Err[resp.Index], lastErr) - } - } - if lastErr != nil { - return subcommands.ExitFailure - } + //var lastErr error + //for resp := range respChan { + // out := state.Out[resp.Index] + // system, status := resp.Resp.GetSystemType(), resp.Resp.GetServiceStatus().GetStatus() + // output := fmt.Sprintf("[%s] %s : %s", systemTypeString(system), serviceName, statusString(status)) + // if resp.Error != nil { + // lastErr = fmt.Errorf("target %s [%d] error: %w\n", resp.Target, resp.Index, resp.Error) + // fmt.Fprint(state.Err[resp.Index], lastErr) + // continue + // } + // if _, err := fmt.Fprintln(out, output); err != nil { + // lastErr = fmt.Errorf("target %s [%d] write error: %w\n", resp.Target, resp.Index, err) + // fmt.Fprint(state.Err[resp.Index], lastErr) + // } + //} + //if lastErr != nil { + // return subcommands.ExitFailure + //} return subcommands.ExitSuccess } @@ -303,13 +306,13 @@ func (l *listCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interfac return subcommands.ExitFailure } - // Error holding the last observed non-nil error, which will - // determine the exit status of the command. - // The contract with the proxy and 'many' functions requires - // that we completely drain the response channel, so we cannot - // return early here. - // Note that this is only the last non-nil error, and previous - // error values may be lost. + // // Error holding the last observed non-nil error, which will + // // determine the exit status of the command. + // // The contract with the proxy and 'many' functions requires + // // that we completely drain the response channel, so we cannot + // // return early here. + // // Note that this is only the last non-nil error, and previous + // // error values may be lost. var lastErr error for resp := range respChan { out := state.Out[resp.Index] diff --git a/services/service/client/utils.go b/services/service/client/utils.go index beaf0633..1327d7fa 100644 --- a/services/service/client/utils.go +++ b/services/service/client/utils.go @@ -20,6 +20,7 @@ import ( "context" "errors" "fmt" + "strings" "github.com/Snowflake-Labs/sansshell/proxy/proxy" pb "github.com/Snowflake-Labs/sansshell/services/service" @@ -96,6 +97,72 @@ func StopRemoteService(ctx context.Context, conn *proxy.Conn, system pb.SystemTy return nil } +// StartManyRemoteServices is a helper function for starting a service on multiple remote targets. +func StartManyRemoteServices(ctx context.Context, conn *proxy.Conn, system pb.SystemType, service string) error { + c := pb.NewServiceClientProxy(conn) + respChan, err := c.ActionOneMany(ctx, &pb.ActionRequest{ + ServiceName: service, + SystemType: system, + Action: pb.Action_ACTION_START, + }) + + if err != nil { + return fmt.Errorf("can't start service %s - %v", service, err) + } + + var errorList []error + for resp := range respChan { + if resp.Error != nil { + err := fmt.Errorf("can't start service on target %s: %w", resp.Target, resp.Error) + errorList = append(errorList, err) + } + } + + if len(errorList) == 0 { + return nil + } + + var errorMessages []string + for _, err := range errorList { + errorMessages = append(errorMessages, err.Error()) + } + + return fmt.Errorf("%s", strings.Join(errorMessages, "\n")) +} + +// StopManyRemoteService is a helper function for stopping a service on multiple remote targets. +func StopManyRemoteService(ctx context.Context, conn *proxy.Conn, system pb.SystemType, service string) error { + c := pb.NewServiceClientProxy(conn) + respChan, err := c.ActionOneMany(ctx, &pb.ActionRequest{ + ServiceName: service, + SystemType: system, + Action: pb.Action_ACTION_STOP, + }) + + if err != nil { + return fmt.Errorf("can't stop service %s - %v", service, err) + } + + var errorList []error + for resp := range respChan { + if resp.Error != nil { + err := fmt.Errorf("can't stop service on target %s: %w", resp.Target, resp.Error) + errorList = append(errorList, err) + } + } + + if len(errorList) == 0 { + return nil + } + + var errorMessages []string + for _, err := range errorList { + errorMessages = append(errorMessages, err.Error()) + } + + return fmt.Errorf("%s", strings.Join(errorMessages, "\n")) +} + // RestartService was the original exported name for RestartRemoteService and now // exists for backwards compatibility. // From 1a063c7deeab3d80a313f4f38367120a7ced5c42 Mon Sep 17 00:00:00 2001 From: Calvin Chan Date: Tue, 5 Dec 2023 00:49:07 -0800 Subject: [PATCH 2/4] Undo changes for client and rewrite error handling --- services/service/client/client.go | 57 +++++++++++++++---------------- services/service/client/utils.go | 55 +++++++++++------------------ 2 files changed, 47 insertions(+), 65 deletions(-) diff --git a/services/service/client/client.go b/services/service/client/client.go index 96054968..4fb92953 100644 --- a/services/service/client/client.go +++ b/services/service/client/client.go @@ -218,22 +218,19 @@ func (s *statusCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interf return subcommands.ExitUsageError } - err = StopManyRemoteService(ctx, state.Conn, system, serviceName) + req := &pb.StatusRequest{ + SystemType: system, + ServiceName: serviceName, + } + c := pb.NewServiceClientProxy(state.Conn) - // Comment out the following code temporarily for testing utils - //req := &pb.StatusRequest{ - // SystemType: system, - // ServiceName: serviceName, - //} - // - //c := pb.NewServiceClientProxy(state.Conn) - //respChan, err := c.StatusOneMany(ctx, req) + respChan, err := c.StatusOneMany(ctx, req) if err != nil { // Emit this to every error file as it's not specific to a given target. - //for _, e := range state.Err { - // fmt.Fprintf(e, "All targets - error executing 'status' for service %s: %v\n", serviceName, err) - //} + for _, e := range state.Err { + fmt.Fprintf(e, "All targets - error executing 'status' for service %s: %v\n", serviceName, err) + } return subcommands.ExitFailure } @@ -244,24 +241,24 @@ func (s *statusCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interf // return early here. // Note that this is only the last non-nil error, and previous // error values may be lost. - //var lastErr error - //for resp := range respChan { - // out := state.Out[resp.Index] - // system, status := resp.Resp.GetSystemType(), resp.Resp.GetServiceStatus().GetStatus() - // output := fmt.Sprintf("[%s] %s : %s", systemTypeString(system), serviceName, statusString(status)) - // if resp.Error != nil { - // lastErr = fmt.Errorf("target %s [%d] error: %w\n", resp.Target, resp.Index, resp.Error) - // fmt.Fprint(state.Err[resp.Index], lastErr) - // continue - // } - // if _, err := fmt.Fprintln(out, output); err != nil { - // lastErr = fmt.Errorf("target %s [%d] write error: %w\n", resp.Target, resp.Index, err) - // fmt.Fprint(state.Err[resp.Index], lastErr) - // } - //} - //if lastErr != nil { - // return subcommands.ExitFailure - //} + var lastErr error + for resp := range respChan { + out := state.Out[resp.Index] + system, status := resp.Resp.GetSystemType(), resp.Resp.GetServiceStatus().GetStatus() + output := fmt.Sprintf("[%s] %s : %s", systemTypeString(system), serviceName, statusString(status)) + if resp.Error != nil { + lastErr = fmt.Errorf("target %s [%d] error: %w\n", resp.Target, resp.Index, resp.Error) + fmt.Fprint(state.Err[resp.Index], lastErr) + continue + } + if _, err := fmt.Fprintln(out, output); err != nil { + lastErr = fmt.Errorf("target %s [%d] write error: %w\n", resp.Target, resp.Index, err) + fmt.Fprint(state.Err[resp.Index], lastErr) + } + } + if lastErr != nil { + return subcommands.ExitFailure + } return subcommands.ExitSuccess } diff --git a/services/service/client/utils.go b/services/service/client/utils.go index 1327d7fa..fa3f074d 100644 --- a/services/service/client/utils.go +++ b/services/service/client/utils.go @@ -20,7 +20,6 @@ import ( "context" "errors" "fmt" - "strings" "github.com/Snowflake-Labs/sansshell/proxy/proxy" pb "github.com/Snowflake-Labs/sansshell/services/service" @@ -97,10 +96,10 @@ func StopRemoteService(ctx context.Context, conn *proxy.Conn, system pb.SystemTy return nil } -// StartManyRemoteServices is a helper function for starting a service on multiple remote targets. -func StartManyRemoteServices(ctx context.Context, conn *proxy.Conn, system pb.SystemType, service string) error { +// StartRemoteServiceManys is a helper function for starting a service on multiple remote targets. +func StartRemoteServiceMany(ctx context.Context, conn *proxy.Conn, system pb.SystemType, service string) error { c := pb.NewServiceClientProxy(conn) - respChan, err := c.ActionOneMany(ctx, &pb.ActionRequest{ + resp, err := c.ActionOneMany(ctx, &pb.ActionRequest{ ServiceName: service, SystemType: system, Action: pb.Action_ACTION_START, @@ -110,30 +109,23 @@ func StartManyRemoteServices(ctx context.Context, conn *proxy.Conn, system pb.Sy return fmt.Errorf("can't start service %s - %v", service, err) } - var errorList []error - for resp := range respChan { - if resp.Error != nil { - err := fmt.Errorf("can't start service on target %s: %w", resp.Target, resp.Error) - errorList = append(errorList, err) + errMsg := "" + for r := range resp { + if r.Error != nil { + errMsg += fmt.Sprintf("target %s (%d): %v", r.Target, r.Index, r.Error) } } - - if len(errorList) == 0 { - return nil - } - - var errorMessages []string - for _, err := range errorList { - errorMessages = append(errorMessages, err.Error()) + if errMsg != "" { + return fmt.Errorf("StartRemoteServiceMany failed: %s", errMsg) } - return fmt.Errorf("%s", strings.Join(errorMessages, "\n")) + return nil } -// StopManyRemoteService is a helper function for stopping a service on multiple remote targets. -func StopManyRemoteService(ctx context.Context, conn *proxy.Conn, system pb.SystemType, service string) error { +// StopRemoteServiceMany is a helper function for stopping a service on multiple remote targets. +func StopRemoteServiceMany(ctx context.Context, conn *proxy.Conn, system pb.SystemType, service string) error { c := pb.NewServiceClientProxy(conn) - respChan, err := c.ActionOneMany(ctx, &pb.ActionRequest{ + resp, err := c.ActionOneMany(ctx, &pb.ActionRequest{ ServiceName: service, SystemType: system, Action: pb.Action_ACTION_STOP, @@ -143,24 +135,17 @@ func StopManyRemoteService(ctx context.Context, conn *proxy.Conn, system pb.Syst return fmt.Errorf("can't stop service %s - %v", service, err) } - var errorList []error - for resp := range respChan { - if resp.Error != nil { - err := fmt.Errorf("can't stop service on target %s: %w", resp.Target, resp.Error) - errorList = append(errorList, err) + errMsg := "" + for r := range resp { + if r.Error != nil { + errMsg += fmt.Sprintf("target %s (%d): %v", r.Target, r.Index, r.Error) } } - - if len(errorList) == 0 { - return nil - } - - var errorMessages []string - for _, err := range errorList { - errorMessages = append(errorMessages, err.Error()) + if errMsg != "" { + return fmt.Errorf("StopRemoteServiceMany failed: %s", errMsg) } - return fmt.Errorf("%s", strings.Join(errorMessages, "\n")) + return nil } // RestartService was the original exported name for RestartRemoteService and now From c6aab9567d998253d6bbff76e32ba1da9837a349 Mon Sep 17 00:00:00 2001 From: Calvin Chan Date: Fri, 8 Dec 2023 08:34:55 -0800 Subject: [PATCH 3/4] Fix spacing for comments in client --- services/service/client/client.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/services/service/client/client.go b/services/service/client/client.go index 4fb92953..1dc87358 100644 --- a/services/service/client/client.go +++ b/services/service/client/client.go @@ -303,13 +303,13 @@ func (l *listCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interfac return subcommands.ExitFailure } - // // Error holding the last observed non-nil error, which will - // // determine the exit status of the command. - // // The contract with the proxy and 'many' functions requires - // // that we completely drain the response channel, so we cannot - // // return early here. - // // Note that this is only the last non-nil error, and previous - // // error values may be lost. + // Error holding the last observed non-nil error, which will + // determine the exit status of the command. + // The contract with the proxy and 'many' functions requires + // that we completely drain the response channel, so we cannot + // return early here. + // Note that this is only the last non-nil error, and previous + // error values may be lost. var lastErr error for resp := range respChan { out := state.Out[resp.Index] From 3459263a08820b8cdcc6807d32404a96842be63f Mon Sep 17 00:00:00 2001 From: Calvin Chan Date: Mon, 18 Dec 2023 13:31:39 -0800 Subject: [PATCH 4/4] Client change for testing --- services/service/client/client.go | 59 ++++++++++++++++--------------- 1 file changed, 30 insertions(+), 29 deletions(-) diff --git a/services/service/client/client.go b/services/service/client/client.go index 1dc87358..4cdbec43 100644 --- a/services/service/client/client.go +++ b/services/service/client/client.go @@ -218,19 +218,21 @@ func (s *statusCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interf return subcommands.ExitUsageError } - req := &pb.StatusRequest{ - SystemType: system, - ServiceName: serviceName, - } - c := pb.NewServiceClientProxy(state.Conn) + err = StopRemoteServiceMany(ctx, state.Conn, system, serviceName) - respChan, err := c.StatusOneMany(ctx, req) + //req := &pb.StatusRequest{ + // SystemType: system, + // ServiceName: serviceName, + //} + //c := pb.NewServiceClientProxy(state.Conn) + // + //respChan, err := c.StatusOneMany(ctx, req) if err != nil { - // Emit this to every error file as it's not specific to a given target. - for _, e := range state.Err { - fmt.Fprintf(e, "All targets - error executing 'status' for service %s: %v\n", serviceName, err) - } + //// Emit this to every error file as it's not specific to a given target. + //for _, e := range state.Err { + // fmt.Fprintf(e, "All targets - error executing 'status' for service %s: %v\n", serviceName, err) + //} return subcommands.ExitFailure } @@ -241,24 +243,24 @@ func (s *statusCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interf // return early here. // Note that this is only the last non-nil error, and previous // error values may be lost. - var lastErr error - for resp := range respChan { - out := state.Out[resp.Index] - system, status := resp.Resp.GetSystemType(), resp.Resp.GetServiceStatus().GetStatus() - output := fmt.Sprintf("[%s] %s : %s", systemTypeString(system), serviceName, statusString(status)) - if resp.Error != nil { - lastErr = fmt.Errorf("target %s [%d] error: %w\n", resp.Target, resp.Index, resp.Error) - fmt.Fprint(state.Err[resp.Index], lastErr) - continue - } - if _, err := fmt.Fprintln(out, output); err != nil { - lastErr = fmt.Errorf("target %s [%d] write error: %w\n", resp.Target, resp.Index, err) - fmt.Fprint(state.Err[resp.Index], lastErr) - } - } - if lastErr != nil { - return subcommands.ExitFailure - } + //var lastErr error + //for resp := range respChan { + // out := state.Out[resp.Index] + // system, status := resp.Resp.GetSystemType(), resp.Resp.GetServiceStatus().GetStatus() + // output := fmt.Sprintf("[%s] %s : %s", systemTypeString(system), serviceName, statusString(status)) + // if resp.Error != nil { + // lastErr = fmt.Errorf("target %s [%d] error: %w\n", resp.Target, resp.Index, resp.Error) + // fmt.Fprint(state.Err[resp.Index], lastErr) + // continue + // } + // if _, err := fmt.Fprintln(out, output); err != nil { + // lastErr = fmt.Errorf("target %s [%d] write error: %w\n", resp.Target, resp.Index, err) + // fmt.Fprint(state.Err[resp.Index], lastErr) + // } + //} + //if lastErr != nil { + // return subcommands.ExitFailure + //} return subcommands.ExitSuccess } @@ -293,7 +295,6 @@ func (l *listCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interfac SystemType: system, } c := pb.NewServiceClientProxy(state.Conn) - respChan, err := c.ListOneMany(ctx, req) if err != nil { // Emit this to every error file as it's not specific to a given target.