From 0ec935352e9605fecc8ba13291e4c6288c1fe2a9 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Tue, 4 Apr 2023 10:17:23 +0200 Subject: [PATCH 01/13] Use devstate.PID.json --- pkg/dev/podmandev/reconcile.go | 3 +- pkg/odo/cli/describe/component.go | 3 +- pkg/odo/cli/dev/dev.go | 9 +- .../kubeportforward/portForward.go | 3 +- pkg/state/const.go | 1 + pkg/state/doc.go | 3 + pkg/state/interface.go | 6 +- pkg/state/state.go | 89 ++++++++++++++++--- pkg/state/state_test.go | 12 ++- pkg/state/types.go | 2 + 10 files changed, 106 insertions(+), 25 deletions(-) diff --git a/pkg/dev/podmandev/reconcile.go b/pkg/dev/podmandev/reconcile.go index 7536fcd4bea..67dfff5b92f 100644 --- a/pkg/dev/podmandev/reconcile.go +++ b/pkg/dev/podmandev/reconcile.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io" + "os" "path/filepath" "strings" "time" @@ -153,7 +154,7 @@ func (o *DevClient) reconcile( s := fmt.Sprintf("Forwarding from %s:%d -> %d", fwPort.LocalAddress, fwPort.LocalPort, fwPort.ContainerPort) fmt.Fprintf(out, " - %s", log.SboldColor(color.FgGreen, s)) } - err = o.stateClient.SetForwardedPorts(fwPorts) + err = o.stateClient.SetForwardedPorts(os.Getpid(), fwPorts) if err != nil { return err } diff --git a/pkg/odo/cli/describe/component.go b/pkg/odo/cli/describe/component.go index 5c62f2af376..e64f8718784 100644 --- a/pkg/odo/cli/describe/component.go +++ b/pkg/odo/cli/describe/component.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "os" "strings" "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" @@ -241,7 +242,7 @@ func (o *ComponentOptions) describeDevfileComponent(ctx context.Context) (result kubeClient = nil } - allFwdPorts, err := o.clientset.StateClient.GetForwardedPorts() + allFwdPorts, err := o.clientset.StateClient.GetForwardedPorts(os.Getpid()) if err != nil { return api.Component{}, nil, err } diff --git a/pkg/odo/cli/dev/dev.go b/pkg/odo/cli/dev/dev.go index a9f34f703ca..5bb6daa2a39 100644 --- a/pkg/odo/cli/dev/dev.go +++ b/pkg/odo/cli/dev/dev.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io" + "os" "path/filepath" "regexp" "sort" @@ -12,13 +13,11 @@ import ( "strings" "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" - "k8s.io/klog" - - "github.com/redhat-developer/odo/pkg/api" - "github.com/spf13/cobra" + "k8s.io/klog" ktemplates "k8s.io/kubectl/pkg/util/templates" + "github.com/redhat-developer/odo/pkg/api" "github.com/redhat-developer/odo/pkg/component" "github.com/redhat-developer/odo/pkg/dev" "github.com/redhat-developer/odo/pkg/kclient" @@ -261,7 +260,7 @@ func (o *DevOptions) Cleanup(ctx context.Context, commandError error) { if commandError != nil { _ = o.clientset.DevClient.CleanupResources(ctx, log.GetStdout()) } - _ = o.clientset.StateClient.SaveExit() + _ = o.clientset.StateClient.SaveExit(os.Getpid()) } // NewCmdDev implements the odo dev command diff --git a/pkg/portForward/kubeportforward/portForward.go b/pkg/portForward/kubeportforward/portForward.go index 653447cf0f8..91cc58d3117 100644 --- a/pkg/portForward/kubeportforward/portForward.go +++ b/pkg/portForward/kubeportforward/portForward.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "io" + "os" "reflect" "sort" "time" @@ -114,7 +115,7 @@ func (o *PFClient) StartPortForwarding(devFileObj parser.DevfileObj, componentNa go func() { portsBuf.Wait() - err = o.stateClient.SetForwardedPorts(portsBuf.GetForwardedPorts()) + err = o.stateClient.SetForwardedPorts(os.Getpid(), portsBuf.GetForwardedPorts()) if err != nil { err = fmt.Errorf("unable to save forwarded ports to state file: %v", err) } diff --git a/pkg/state/const.go b/pkg/state/const.go index f81a14eaf4f..58f2b85391e 100644 --- a/pkg/state/const.go +++ b/pkg/state/const.go @@ -1,3 +1,4 @@ package state const _filepath = "./.odo/devstate.json" +const _filepathPid = "./.odo/devstate.%d.json" diff --git a/pkg/state/doc.go b/pkg/state/doc.go index cea421b0d98..1bb7898e400 100644 --- a/pkg/state/doc.go +++ b/pkg/state/doc.go @@ -1,2 +1,5 @@ // Package state gives access to the state of the odo process stored in a local file +// The state of an instance is stored in a file .odo/devstate.${PID}.json. +// For compatibility with previous versions of odo, the `devstate.json` file contains +// the state of the first instance of odo. package state diff --git a/pkg/state/interface.go b/pkg/state/interface.go index ce2b9a66ec0..231c2ad03db 100644 --- a/pkg/state/interface.go +++ b/pkg/state/interface.go @@ -4,11 +4,11 @@ import "github.com/redhat-developer/odo/pkg/api" type Client interface { // SetForwardedPorts sets the forwarded ports in the state file and saves it to the file, updating the metadata - SetForwardedPorts(fwPorts []api.ForwardedPort) error + SetForwardedPorts(pid int, fwPorts []api.ForwardedPort) error // GetForwardedPorts returns the ports forwarded by the current odo dev session - GetForwardedPorts() ([]api.ForwardedPort, error) + GetForwardedPorts(pid int) ([]api.ForwardedPort, error) // SaveExit resets the state file to indicate odo is not running - SaveExit() error + SaveExit(pid int) error } diff --git a/pkg/state/state.go b/pkg/state/state.go index 79f69f21b47..76842e8c6e9 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -3,6 +3,7 @@ package state import ( "encoding/json" "errors" + "fmt" "io/fs" "os" "path/filepath" @@ -24,14 +25,15 @@ func NewStateClient(fs filesystem.Filesystem) *State { } } -func (o *State) SetForwardedPorts(fwPorts []api.ForwardedPort) error { +func (o *State) SetForwardedPorts(pid int, fwPorts []api.ForwardedPort) error { // TODO(feloy) When other data is persisted into the state file, it will be needed to read the file first o.content.ForwardedPorts = fwPorts - return o.save() + o.content.PID = pid + return o.save(pid) } -func (o *State) GetForwardedPorts() ([]api.ForwardedPort, error) { - err := o.read() +func (o *State) GetForwardedPorts(pid int) ([]api.ForwardedPort, error) { + err := o.read(pid) if err != nil { if errors.Is(err, fs.ErrNotExist) { return nil, nil // if the state file does not exist, no ports are forwarded @@ -41,13 +43,63 @@ func (o *State) GetForwardedPorts() ([]api.ForwardedPort, error) { return o.content.ForwardedPorts, err } -func (o *State) SaveExit() error { +func (o *State) SaveExit(pid int) error { o.content.ForwardedPorts = nil - return o.save() + o.content.PID = 0 + err := o.delete(pid) + if err != nil { + return err + } + return o.saveCommonIfOwner(pid) } // save writes the content structure in json format in file -func (o *State) save() error { +func (o *State) save(pid int) error { + + err := o.saveCommonIfOwner(pid) + if err != nil { + return err + } + + jsonContent, err := json.MarshalIndent(o.content, "", " ") + if err != nil { + return err + } + // .odo directory is supposed to exist, don't create it + dir := filepath.Dir(getFilename(pid)) + err = os.MkdirAll(dir, 0750) + if err != nil { + return err + } + return o.fs.WriteFile(getFilename(pid), jsonContent, 0644) +} + +func (o *State) read(pid int) error { + jsonContent, err := o.fs.ReadFile(getFilename(pid)) + if err != nil { + return err + } + return json.Unmarshal(jsonContent, &o.content) +} + +func (o *State) delete(pid int) error { + return o.fs.Remove(getFilename(pid)) +} + +func getFilename(pid int) string { + return fmt.Sprintf(_filepathPid, pid) +} + +func (o *State) saveCommonIfOwner(pid int) error { + + ok, err := o.isFreeOrOwnedBy(pid) + if err != nil { + return err + } + if !ok { + return nil + } + jsonContent, err := json.MarshalIndent(o.content, "", " ") if err != nil { return err @@ -61,10 +113,27 @@ func (o *State) save() error { return o.fs.WriteFile(_filepath, jsonContent, 0644) } -func (o *State) read() error { +func (o *State) isFreeOrOwnedBy(pid int) (bool, error) { jsonContent, err := o.fs.ReadFile(_filepath) if err != nil { - return err + if errors.Is(err, os.ErrNotExist) { + return true, nil + } + // File not found, it is free + return false, err } - return json.Unmarshal(jsonContent, &o.content) + var savedContent Content + err = json.Unmarshal(jsonContent, &savedContent) + if err != nil { + return false, err + } + if savedContent.PID == 0 { + // PID is 0 in file, it is free + return true, nil + } + if savedContent.PID == pid { + // File is owned by process + return true, nil + } + return false, nil } diff --git a/pkg/state/state_test.go b/pkg/state/state_test.go index 776f56591a8..bab29a57715 100644 --- a/pkg/state/state_test.go +++ b/pkg/state/state_test.go @@ -77,7 +77,7 @@ func TestState_SetForwardedPorts(t *testing.T) { o := State{ fs: fs, } - if err := o.SetForwardedPorts(tt.args.fwPorts); (err != nil) != tt.wantErr { + if err := o.SetForwardedPorts(1, tt.args.fwPorts); (err != nil) != tt.wantErr { t.Errorf("State.SetForwardedPorts() error = %v, wantErr %v", err, tt.wantErr) } if check := tt.checkState(fs); check != nil { @@ -136,7 +136,9 @@ func TestState_SaveExit(t *testing.T) { o := State{ fs: fs, } - if err := o.SaveExit(); (err != nil) != tt.wantErr { + pid := 1 + _ = o.SetForwardedPorts(pid, nil) + if err := o.SaveExit(pid); (err != nil) != tt.wantErr { t.Errorf("State.SaveExit() error = %v, wantErr %v", err, tt.wantErr) } if check := tt.checkState(fs); check != nil { @@ -177,7 +179,8 @@ func TestState_GetForwardedPorts(t *testing.T) { if err != nil { t.Errorf("Error marshaling data") } - err = fs.WriteFile(_filepath, jsonContent, 0644) + pid := 1 + err = fs.WriteFile(getFilename(pid), jsonContent, 0644) if err != nil { t.Errorf("Error saving content to file") } @@ -194,7 +197,8 @@ func TestState_GetForwardedPorts(t *testing.T) { content: tt.fields.content, fs: tt.fields.fs(t), } - got, err := o.GetForwardedPorts() + pid := 1 + got, err := o.GetForwardedPorts(pid) if (err != nil) != tt.wantErr { t.Errorf("State.GetForwardedPorts() error = %v, wantErr %v", err, tt.wantErr) return diff --git a/pkg/state/types.go b/pkg/state/types.go index 1a68bebe629..242c994fbc9 100644 --- a/pkg/state/types.go +++ b/pkg/state/types.go @@ -5,6 +5,8 @@ import ( ) type Content struct { + // PID is the ID of the process to which the state belongs + PID int `json:"pid"` // ForwardedPorts are the ports forwarded during odo dev session ForwardedPorts []api.ForwardedPort `json:"forwardedPorts"` } From 9c2edf76e041e99565e3d04481ae93fa7a6d85cf Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Tue, 4 Apr 2023 10:54:46 +0200 Subject: [PATCH 02/13] odo describe component gets forwarded ports from devstate.json --- pkg/odo/cli/describe/component.go | 4 ++-- pkg/state/state.go | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/odo/cli/describe/component.go b/pkg/odo/cli/describe/component.go index e64f8718784..4773af25b6b 100644 --- a/pkg/odo/cli/describe/component.go +++ b/pkg/odo/cli/describe/component.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "os" "strings" "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" @@ -242,7 +241,8 @@ func (o *ComponentOptions) describeDevfileComponent(ctx context.Context) (result kubeClient = nil } - allFwdPorts, err := o.clientset.StateClient.GetForwardedPorts(os.Getpid()) + // TODO(feloy) Pass PID with `--pid` flag + allFwdPorts, err := o.clientset.StateClient.GetForwardedPorts(0) if err != nil { return api.Component{}, nil, err } diff --git a/pkg/state/state.go b/pkg/state/state.go index 76842e8c6e9..75b1e0baa38 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -87,6 +87,9 @@ func (o *State) delete(pid int) error { } func getFilename(pid int) string { + if pid == 0 { + return _filepath + } return fmt.Sprintf(_filepathPid, pid) } From 085ead35f94a9c9be1ec5ccf9e57b2f9e86a580d Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Wed, 5 Apr 2023 19:21:15 +0200 Subject: [PATCH 03/13] Pass pid in context --- cmd/odo/odo.go | 2 ++ pkg/dev/podmandev/reconcile.go | 5 ++--- .../adapters/kubernetes/component/adapter.go | 2 +- pkg/odo/cli/describe/component.go | 2 +- pkg/odo/cli/dev/dev.go | 3 +-- pkg/odo/context/odo.go | 19 +++++++++++++++++++ pkg/portForward/interface.go | 2 ++ .../kubeportforward/portForward.go | 6 +++--- .../podmanportforward/portForward.go | 2 ++ pkg/registry/mock.go | 8 ++++---- pkg/state/interface.go | 12 ++++++++---- pkg/state/state.go | 17 ++++++++++++++--- pkg/state/state_test.go | 18 ++++++++++++------ 13 files changed, 71 insertions(+), 27 deletions(-) diff --git a/cmd/odo/odo.go b/cmd/odo/odo.go index ed54bf5b584..cb737c4119f 100644 --- a/cmd/odo/odo.go +++ b/cmd/odo/odo.go @@ -14,6 +14,7 @@ import ( "github.com/redhat-developer/odo/pkg/log" "github.com/redhat-developer/odo/pkg/odo/cli" "github.com/redhat-developer/odo/pkg/odo/cli/version" + odocontext "github.com/redhat-developer/odo/pkg/odo/context" "github.com/redhat-developer/odo/pkg/odo/util" "github.com/redhat-developer/odo/pkg/odo/util/completion" "github.com/redhat-developer/odo/pkg/preference" @@ -31,6 +32,7 @@ func main() { util.LogErrorAndExit(err, "") } ctx = envcontext.WithEnvConfig(ctx, *envConfig) + ctx = odocontext.WithPID(ctx, os.Getpid()) // create the complete command klog.InitFlags(nil) diff --git a/pkg/dev/podmandev/reconcile.go b/pkg/dev/podmandev/reconcile.go index 67dfff5b92f..31c4e169259 100644 --- a/pkg/dev/podmandev/reconcile.go +++ b/pkg/dev/podmandev/reconcile.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "io" - "os" "path/filepath" "strings" "time" @@ -144,7 +143,7 @@ func (o *DevClient) reconcile( if options.ForwardLocalhost { // Port-forwarding is enabled by executing dedicated socat commands - err = o.portForwardClient.StartPortForwarding(*devfileObj, componentName, options.Debug, options.RandomPorts, out, errOut, fwPorts) + err = o.portForwardClient.StartPortForwarding(ctx, *devfileObj, componentName, options.Debug, options.RandomPorts, out, errOut, fwPorts) if err != nil { return adapters.NewErrPortForward(err) } @@ -154,7 +153,7 @@ func (o *DevClient) reconcile( s := fmt.Sprintf("Forwarding from %s:%d -> %d", fwPort.LocalAddress, fwPort.LocalPort, fwPort.ContainerPort) fmt.Fprintf(out, " - %s", log.SboldColor(color.FgGreen, s)) } - err = o.stateClient.SetForwardedPorts(os.Getpid(), fwPorts) + err = o.stateClient.SetForwardedPorts(ctx, fwPorts) if err != nil { return err } diff --git a/pkg/devfile/adapters/kubernetes/component/adapter.go b/pkg/devfile/adapters/kubernetes/component/adapter.go index cafad970e2f..3304569d8c1 100644 --- a/pkg/devfile/adapters/kubernetes/component/adapter.go +++ b/pkg/devfile/adapters/kubernetes/component/adapter.go @@ -366,7 +366,7 @@ func (a Adapter) Push(ctx context.Context, parameters adapters.PushParameters, c fmt.Fprintln(log.GetStdout()) } - err = a.portForwardClient.StartPortForwarding(a.Devfile, a.ComponentName, parameters.Debug, parameters.RandomPorts, log.GetStdout(), parameters.ErrOut, parameters.CustomForwardedPorts) + err = a.portForwardClient.StartPortForwarding(ctx, a.Devfile, a.ComponentName, parameters.Debug, parameters.RandomPorts, log.GetStdout(), parameters.ErrOut, parameters.CustomForwardedPorts) if err != nil { return adapters.NewErrPortForward(err) } diff --git a/pkg/odo/cli/describe/component.go b/pkg/odo/cli/describe/component.go index 4773af25b6b..d3801bd7d47 100644 --- a/pkg/odo/cli/describe/component.go +++ b/pkg/odo/cli/describe/component.go @@ -242,7 +242,7 @@ func (o *ComponentOptions) describeDevfileComponent(ctx context.Context) (result } // TODO(feloy) Pass PID with `--pid` flag - allFwdPorts, err := o.clientset.StateClient.GetForwardedPorts(0) + allFwdPorts, err := o.clientset.StateClient.GetForwardedPorts(ctx) if err != nil { return api.Component{}, nil, err } diff --git a/pkg/odo/cli/dev/dev.go b/pkg/odo/cli/dev/dev.go index 5bb6daa2a39..3895c19e6c7 100644 --- a/pkg/odo/cli/dev/dev.go +++ b/pkg/odo/cli/dev/dev.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "io" - "os" "path/filepath" "regexp" "sort" @@ -260,7 +259,7 @@ func (o *DevOptions) Cleanup(ctx context.Context, commandError error) { if commandError != nil { _ = o.clientset.DevClient.CleanupResources(ctx, log.GetStdout()) } - _ = o.clientset.StateClient.SaveExit(os.Getpid()) + _ = o.clientset.StateClient.SaveExit(ctx) } // NewCmdDev implements the odo dev command diff --git a/pkg/odo/context/odo.go b/pkg/odo/context/odo.go index 9ac76f150da..83e755289db 100644 --- a/pkg/odo/context/odo.go +++ b/pkg/odo/context/odo.go @@ -9,6 +9,7 @@ import ( type ( applicationKeyType struct{} cwdKeyType struct{} + pidKeyType struct{} devfilePathKeyType struct{} devfileObjKeyType struct{} componentNameKeyType struct{} @@ -17,6 +18,7 @@ type ( var ( applicationKey applicationKeyType cwdKey cwdKeyType + pidKey pidKeyType devfilePathKey devfilePathKeyType devfileObjKey devfileObjKeyType componentNameKey componentNameKeyType @@ -57,6 +59,23 @@ func GetWorkingDirectory(ctx context.Context) string { panic("this should not happen, either the original context is not passed or WithWorkingDirectory is not called as it should. Check that FILESYSTEM dependency is added to the command") } +// WithPID sets the value of the PID in ctx +// This function must be used before calling GetPID +// Use this function only with a context obtained from Complete/Validate/Run/... methods of Runnable interface +func WithPID(ctx context.Context, val int) context.Context { + return context.WithValue(ctx, pidKey, val) +} + +// GetPID gets the PID value in ctx +// This function will panic if the context does not contain the value +func GetPID(ctx context.Context) int { + value := ctx.Value(pidKey) + if cast, ok := value.(int); ok { + return cast + } + panic("this should not happen, either the original context is not passed or WithPID is not called as it should") +} + // WithDevfilePath sets the value of the devfile path in ctx // This function must be called before using GetDevfilePath func WithDevfilePath(ctx context.Context, val string) context.Context { diff --git a/pkg/portForward/interface.go b/pkg/portForward/interface.go index c8e0a386d37..c29e579956b 100644 --- a/pkg/portForward/interface.go +++ b/pkg/portForward/interface.go @@ -1,6 +1,7 @@ package portForward import ( + "context" "io" "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" @@ -16,6 +17,7 @@ type Client interface { // output will be written to errOut writer // definedPorts allows callers to explicitly define the mapping they want to set. StartPortForwarding( + ctx context.Context, devFileObj parser.DevfileObj, componentName string, debug bool, diff --git a/pkg/portForward/kubeportforward/portForward.go b/pkg/portForward/kubeportforward/portForward.go index 91cc58d3117..0e51e1cd187 100644 --- a/pkg/portForward/kubeportforward/portForward.go +++ b/pkg/portForward/kubeportforward/portForward.go @@ -1,10 +1,10 @@ package kubeportforward import ( + "context" "errors" "fmt" "io" - "os" "reflect" "sort" "time" @@ -50,7 +50,7 @@ func NewPFClient(kubernetesClient kclient.ClientInterface, stateClient state.Cli } } -func (o *PFClient) StartPortForwarding(devFileObj parser.DevfileObj, componentName string, debug bool, randomPorts bool, out io.Writer, errOut io.Writer, definedPorts []api.ForwardedPort) error { +func (o *PFClient) StartPortForwarding(ctx context.Context, devFileObj parser.DevfileObj, componentName string, debug bool, randomPorts bool, out io.Writer, errOut io.Writer, definedPorts []api.ForwardedPort) error { if randomPorts && len(definedPorts) != 0 { return errors.New("cannot use randomPorts and custom definePorts together") } @@ -115,7 +115,7 @@ func (o *PFClient) StartPortForwarding(devFileObj parser.DevfileObj, componentNa go func() { portsBuf.Wait() - err = o.stateClient.SetForwardedPorts(os.Getpid(), portsBuf.GetForwardedPorts()) + err = o.stateClient.SetForwardedPorts(ctx, portsBuf.GetForwardedPorts()) if err != nil { err = fmt.Errorf("unable to save forwarded ports to state file: %v", err) } diff --git a/pkg/portForward/podmanportforward/portForward.go b/pkg/portForward/podmanportforward/portForward.go index 0dbcf31c140..dd9b5c1fdc4 100644 --- a/pkg/portForward/podmanportforward/portForward.go +++ b/pkg/portForward/podmanportforward/portForward.go @@ -1,6 +1,7 @@ package podmanportforward import ( + "context" "fmt" "io" "reflect" @@ -36,6 +37,7 @@ func NewPFClient(execClient exec.Client) *PFClient { } func (o *PFClient) StartPortForwarding( + ctx context.Context, devFileObj parser.DevfileObj, componentName string, debug bool, diff --git a/pkg/registry/mock.go b/pkg/registry/mock.go index a71c4bea16e..9f99b909d7b 100644 --- a/pkg/registry/mock.go +++ b/pkg/registry/mock.go @@ -83,18 +83,18 @@ func (mr *MockClientMockRecorder) GetDevfileRegistries(registryName interface{}) } // ListDevfileStacks mocks base method. -func (m *MockClient) ListDevfileStacks(ctx context.Context, registryName, devfileFlag, filterFlag string, detailsFlag, jsonOutput bool) (DevfileStackList, error) { +func (m *MockClient) ListDevfileStacks(ctx context.Context, registryName, devfileFlag, filterFlag string, detailsFlag, withDevfileContent bool) (DevfileStackList, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ListDevfileStacks", ctx, registryName, devfileFlag, filterFlag, detailsFlag, jsonOutput) + ret := m.ctrl.Call(m, "ListDevfileStacks", ctx, registryName, devfileFlag, filterFlag, detailsFlag, withDevfileContent) ret0, _ := ret[0].(DevfileStackList) ret1, _ := ret[1].(error) return ret0, ret1 } // ListDevfileStacks indicates an expected call of ListDevfileStacks. -func (mr *MockClientMockRecorder) ListDevfileStacks(ctx, registryName, devfileFlag, filterFlag, detailsFlag, jsonOutput interface{}) *gomock.Call { +func (mr *MockClientMockRecorder) ListDevfileStacks(ctx, registryName, devfileFlag, filterFlag, detailsFlag, withDevfileContent interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListDevfileStacks", reflect.TypeOf((*MockClient)(nil).ListDevfileStacks), ctx, registryName, devfileFlag, filterFlag, detailsFlag, jsonOutput) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListDevfileStacks", reflect.TypeOf((*MockClient)(nil).ListDevfileStacks), ctx, registryName, devfileFlag, filterFlag, detailsFlag, withDevfileContent) } // PullStackFromRegistry mocks base method. diff --git a/pkg/state/interface.go b/pkg/state/interface.go index 231c2ad03db..2306c3cf98b 100644 --- a/pkg/state/interface.go +++ b/pkg/state/interface.go @@ -1,14 +1,18 @@ package state -import "github.com/redhat-developer/odo/pkg/api" +import ( + "context" + + "github.com/redhat-developer/odo/pkg/api" +) type Client interface { // SetForwardedPorts sets the forwarded ports in the state file and saves it to the file, updating the metadata - SetForwardedPorts(pid int, fwPorts []api.ForwardedPort) error + SetForwardedPorts(ctx context.Context, fwPorts []api.ForwardedPort) error // GetForwardedPorts returns the ports forwarded by the current odo dev session - GetForwardedPorts(pid int) ([]api.ForwardedPort, error) + GetForwardedPorts(ctx context.Context) ([]api.ForwardedPort, error) // SaveExit resets the state file to indicate odo is not running - SaveExit(pid int) error + SaveExit(ctx context.Context) error } diff --git a/pkg/state/state.go b/pkg/state/state.go index 75b1e0baa38..2d229e03801 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -1,6 +1,7 @@ package state import ( + "context" "encoding/json" "errors" "fmt" @@ -9,6 +10,7 @@ import ( "path/filepath" "github.com/redhat-developer/odo/pkg/api" + odocontext "github.com/redhat-developer/odo/pkg/odo/context" "github.com/redhat-developer/odo/pkg/testingutil/filesystem" ) @@ -25,14 +27,20 @@ func NewStateClient(fs filesystem.Filesystem) *State { } } -func (o *State) SetForwardedPorts(pid int, fwPorts []api.ForwardedPort) error { +func (o *State) SetForwardedPorts(ctx context.Context, fwPorts []api.ForwardedPort) error { + var ( + pid = odocontext.GetPID(ctx) + ) // TODO(feloy) When other data is persisted into the state file, it will be needed to read the file first o.content.ForwardedPorts = fwPorts o.content.PID = pid return o.save(pid) } -func (o *State) GetForwardedPorts(pid int) ([]api.ForwardedPort, error) { +func (o *State) GetForwardedPorts(ctx context.Context) ([]api.ForwardedPort, error) { + var ( + pid = odocontext.GetPID(ctx) + ) err := o.read(pid) if err != nil { if errors.Is(err, fs.ErrNotExist) { @@ -43,7 +51,10 @@ func (o *State) GetForwardedPorts(pid int) ([]api.ForwardedPort, error) { return o.content.ForwardedPorts, err } -func (o *State) SaveExit(pid int) error { +func (o *State) SaveExit(ctx context.Context) error { + var ( + pid = odocontext.GetPID(ctx) + ) o.content.ForwardedPorts = nil o.content.PID = 0 err := o.delete(pid) diff --git a/pkg/state/state_test.go b/pkg/state/state_test.go index bab29a57715..6f713ab8016 100644 --- a/pkg/state/state_test.go +++ b/pkg/state/state_test.go @@ -1,6 +1,7 @@ package state import ( + "context" "encoding/json" "fmt" "testing" @@ -8,6 +9,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/redhat-developer/odo/pkg/api" + odocontext "github.com/redhat-developer/odo/pkg/odo/context" "github.com/redhat-developer/odo/pkg/testingutil/filesystem" ) @@ -77,7 +79,9 @@ func TestState_SetForwardedPorts(t *testing.T) { o := State{ fs: fs, } - if err := o.SetForwardedPorts(1, tt.args.fwPorts); (err != nil) != tt.wantErr { + ctx := context.Background() + ctx = odocontext.WithPID(ctx, 1) + if err := o.SetForwardedPorts(ctx, tt.args.fwPorts); (err != nil) != tt.wantErr { t.Errorf("State.SetForwardedPorts() error = %v, wantErr %v", err, tt.wantErr) } if check := tt.checkState(fs); check != nil { @@ -136,9 +140,10 @@ func TestState_SaveExit(t *testing.T) { o := State{ fs: fs, } - pid := 1 - _ = o.SetForwardedPorts(pid, nil) - if err := o.SaveExit(pid); (err != nil) != tt.wantErr { + ctx := context.Background() + ctx = odocontext.WithPID(ctx, 1) + _ = o.SetForwardedPorts(ctx, nil) + if err := o.SaveExit(ctx); (err != nil) != tt.wantErr { t.Errorf("State.SaveExit() error = %v, wantErr %v", err, tt.wantErr) } if check := tt.checkState(fs); check != nil { @@ -197,8 +202,9 @@ func TestState_GetForwardedPorts(t *testing.T) { content: tt.fields.content, fs: tt.fields.fs(t), } - pid := 1 - got, err := o.GetForwardedPorts(pid) + ctx := context.Background() + ctx = odocontext.WithPID(ctx, 1) + got, err := o.GetForwardedPorts(ctx) if (err != nil) != tt.wantErr { t.Errorf("State.GetForwardedPorts() error = %v, wantErr %v", err, tt.wantErr) return From d664236c3b45aa7e7b62c3f159e8da6c1e48be08 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Wed, 5 Apr 2023 19:29:22 +0200 Subject: [PATCH 04/13] Add platform to state --- pkg/state/state.go | 7 ++++++- pkg/state/types.go | 2 ++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/state/state.go b/pkg/state/state.go index 2d229e03801..9fa70688fa6 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -10,6 +10,8 @@ import ( "path/filepath" "github.com/redhat-developer/odo/pkg/api" + "github.com/redhat-developer/odo/pkg/odo/commonflags" + fcontext "github.com/redhat-developer/odo/pkg/odo/commonflags/context" odocontext "github.com/redhat-developer/odo/pkg/odo/context" "github.com/redhat-developer/odo/pkg/testingutil/filesystem" ) @@ -29,11 +31,13 @@ func NewStateClient(fs filesystem.Filesystem) *State { func (o *State) SetForwardedPorts(ctx context.Context, fwPorts []api.ForwardedPort) error { var ( - pid = odocontext.GetPID(ctx) + pid = odocontext.GetPID(ctx) + platform = fcontext.GetPlatform(ctx, commonflags.PlatformCluster) ) // TODO(feloy) When other data is persisted into the state file, it will be needed to read the file first o.content.ForwardedPorts = fwPorts o.content.PID = pid + o.content.Platform = platform return o.save(pid) } @@ -57,6 +61,7 @@ func (o *State) SaveExit(ctx context.Context) error { ) o.content.ForwardedPorts = nil o.content.PID = 0 + o.content.Platform = "" err := o.delete(pid) if err != nil { return err diff --git a/pkg/state/types.go b/pkg/state/types.go index 242c994fbc9..0aefcca4048 100644 --- a/pkg/state/types.go +++ b/pkg/state/types.go @@ -7,6 +7,8 @@ import ( type Content struct { // PID is the ID of the process to which the state belongs PID int `json:"pid"` + // Platform indicates on which platform the session works + Platform string `json:"platform"` // ForwardedPorts are the ports forwarded during odo dev session ForwardedPorts []api.ForwardedPort `json:"forwardedPorts"` } From 26b28497521afc41d10ce757b09a40413fff4da4 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Thu, 6 Apr 2023 08:14:49 +0200 Subject: [PATCH 05/13] Overwrite devstate.json if PId not exists --- pkg/state/process_unix.go | 38 ++++++++++++++++++++++++++++++++++++ pkg/state/process_windows.go | 17 ++++++++++++++++ pkg/state/state.go | 10 ++++++++++ 3 files changed, 65 insertions(+) create mode 100644 pkg/state/process_unix.go create mode 100644 pkg/state/process_windows.go diff --git a/pkg/state/process_unix.go b/pkg/state/process_unix.go new file mode 100644 index 00000000000..988f16770bd --- /dev/null +++ b/pkg/state/process_unix.go @@ -0,0 +1,38 @@ +//go:build aix || darwin || dragonfly || freebsd || linux || netbsd || openbsd || solaris || zos +// +build aix darwin dragonfly freebsd linux netbsd openbsd solaris zos + +package state + +import ( + "fmt" + "os" + "syscall" +) + +func pidExists(pid int) (bool, error) { + if pid <= 0 { + return false, fmt.Errorf("invalid pid %v", pid) + } + proc, err := os.FindProcess(pid) + if err != nil { + return false, nil + } + err = proc.Signal(syscall.Signal(0)) + if err == nil { + return true, nil + } + if err.Error() == "os: process already finished" { + return false, nil + } + errno, ok := err.(syscall.Errno) + if !ok { + return false, err + } + switch errno { + case syscall.ESRCH: + return false, nil + case syscall.EPERM: + return true, nil + } + return false, err +} diff --git a/pkg/state/process_windows.go b/pkg/state/process_windows.go new file mode 100644 index 00000000000..890b93909f2 --- /dev/null +++ b/pkg/state/process_windows.go @@ -0,0 +1,17 @@ +package state + +import ( + "fmt" + "os" +) + +func pidExists(pid int) (bool, error) { + if pid <= 0 { + return false, fmt.Errorf("invalid pid %v", pid) + } + _, err := os.FindProcess(pid) + if err != nil { + return false, nil + } + return true, nil +} diff --git a/pkg/state/state.go b/pkg/state/state.go index 9fa70688fa6..c9e6b546c69 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -154,5 +154,15 @@ func (o *State) isFreeOrOwnedBy(pid int) (bool, error) { // File is owned by process return true, nil } + + exists, err := pidExists(savedContent.PID) + if err != nil { + return false, err + } + if !exists { + // Process already finished + return true, nil + } + return false, nil } From 88ea70fcf73935f930567ec69311c08d12ec35e9 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Thu, 6 Apr 2023 15:18:43 +0200 Subject: [PATCH 06/13] odo describe component displays forwarded ports from podman/cluster --- pkg/state/const.go | 1 + pkg/state/state.go | 64 +++++++++++++++++++++++++++-------- pkg/state/state_test.go | 74 ++++++++++++++++++++++++++++++++++++++--- 3 files changed, 121 insertions(+), 18 deletions(-) diff --git a/pkg/state/const.go b/pkg/state/const.go index 58f2b85391e..d8adc11eb7a 100644 --- a/pkg/state/const.go +++ b/pkg/state/const.go @@ -1,4 +1,5 @@ package state +const _dirpath = "./.odo" const _filepath = "./.odo/devstate.json" const _filepathPid = "./.odo/devstate.%d.json" diff --git a/pkg/state/state.go b/pkg/state/state.go index c9e6b546c69..4f577637e86 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -8,6 +8,7 @@ import ( "io/fs" "os" "path/filepath" + "regexp" "github.com/redhat-developer/odo/pkg/api" "github.com/redhat-developer/odo/pkg/odo/commonflags" @@ -43,16 +44,27 @@ func (o *State) SetForwardedPorts(ctx context.Context, fwPorts []api.ForwardedPo func (o *State) GetForwardedPorts(ctx context.Context) ([]api.ForwardedPort, error) { var ( - pid = odocontext.GetPID(ctx) + result []api.ForwardedPort + platforms []string + platform = fcontext.GetPlatform(ctx, "") ) - err := o.read(pid) - if err != nil { - if errors.Is(err, fs.ErrNotExist) { - return nil, nil // if the state file does not exist, no ports are forwarded + if platform == "" { + platforms = []string{commonflags.PlatformCluster, commonflags.PlatformPodman} + } else { + platforms = []string{platform} + } + + for _, platform = range platforms { + content, err := o.read(platform) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + continue // if the state file does not exist, no ports are forwarded + } + return nil, err } - return nil, err + result = append(result, content.ForwardedPorts...) } - return o.content.ForwardedPorts, err + return result, nil } func (o *State) SaveExit(ctx context.Context) error { @@ -90,12 +102,39 @@ func (o *State) save(pid int) error { return o.fs.WriteFile(getFilename(pid), jsonContent, 0644) } -func (o *State) read(pid int) error { - jsonContent, err := o.fs.ReadFile(getFilename(pid)) +// read returns the content of the devstate.${PID}.json file for the platform +func (o *State) read(platform string) (Content, error) { + + var content Content + + // We could use Glob, but it is not implemented by the Filesystem abstraction + entries, err := o.fs.ReadDir(_dirpath) if err != nil { - return err + return Content{}, nil + } + re := regexp.MustCompile(`^devstate\.[0-9]*\.json$`) + for _, entry := range entries { + if !re.MatchString(entry.Name()) { + continue + } + jsonContent, err := o.fs.ReadFile(filepath.Join(_dirpath, entry.Name())) + if err != nil { + return Content{}, err + } + err = json.Unmarshal(jsonContent, &content) + if err != nil { + return Content{}, err + } + if content.Platform == platform { + break + } else { + content = Content{} + } + } + if content.Platform == "" { + return Content{}, fs.ErrNotExist } - return json.Unmarshal(jsonContent, &o.content) + return content, nil } func (o *State) delete(pid int) error { @@ -103,9 +142,6 @@ func (o *State) delete(pid int) error { } func getFilename(pid int) string { - if pid == 0 { - return _filepath - } return fmt.Sprintf(_filepathPid, pid) } diff --git a/pkg/state/state_test.go b/pkg/state/state_test.go index 6f713ab8016..4e7ff04e31f 100644 --- a/pkg/state/state_test.go +++ b/pkg/state/state_test.go @@ -154,7 +154,8 @@ func TestState_SaveExit(t *testing.T) { } func TestState_GetForwardedPorts(t *testing.T) { - content1 := Content{ + contentPodman := Content{ + Platform: "podman", ForwardedPorts: []api.ForwardedPort{ { ContainerName: "acontainer", @@ -164,6 +165,17 @@ func TestState_GetForwardedPorts(t *testing.T) { }, }, } + contentCluster := Content{ + Platform: "cluster", + ForwardedPorts: []api.ForwardedPort{ + { + ContainerName: "acontainer", + LocalAddress: "localhost", + LocalPort: 20002, + ContainerPort: 3000, + }, + }, + } type fields struct { content Content fs func(t *testing.T) filesystem.Filesystem @@ -175,12 +187,33 @@ func TestState_GetForwardedPorts(t *testing.T) { wantErr bool }{ { - name: "get forwarded ports", + name: "get forwarded ports, only deployed on podman", + fields: fields{ + content: Content{}, + fs: func(t *testing.T) filesystem.Filesystem { + fs := filesystem.NewFakeFs() + jsonContent, err := json.Marshal(contentPodman) + if err != nil { + t.Errorf("Error marshaling data") + } + pid := 1 + err = fs.WriteFile(getFilename(pid), jsonContent, 0644) + if err != nil { + t.Errorf("Error saving content to file") + } + return fs + }, + }, + want: contentPodman.ForwardedPorts, + wantErr: false, + }, + { + name: "get forwarded ports, only deployed on cluster", fields: fields{ content: Content{}, fs: func(t *testing.T) filesystem.Filesystem { fs := filesystem.NewFakeFs() - jsonContent, err := json.Marshal(content1) + jsonContent, err := json.Marshal(contentCluster) if err != nil { t.Errorf("Error marshaling data") } @@ -192,7 +225,40 @@ func TestState_GetForwardedPorts(t *testing.T) { return fs }, }, - want: content1.ForwardedPorts, + want: contentCluster.ForwardedPorts, + wantErr: false, + }, + { + name: "get forwarded ports, deployed on both podman and cluster", + fields: fields{ + content: Content{}, + fs: func(t *testing.T) filesystem.Filesystem { + fs := filesystem.NewFakeFs() + + pidCluster := 1 + jsonContentCluster, err := json.Marshal(contentCluster) + if err != nil { + t.Errorf("Error marshaling data") + } + err = fs.WriteFile(getFilename(pidCluster), jsonContentCluster, 0644) + if err != nil { + t.Errorf("Error saving content to file") + } + + pidPodman := 2 + jsonContentPodman, err := json.Marshal(contentPodman) + if err != nil { + t.Errorf("Error marshaling data") + } + err = fs.WriteFile(getFilename(pidPodman), jsonContentPodman, 0644) + if err != nil { + t.Errorf("Error saving content to file") + } + + return fs + }, + }, + want: append(append([]api.ForwardedPort{}, contentCluster.ForwardedPorts...), contentPodman.ForwardedPorts...), wantErr: false, }, } From 444adfdb4bdd2d26ed181c4f554c5620fce674b7 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Tue, 11 Apr 2023 10:32:10 +0200 Subject: [PATCH 07/13] Start only one session on platform --- pkg/odo/cli/dev/dev.go | 12 +++++++- pkg/state/errors.go | 15 ++++++++++ pkg/state/interface.go | 3 ++ pkg/state/state.go | 66 +++++++++++++++++++++++++++++++++++++++--- 4 files changed, 91 insertions(+), 5 deletions(-) create mode 100644 pkg/state/errors.go diff --git a/pkg/odo/cli/dev/dev.go b/pkg/odo/cli/dev/dev.go index 3895c19e6c7..9a928986cae 100644 --- a/pkg/odo/cli/dev/dev.go +++ b/pkg/odo/cli/dev/dev.go @@ -13,7 +13,7 @@ import ( "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" "github.com/spf13/cobra" - "k8s.io/klog" + "k8s.io/klog/v2" ktemplates "k8s.io/kubectl/pkg/util/templates" "github.com/redhat-developer/odo/pkg/api" @@ -33,6 +33,7 @@ import ( odoutil "github.com/redhat-developer/odo/pkg/odo/util" "github.com/redhat-developer/odo/pkg/podman" scontext "github.com/redhat-developer/odo/pkg/segment/context" + "github.com/redhat-developer/odo/pkg/state" "github.com/redhat-developer/odo/pkg/util" "github.com/redhat-developer/odo/pkg/version" ) @@ -229,6 +230,11 @@ func (o *DevOptions) Run(ctx context.Context) (err error) { log.Sectionf("Running on %s in Dev mode", deployingTo) + err = o.clientset.StateClient.Init(ctx) + if err != nil { + return err + } + return o.clientset.DevClient.Start( o.ctx, o.out, @@ -256,6 +262,10 @@ func (o *DevOptions) HandleSignal() error { } func (o *DevOptions) Cleanup(ctx context.Context, commandError error) { + if errors.As(commandError, &state.ErrAlreadyRunningOnPlatform{}) { + klog.V(4).Info("session already running, no need to cleanup") + return + } if commandError != nil { _ = o.clientset.DevClient.CleanupResources(ctx, log.GetStdout()) } diff --git a/pkg/state/errors.go b/pkg/state/errors.go new file mode 100644 index 00000000000..1f93fc24d04 --- /dev/null +++ b/pkg/state/errors.go @@ -0,0 +1,15 @@ +package state + +import "fmt" + +type ErrAlreadyRunningOnPlatform struct { + platform string +} + +func NewErrAlreadyRunningOnPlatform(platform string) ErrAlreadyRunningOnPlatform { + return ErrAlreadyRunningOnPlatform{platform: platform} +} + +func (e ErrAlreadyRunningOnPlatform) Error() string { + return fmt.Sprintf("a session is already running on platform %q", e.platform) +} diff --git a/pkg/state/interface.go b/pkg/state/interface.go index 2306c3cf98b..9029000786f 100644 --- a/pkg/state/interface.go +++ b/pkg/state/interface.go @@ -7,6 +7,9 @@ import ( ) type Client interface { + // Init creates a devstate file for the process + Init(ctx context.Context) error + // SetForwardedPorts sets the forwarded ports in the state file and saves it to the file, updating the metadata SetForwardedPorts(ctx context.Context, fwPorts []api.ForwardedPort) error diff --git a/pkg/state/state.go b/pkg/state/state.go index 4f577637e86..54242a4204b 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -30,6 +30,17 @@ func NewStateClient(fs filesystem.Filesystem) *State { } } +func (o *State) Init(ctx context.Context) error { + var ( + pid = odocontext.GetPID(ctx) + platform = fcontext.GetPlatform(ctx, commonflags.PlatformCluster) + ) + o.content.PID = pid + o.content.Platform = platform + return o.save(ctx, pid) + +} + func (o *State) SetForwardedPorts(ctx context.Context, fwPorts []api.ForwardedPort) error { var ( pid = odocontext.GetPID(ctx) @@ -39,7 +50,7 @@ func (o *State) SetForwardedPorts(ctx context.Context, fwPorts []api.ForwardedPo o.content.ForwardedPorts = fwPorts o.content.PID = pid o.content.Platform = platform - return o.save(pid) + return o.save(ctx, pid) } func (o *State) GetForwardedPorts(ctx context.Context) ([]api.ForwardedPort, error) { @@ -82,9 +93,14 @@ func (o *State) SaveExit(ctx context.Context) error { } // save writes the content structure in json format in file -func (o *State) save(pid int) error { +func (o *State) save(ctx context.Context, pid int) error { + + err := o.checkFirstInPlatform(ctx) + if err != nil { + return err + } - err := o.saveCommonIfOwner(pid) + err = o.saveCommonIfOwner(pid) if err != nil { return err } @@ -102,7 +118,7 @@ func (o *State) save(pid int) error { return o.fs.WriteFile(getFilename(pid), jsonContent, 0644) } -// read returns the content of the devstate.${PID}.json file for the platform +// read returns the content of the devstate.${PID}.json file for the given platform func (o *State) read(platform string) (Content, error) { var content Content @@ -202,3 +218,45 @@ func (o *State) isFreeOrOwnedBy(pid int) (bool, error) { return false, nil } + +func (o *State) checkFirstInPlatform(ctx context.Context) error { + var ( + pid = odocontext.GetPID(ctx) + platform = fcontext.GetPlatform(ctx, "cluster") + ) + + re := regexp.MustCompile(`^devstate\.[0-9]*\.json$`) + entries, err := o.fs.ReadDir(_dirpath) + if err != nil { + // No file found => no problem + return nil + } + for _, entry := range entries { + if !re.MatchString(entry.Name()) { + continue + } + jsonContent, err := o.fs.ReadFile(filepath.Join(_dirpath, entry.Name())) + if err != nil { + return err + } + var content Content + err = json.Unmarshal(jsonContent, &content) + if err != nil { + return err + } + if content.Platform == platform { + if content.PID == pid { + continue + } + exists, err := pidExists(content.PID) + if err != nil { + return err + } + if exists { + // Process exists => problem + return NewErrAlreadyRunningOnPlatform(platform) + } + } + } + return nil +} From f75c4b828d7bb2ce8cf1a3827aee03698781b54f Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Tue, 11 Apr 2023 11:26:31 +0200 Subject: [PATCH 08/13] Fix integration test --- pkg/odo/cli/dev/dev.go | 1 + pkg/state/state.go | 18 ++++++------------ tests/integration/cmd_dev_test.go | 2 +- 3 files changed, 8 insertions(+), 13 deletions(-) diff --git a/pkg/odo/cli/dev/dev.go b/pkg/odo/cli/dev/dev.go index 9a928986cae..606a362c594 100644 --- a/pkg/odo/cli/dev/dev.go +++ b/pkg/odo/cli/dev/dev.go @@ -232,6 +232,7 @@ func (o *DevOptions) Run(ctx context.Context) (err error) { err = o.clientset.StateClient.Init(ctx) if err != nil { + err = fmt.Errorf("unable to save state file: %v", err) return err } diff --git a/pkg/state/state.go b/pkg/state/state.go index 54242a4204b..e3d722476cb 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -137,10 +137,8 @@ func (o *State) read(platform string) (Content, error) { if err != nil { return Content{}, err } - err = json.Unmarshal(jsonContent, &content) - if err != nil { - return Content{}, err - } + // Ignore error, to handle empty file + _ = json.Unmarshal(jsonContent, &content) if content.Platform == platform { break } else { @@ -194,10 +192,8 @@ func (o *State) isFreeOrOwnedBy(pid int) (bool, error) { return false, err } var savedContent Content - err = json.Unmarshal(jsonContent, &savedContent) - if err != nil { - return false, err - } + // Ignore error, to handle empty file + _ = json.Unmarshal(jsonContent, &savedContent) if savedContent.PID == 0 { // PID is 0 in file, it is free return true, nil @@ -240,10 +236,8 @@ func (o *State) checkFirstInPlatform(ctx context.Context) error { return err } var content Content - err = json.Unmarshal(jsonContent, &content) - if err != nil { - return err - } + // Ignore error, to handle empty file + _ = json.Unmarshal(jsonContent, &content) if content.Platform == platform { if content.PID == pid { continue diff --git a/tests/integration/cmd_dev_test.go b/tests/integration/cmd_dev_test.go index bf54b6e26d8..7de1d88f8b5 100644 --- a/tests/integration/cmd_dev_test.go +++ b/tests/integration/cmd_dev_test.go @@ -277,7 +277,7 @@ var _ = Describe("odo dev command tests", func() { stdout := res.Out() stderr := res.Err() Expect(stdout).To(ContainSubstring("Cleaning")) - Expect(stderr).To(ContainSubstring("unable to save forwarded ports to state file")) + Expect(stderr).To(ContainSubstring("unable to save state file")) }) }) From 0832f7bfe2456e1d50944a6acdb44d3d383fd31b Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Thu, 13 Apr 2023 10:35:29 +0200 Subject: [PATCH 09/13] Review --- pkg/odo/cli/dev/dev.go | 2 +- pkg/state/errors.go | 5 ++-- pkg/state/state.go | 56 +++++++++++++++++++++--------------------- 3 files changed, 32 insertions(+), 31 deletions(-) diff --git a/pkg/odo/cli/dev/dev.go b/pkg/odo/cli/dev/dev.go index 606a362c594..2e35f481471 100644 --- a/pkg/odo/cli/dev/dev.go +++ b/pkg/odo/cli/dev/dev.go @@ -232,7 +232,7 @@ func (o *DevOptions) Run(ctx context.Context) (err error) { err = o.clientset.StateClient.Init(ctx) if err != nil { - err = fmt.Errorf("unable to save state file: %v", err) + err = fmt.Errorf("unable to save state file: %w", err) return err } diff --git a/pkg/state/errors.go b/pkg/state/errors.go index 1f93fc24d04..329c9af5806 100644 --- a/pkg/state/errors.go +++ b/pkg/state/errors.go @@ -4,12 +4,13 @@ import "fmt" type ErrAlreadyRunningOnPlatform struct { platform string + pid int } -func NewErrAlreadyRunningOnPlatform(platform string) ErrAlreadyRunningOnPlatform { +func NewErrAlreadyRunningOnPlatform(platform string, pid int) ErrAlreadyRunningOnPlatform { return ErrAlreadyRunningOnPlatform{platform: platform} } func (e ErrAlreadyRunningOnPlatform) Error() string { - return fmt.Sprintf("a session is already running on platform %q", e.platform) + return fmt.Sprintf("a session with PID %d is already running on platform %q", e.pid, e.platform) } diff --git a/pkg/state/state.go b/pkg/state/state.go index e3d722476cb..4434c0fc0fa 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -105,17 +105,21 @@ func (o *State) save(ctx context.Context, pid int) error { return err } + return writeStateFile(getFilename(pid)) +} + +func writeStateFile(path string) error { jsonContent, err := json.MarshalIndent(o.content, "", " ") if err != nil { return err } // .odo directory is supposed to exist, don't create it - dir := filepath.Dir(getFilename(pid)) + dir := filepath.Dir(path) err = os.MkdirAll(dir, 0750) if err != nil { return err } - return o.fs.WriteFile(getFilename(pid), jsonContent, 0644) + return o.fs.WriteFile(path, jsonContent, 0644) } // read returns the content of the devstate.${PID}.json file for the given platform @@ -169,26 +173,16 @@ func (o *State) saveCommonIfOwner(pid int) error { return nil } - jsonContent, err := json.MarshalIndent(o.content, "", " ") - if err != nil { - return err - } - // .odo directory is supposed to exist, don't create it - dir := filepath.Dir(_filepath) - err = os.MkdirAll(dir, 0750) - if err != nil { - return err - } - return o.fs.WriteFile(_filepath, jsonContent, 0644) + return writeStateFile(_filepath) } func (o *State) isFreeOrOwnedBy(pid int) (bool, error) { jsonContent, err := o.fs.ReadFile(_filepath) if err != nil { if errors.Is(err, os.ErrNotExist) { + // File not found, it is free return true, nil } - // File not found, it is free return false, err } var savedContent Content @@ -224,8 +218,11 @@ func (o *State) checkFirstInPlatform(ctx context.Context) error { re := regexp.MustCompile(`^devstate\.[0-9]*\.json$`) entries, err := o.fs.ReadDir(_dirpath) if err != nil { - // No file found => no problem - return nil + if errors.Is(err, os.ErrNotExist) { + // No file found => no problem + return nil + } + return err } for _, entry := range entries { if !re.MatchString(entry.Name()) { @@ -238,18 +235,21 @@ func (o *State) checkFirstInPlatform(ctx context.Context) error { var content Content // Ignore error, to handle empty file _ = json.Unmarshal(jsonContent, &content) - if content.Platform == platform { - if content.PID == pid { - continue - } - exists, err := pidExists(content.PID) - if err != nil { - return err - } - if exists { - // Process exists => problem - return NewErrAlreadyRunningOnPlatform(platform) - } + + if content.Platform != platform { + continue + } + + if content.PID == pid { + continue + } + exists, err := pidExists(content.PID) + if err != nil { + return err + } + if exists { + // Process exists => problem + return NewErrAlreadyRunningOnPlatform(platform, content.PID) } } return nil From 16314e295fb0ebbbcef425622e7e3e69bcaa46bd Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Thu, 13 Apr 2023 13:01:12 +0200 Subject: [PATCH 10/13] Fix --- pkg/state/state.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/state/state.go b/pkg/state/state.go index 4434c0fc0fa..71e4d5db109 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -105,10 +105,10 @@ func (o *State) save(ctx context.Context, pid int) error { return err } - return writeStateFile(getFilename(pid)) + return o.writeStateFile(getFilename(pid)) } -func writeStateFile(path string) error { +func (o *State) writeStateFile(path string) error { jsonContent, err := json.MarshalIndent(o.content, "", " ") if err != nil { return err @@ -173,7 +173,7 @@ func (o *State) saveCommonIfOwner(pid int) error { return nil } - return writeStateFile(_filepath) + return o.writeStateFile(_filepath) } func (o *State) isFreeOrOwnedBy(pid int) (bool, error) { From fa4aac6e9dbeb59b3f6404365543938691ba6f0c Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Fri, 14 Apr 2023 09:02:41 +0200 Subject: [PATCH 11/13] Update pkg/state/errors.go Co-authored-by: Armel Soro --- pkg/state/errors.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/state/errors.go b/pkg/state/errors.go index 329c9af5806..e5db1679e7f 100644 --- a/pkg/state/errors.go +++ b/pkg/state/errors.go @@ -8,7 +8,10 @@ type ErrAlreadyRunningOnPlatform struct { } func NewErrAlreadyRunningOnPlatform(platform string, pid int) ErrAlreadyRunningOnPlatform { - return ErrAlreadyRunningOnPlatform{platform: platform} + return ErrAlreadyRunningOnPlatform{ + platform: platform, + pid: pid, + } } func (e ErrAlreadyRunningOnPlatform) Error() string { From ecebe7812d7db57c4470477d10350be4eb9fe8ac Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Mon, 17 Apr 2023 10:39:01 +0200 Subject: [PATCH 12/13] Integration tests --- tests/integration/cmd_dev_test.go | 55 ++++++++++++++++++++++++------- 1 file changed, 43 insertions(+), 12 deletions(-) diff --git a/tests/integration/cmd_dev_test.go b/tests/integration/cmd_dev_test.go index 7de1d88f8b5..7870897f3c6 100644 --- a/tests/integration/cmd_dev_test.go +++ b/tests/integration/cmd_dev_test.go @@ -2,6 +2,7 @@ package integration import ( "bufio" + "encoding/json" "fmt" "io" "net/http" @@ -23,6 +24,7 @@ import ( "github.com/redhat-developer/odo/pkg/labels" "github.com/redhat-developer/odo/pkg/remotecmd" segment "github.com/redhat-developer/odo/pkg/segment/context" + "github.com/redhat-developer/odo/pkg/state" "github.com/redhat-developer/odo/pkg/storage" "github.com/redhat-developer/odo/pkg/util" @@ -3011,18 +3013,47 @@ CMD ["npm", "start"] devSession.WaitEnd() }) - It("should create a state file containing forwarded ports", func() { - Expect(helper.VerifyFileExists(stateFile)).To(BeTrue()) - contentJSON, err := os.ReadFile(stateFile) - Expect(err).ToNot(HaveOccurred()) - helper.JsonPathContentIs(string(contentJSON), "forwardedPorts.0.containerName", "runtime") - helper.JsonPathContentIs(string(contentJSON), "forwardedPorts.1.containerName", "runtime") - helper.JsonPathContentIs(string(contentJSON), "forwardedPorts.0.localAddress", "127.0.0.1") - helper.JsonPathContentIs(string(contentJSON), "forwardedPorts.1.localAddress", "127.0.0.1") - helper.JsonPathContentIs(string(contentJSON), "forwardedPorts.0.containerPort", "3000") - helper.JsonPathContentIs(string(contentJSON), "forwardedPorts.1.containerPort", "4567") - helper.JsonPathContentIsValidUserPort(string(contentJSON), "forwardedPorts.0.localPort") - helper.JsonPathContentIsValidUserPort(string(contentJSON), "forwardedPorts.1.localPort") + It("should fail running a second session on the same platform", func() { + _, _, _, err := helper.WaitForDevModeToContain(helper.DevSessionOpts{ + RunOnPodman: podman, + }, "unable to save state file", true) + Expect(err).To(HaveOccurred()) + }) + + It("should create state files containing information, including forwarded ports", func() { + var pid int + var contentJSON []byte + By("creating a devsate.json file", func() { + Expect(helper.VerifyFileExists(stateFile)).To(BeTrue()) + var err error + contentJSON, err = os.ReadFile(stateFile) + Expect(err).ToNot(HaveOccurred()) + helper.JsonPathExist(string(contentJSON), "pid") + platform := "cluster" + if podman { + platform = "podman" + } + helper.JsonPathContentIs(string(contentJSON), "platform", platform) + helper.JsonPathContentIs(string(contentJSON), "forwardedPorts.0.containerName", "runtime") + helper.JsonPathContentIs(string(contentJSON), "forwardedPorts.1.containerName", "runtime") + helper.JsonPathContentIs(string(contentJSON), "forwardedPorts.0.localAddress", "127.0.0.1") + helper.JsonPathContentIs(string(contentJSON), "forwardedPorts.1.localAddress", "127.0.0.1") + helper.JsonPathContentIs(string(contentJSON), "forwardedPorts.0.containerPort", "3000") + helper.JsonPathContentIs(string(contentJSON), "forwardedPorts.1.containerPort", "4567") + helper.JsonPathContentIsValidUserPort(string(contentJSON), "forwardedPorts.0.localPort") + helper.JsonPathContentIsValidUserPort(string(contentJSON), "forwardedPorts.1.localPort") + var content state.Content + err = json.Unmarshal(contentJSON, &content) + Expect(err).ShouldNot(HaveOccurred()) + pid = content.PID + fmt.Printf("PID: %d\n", pid) + }) + By("creating a devsate.$PID.json file with same content as devstate.json", func() { + pidStateFile := fmt.Sprintf(".odo/devstate.%d.json", pid) + pidContentJSON, err := os.ReadFile(pidStateFile) + Expect(err).ToNot(HaveOccurred()) + Expect(pidContentJSON).To(Equal(contentJSON)) + }) }) })) From 087716957fe66cbbc92c6c5cdf29d899f381033f Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Mon, 17 Apr 2023 13:15:31 +0200 Subject: [PATCH 13/13] Fix integration test --- tests/helper/helper_dev.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/helper/helper_dev.go b/tests/helper/helper_dev.go index aea7fb6cd25..88a8e875ffa 100644 --- a/tests/helper/helper_dev.go +++ b/tests/helper/helper_dev.go @@ -191,8 +191,10 @@ func (o *DevSession) Stop() { return } - err := terminateProc(o.session) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) + if o.session.ExitCode() == -1 { + err := terminateProc(o.session) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + } o.stopped = true }