From 0cefc43e15c890a62710752442a3e4ba23771fb4 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Mon, 24 Apr 2023 09:56:36 +0200 Subject: [PATCH 1/7] Same signature for reconcile in kubedev/podmandev --- pkg/dev/kubedev/kubedev.go | 7 ++++--- pkg/dev/podmandev/podmandev.go | 12 ++++++++++-- pkg/dev/podmandev/reconcile.go | 19 ++++++++++--------- 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/pkg/dev/kubedev/kubedev.go b/pkg/dev/kubedev/kubedev.go index 961c96bd7fc..a62ea89352c 100644 --- a/pkg/dev/kubedev/kubedev.go +++ b/pkg/dev/kubedev/kubedev.go @@ -88,6 +88,10 @@ func (o *DevClient) Start( var ( devfileObj = odocontext.GetDevfileObj(ctx) + + componentStatus = watch.ComponentStatus{ + ImageComponentsAutoApplied: make(map[string]devfilev1.ImageComponent), + } ) pushParameters := common.PushParameters{ @@ -96,9 +100,6 @@ func (o *DevClient) Start( } klog.V(4).Infoln("Creating inner-loop resources for the component") - componentStatus := watch.ComponentStatus{ - ImageComponentsAutoApplied: make(map[string]devfilev1.ImageComponent), - } err := o.reconcile(ctx, pushParameters, &componentStatus) if err != nil { return err diff --git a/pkg/dev/podmandev/podmandev.go b/pkg/dev/podmandev/podmandev.go index 7db0c996049..66f72f5bad8 100644 --- a/pkg/dev/podmandev/podmandev.go +++ b/pkg/dev/podmandev/podmandev.go @@ -80,16 +80,24 @@ func (o *DevClient) Start( var ( devfilePath = odocontext.GetDevfilePath(ctx) path = filepath.Dir(devfilePath) + devfileObj = odocontext.GetDevfileObj(ctx) componentStatus = watch.ComponentStatus{ ImageComponentsAutoApplied: make(map[string]devfilev1.ImageComponent), } ) - err := o.reconcile(ctx, options, &componentStatus) + pushParameters := common.PushParameters{ + StartOptions: options, + Devfile: *devfileObj, + } + + klog.V(4).Infoln("Creating inner-loop resources for the component") + err := o.reconcile(ctx, pushParameters, &componentStatus) if err != nil { return err } + klog.V(4).Infoln("Successfully created inner-loop resources") watch.PrintInfoMessage(options.Out, path, options.WatchFiles, promptMessage) @@ -172,7 +180,7 @@ func (o *DevClient) checkVolumesFree(pod *corev1.Pod) error { func (o *DevClient) watchHandler(ctx context.Context, pushParams common.PushParameters, componentStatus *watch.ComponentStatus) error { printWarningsOnDevfileChanges(ctx, pushParams.StartOptions) - return o.reconcile(ctx, pushParams.StartOptions, componentStatus) + return o.reconcile(ctx, pushParams, componentStatus) } func printWarningsOnDevfileChanges(ctx context.Context, options dev.StartOptions) { diff --git a/pkg/dev/podmandev/reconcile.go b/pkg/dev/podmandev/reconcile.go index ccfab4b7dda..603669ad658 100644 --- a/pkg/dev/podmandev/reconcile.go +++ b/pkg/dev/podmandev/reconcile.go @@ -31,20 +31,21 @@ import ( func (o *DevClient) reconcile( ctx context.Context, - options dev.StartOptions, + parameters common.PushParameters, componentStatus *watch.ComponentStatus, ) error { var ( appName = odocontext.GetApplication(ctx) componentName = odocontext.GetComponentName(ctx) - devfileObj = odocontext.GetDevfileObj(ctx) devfilePath = odocontext.GetDevfilePath(ctx) path = filepath.Dir(devfilePath) + options = parameters.StartOptions + devfileObj = parameters.Devfile ) - o.warnAboutK8sComponents(*devfileObj) + o.warnAboutK8sComponents(devfileObj) - err := o.buildPushAutoImageComponents(ctx, *devfileObj) + err := o.buildPushAutoImageComponents(ctx, devfileObj) if err != nil { return err } @@ -62,7 +63,7 @@ func (o *DevClient) reconcile( // PostStart events from the devfile will only be executed when the component // didn't previously exist - if !componentStatus.PostStartEventsDone && libdevfile.HasPostStartEvents(*devfileObj) { + if !componentStatus.PostStartEventsDone && libdevfile.HasPostStartEvents(devfileObj) { execHandler := component.NewExecHandler( o.podmanClient, o.execClient, @@ -72,7 +73,7 @@ func (o *DevClient) reconcile( "Executing post-start command in container", false, /* TODO */ ) - err = libdevfile.ExecPostStartEvents(ctx, *devfileObj, execHandler) + err = libdevfile.ExecPostStartEvents(ctx, devfileObj, execHandler) if err != nil { return err } @@ -90,7 +91,7 @@ func (o *DevClient) reconcile( "Building your application in container", false, /* TODO */ ) - return libdevfile.Build(ctx, *devfileObj, options.BuildCommand, execHandler) + return libdevfile.Build(ctx, devfileObj, options.BuildCommand, execHandler) } err = doExecuteBuildCommand() if err != nil { @@ -113,7 +114,7 @@ func (o *DevClient) reconcile( appName: appName, componentName: componentName, } - err = libdevfile.ExecuteCommandByNameAndKind(ctx, *devfileObj, cmdName, cmdKind, &cmdHandler, false) + err = libdevfile.ExecuteCommandByNameAndKind(ctx, devfileObj, cmdName, cmdKind, &cmdHandler, false) if err != nil { return err } @@ -140,7 +141,7 @@ func (o *DevClient) reconcile( if options.ForwardLocalhost { // Port-forwarding is enabled by executing dedicated socat commands - err = o.portForwardClient.StartPortForwarding(ctx, *devfileObj, componentName, options.Debug, options.RandomPorts, options.Out, options.ErrOut, fwPorts) + err = o.portForwardClient.StartPortForwarding(ctx, devfileObj, componentName, options.Debug, options.RandomPorts, options.Out, options.ErrOut, fwPorts) if err != nil { return common.NewErrPortForward(err) } From c0217acccb3afda85de48c2ce9d094f40407eb26 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Mon, 24 Apr 2023 10:31:31 +0200 Subject: [PATCH 2/7] Do not call initial reconcile --- pkg/dev/kubedev/kubedev.go | 13 -------- pkg/dev/podmandev/podmandev.go | 20 ++---------- .../parent-devfile-commands-only.yaml | 20 ++++++++++++ .../parent-devfile-components-only.yaml | 15 +++++++++ .../testdata/parent-devfile-empty.yaml | 3 ++ pkg/libdevfile/testdata/parent-devfile.yaml | 32 +++++++++++++++++++ pkg/watch/watch.go | 6 ++++ 7 files changed, 79 insertions(+), 30 deletions(-) create mode 100644 pkg/libdevfile/testdata/parent-devfile-commands-only.yaml create mode 100644 pkg/libdevfile/testdata/parent-devfile-components-only.yaml create mode 100644 pkg/libdevfile/testdata/parent-devfile-empty.yaml create mode 100644 pkg/libdevfile/testdata/parent-devfile.yaml diff --git a/pkg/dev/kubedev/kubedev.go b/pkg/dev/kubedev/kubedev.go index a62ea89352c..5f176687536 100644 --- a/pkg/dev/kubedev/kubedev.go +++ b/pkg/dev/kubedev/kubedev.go @@ -15,7 +15,6 @@ import ( "github.com/redhat-developer/odo/pkg/devfile/location" "github.com/redhat-developer/odo/pkg/exec" "github.com/redhat-developer/odo/pkg/kclient" - odocontext "github.com/redhat-developer/odo/pkg/odo/context" "github.com/redhat-developer/odo/pkg/portForward" "github.com/redhat-developer/odo/pkg/preference" "github.com/redhat-developer/odo/pkg/sync" @@ -87,24 +86,12 @@ func (o *DevClient) Start( klog.V(4).Infoln("Creating new adapter") var ( - devfileObj = odocontext.GetDevfileObj(ctx) - componentStatus = watch.ComponentStatus{ ImageComponentsAutoApplied: make(map[string]devfilev1.ImageComponent), } ) - pushParameters := common.PushParameters{ - StartOptions: options, - Devfile: *devfileObj, - } - klog.V(4).Infoln("Creating inner-loop resources for the component") - err := o.reconcile(ctx, pushParameters, &componentStatus) - if err != nil { - return err - } - klog.V(4).Infoln("Successfully created inner-loop resources") watchParameters := watch.WatchParameters{ StartOptions: options, diff --git a/pkg/dev/podmandev/podmandev.go b/pkg/dev/podmandev/podmandev.go index 66f72f5bad8..d90a2486d8c 100644 --- a/pkg/dev/podmandev/podmandev.go +++ b/pkg/dev/podmandev/podmandev.go @@ -5,7 +5,6 @@ import ( "context" "encoding/json" "fmt" - "path/filepath" "strings" devfilev1 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" @@ -77,29 +76,15 @@ func (o *DevClient) Start( ctx context.Context, options dev.StartOptions, ) error { - var ( - devfilePath = odocontext.GetDevfilePath(ctx) - path = filepath.Dir(devfilePath) - devfileObj = odocontext.GetDevfileObj(ctx) + klog.V(4).Infoln("Creating new adapter") + var ( componentStatus = watch.ComponentStatus{ ImageComponentsAutoApplied: make(map[string]devfilev1.ImageComponent), } ) - pushParameters := common.PushParameters{ - StartOptions: options, - Devfile: *devfileObj, - } - klog.V(4).Infoln("Creating inner-loop resources for the component") - err := o.reconcile(ctx, pushParameters, &componentStatus) - if err != nil { - return err - } - klog.V(4).Infoln("Successfully created inner-loop resources") - - watch.PrintInfoMessage(options.Out, path, options.WatchFiles, promptMessage) watchParameters := watch.WatchParameters{ StartOptions: options, @@ -179,6 +164,7 @@ func (o *DevClient) checkVolumesFree(pod *corev1.Pod) error { } func (o *DevClient) watchHandler(ctx context.Context, pushParams common.PushParameters, componentStatus *watch.ComponentStatus) error { + pushParams.Devfile = *odocontext.GetDevfileObj(ctx) // TOO reload devfile from disk printWarningsOnDevfileChanges(ctx, pushParams.StartOptions) return o.reconcile(ctx, pushParams, componentStatus) } diff --git a/pkg/libdevfile/testdata/parent-devfile-commands-only.yaml b/pkg/libdevfile/testdata/parent-devfile-commands-only.yaml new file mode 100644 index 00000000000..206682178dc --- /dev/null +++ b/pkg/libdevfile/testdata/parent-devfile-commands-only.yaml @@ -0,0 +1,20 @@ +commands: +- exec: + commandLine: GOCACHE=${PROJECT_SOURCE}/.cache go build main.go + component: runtime + group: + isDefault: true + kind: build + workingDir: ${PROJECT_SOURCE} + id: build +- exec: + commandLine: ./main + component: runtime + group: + isDefault: true + kind: run + workingDir: ${PROJECT_SOURCE} + id: run +schemaVersion: 2.1.0 +metadata: + name: parent diff --git a/pkg/libdevfile/testdata/parent-devfile-components-only.yaml b/pkg/libdevfile/testdata/parent-devfile-components-only.yaml new file mode 100644 index 00000000000..74d64ff35bf --- /dev/null +++ b/pkg/libdevfile/testdata/parent-devfile-components-only.yaml @@ -0,0 +1,15 @@ +components: +- container: + endpoints: + - name: http + targetPort: 8080 + image: quay.io/devfile/golang:latest + memoryLimit: 1024Mi + mountSources: true + name: runtime +- kubernetes: + uri: "manifest.yaml" + name: kube-cmp +metadata: + name: my-go-app +schemaVersion: 2.1.0 diff --git a/pkg/libdevfile/testdata/parent-devfile-empty.yaml b/pkg/libdevfile/testdata/parent-devfile-empty.yaml new file mode 100644 index 00000000000..a3e59f1af10 --- /dev/null +++ b/pkg/libdevfile/testdata/parent-devfile-empty.yaml @@ -0,0 +1,3 @@ +metadata: + name: my-go-app +schemaVersion: 2.1.0 diff --git a/pkg/libdevfile/testdata/parent-devfile.yaml b/pkg/libdevfile/testdata/parent-devfile.yaml new file mode 100644 index 00000000000..08824612b9b --- /dev/null +++ b/pkg/libdevfile/testdata/parent-devfile.yaml @@ -0,0 +1,32 @@ +commands: +- exec: + commandLine: GOCACHE=${PROJECT_SOURCE}/.cache go build main.go + component: runtime + group: + isDefault: true + kind: build + workingDir: ${PROJECT_SOURCE} + id: build +- exec: + commandLine: ./main + component: runtime + group: + isDefault: true + kind: run + workingDir: ${PROJECT_SOURCE} + id: run +components: +- container: + endpoints: + - name: http + targetPort: 8080 + image: quay.io/devfile/golang:latest + memoryLimit: 1024Mi + mountSources: true + name: runtime +- kubernetes: + uri: "manifest.yaml" + name: kube-cmp +metadata: + name: my-go-app +schemaVersion: 2.1.0 diff --git a/pkg/watch/watch.go b/pkg/watch/watch.go index 5e75167c2c0..126d30be028 100644 --- a/pkg/watch/watch.go +++ b/pkg/watch/watch.go @@ -159,6 +159,12 @@ func (o *WatchClient) WatchAndPush(ctx context.Context, parameters WatchParamete } o.keyWatcher = getKeyWatcher(ctx, parameters.StartOptions.Out) + + _, err = o.processEvents(ctx, parameters, nil, nil, &componentStatus, NewExpBackoff()) + if err != nil { + return err + } + return o.eventWatcher(ctx, parameters, evaluateFileChanges, o.processEvents, componentStatus) } From a7af3b56085e30cb239977e6d1eb58974768d202 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Wed, 26 Apr 2023 09:30:24 +0200 Subject: [PATCH 3/7] Do not retry + filter deployment events --- pkg/watch/watch.go | 74 ++++++++++++----------------------------- pkg/watch/watch_test.go | 4 +-- 2 files changed, 23 insertions(+), 55 deletions(-) diff --git a/pkg/watch/watch.go b/pkg/watch/watch.go index 126d30be028..0c8f37b9152 100644 --- a/pkg/watch/watch.go +++ b/pkg/watch/watch.go @@ -47,6 +47,10 @@ type WatchClient struct { // true to force sync, used when manual sync forceSync bool + + // deploymentGeneration indicates the generation of the latest observed Deployment + deploymentGeneration int64 + readyReplicas int32 } var _ Client = (*WatchClient)(nil) @@ -83,7 +87,7 @@ type evaluateChangesFunc func(events []fsnotify.Event, path string, fileIgnores // processEventsFunc processes the events received on the watcher. It uses the WatchParameters to trigger watch handler and writes to out // It returns a Duration after which to recall in case of error -type processEventsFunc func(ctx context.Context, parameters WatchParameters, changedFiles, deletedPaths []string, componentStatus *ComponentStatus, backoff *ExpBackoff) (*time.Duration, error) +type processEventsFunc func(ctx context.Context, parameters WatchParameters, changedFiles, deletedPaths []string, componentStatus *ComponentStatus) error func (o *WatchClient) WatchAndPush(ctx context.Context, parameters WatchParameters, componentStatus ComponentStatus) error { var ( @@ -160,7 +164,7 @@ func (o *WatchClient) WatchAndPush(ctx context.Context, parameters WatchParamete o.keyWatcher = getKeyWatcher(ctx, parameters.StartOptions.Out) - _, err = o.processEvents(ctx, parameters, nil, nil, &componentStatus, NewExpBackoff()) + err = o.processEvents(ctx, parameters, nil, nil, &componentStatus) if err != nil { return err } @@ -187,8 +191,6 @@ func (o *WatchClient) eventWatcher( out = parameters.StartOptions.Out ) - expBackoff := NewExpBackoff() - var events []fsnotify.Event // sourcesTimer helps collect multiple events that happen in a quick succession. We start with 1ms as we don't care much @@ -208,10 +210,6 @@ func (o *WatchClient) eventWatcher( deployTimer := time.NewTimer(time.Millisecond) <-deployTimer.C - // retryTimer is a timer used to retry later a sync that has failed - retryTimer := time.NewTimer(time.Millisecond) - <-retryTimer.C - podsPhases := NewPodPhases() for { @@ -240,7 +238,7 @@ func (o *WatchClient) eventWatcher( componentStatus.State = StateSyncOutdated fmt.Fprintf(out, "Pushing files...\n\n") - retry, err := processEventsHandler(ctx, parameters, changedFiles, deletedPaths, &componentStatus, expBackoff) + err := processEventsHandler(ctx, parameters, changedFiles, deletedPaths, &componentStatus) o.forceSync = false if err != nil { return err @@ -250,13 +248,6 @@ func (o *WatchClient) eventWatcher( events = []fsnotify.Event{} // empty the events slice to capture new events } - if retry != nil { - retryTimer.Reset(*retry) - } else { - retryTimer.Reset(time.Millisecond) - <-retryTimer.C - } - case watchErr := <-o.sourcesWatcher.Errors: return watchErr @@ -269,53 +260,33 @@ func (o *WatchClient) eventWatcher( case ev := <-o.deploymentWatcher.ResultChan(): switch obj := ev.Object.(type) { case *appsv1.Deployment: - klog.V(4).Infof("deployment watcher Event: Type: %s, name: %s, rv: %s, pods: %d\n", - ev.Type, obj.GetName(), obj.GetResourceVersion(), obj.Status.ReadyReplicas) - deployTimer.Reset(300 * time.Millisecond) + klog.V(0).Infof("deployment watcher Event: Type: %s, name: %s, rv: %s, generation: %d, pods: %d\n", + ev.Type, obj.GetName(), obj.GetResourceVersion(), obj.GetGeneration(), obj.Status.ReadyReplicas) + if obj.GetGeneration() > o.deploymentGeneration || obj.Status.ReadyReplicas != o.readyReplicas { + o.deploymentGeneration = obj.GetGeneration() + o.readyReplicas = obj.Status.ReadyReplicas + deployTimer.Reset(300 * time.Millisecond) + } case *metav1.Status: klog.V(4).Infof("Status: %+v\n", obj) } case <-deployTimer.C: - retry, err := processEventsHandler(ctx, parameters, nil, nil, &componentStatus, expBackoff) + err := processEventsHandler(ctx, parameters, nil, nil, &componentStatus) if err != nil { return err } - if retry != nil { - retryTimer.Reset(*retry) - } else { - retryTimer.Reset(time.Millisecond) - <-retryTimer.C - } case <-o.devfileWatcher.Events: devfileTimer.Reset(100 * time.Millisecond) case <-devfileTimer.C: fmt.Fprintf(out, "Updating Component...\n\n") - retry, err := processEventsHandler(ctx, parameters, nil, nil, &componentStatus, expBackoff) - if err != nil { - return err - } - if retry != nil { - retryTimer.Reset(*retry) - } else { - retryTimer.Reset(time.Millisecond) - <-retryTimer.C - } - - case <-retryTimer.C: - retry, err := processEventsHandler(ctx, parameters, nil, nil, &componentStatus, expBackoff) + err := processEventsHandler(ctx, parameters, nil, nil, &componentStatus) if err != nil { return err } - if retry != nil { - retryTimer.Reset(*retry) - } else { - retryTimer.Reset(time.Millisecond) - <-retryTimer.C - } case ev := <-o.podWatcher.ResultChan(): switch ev.Type { @@ -427,8 +398,7 @@ func (o *WatchClient) processEvents( parameters WatchParameters, changedFiles, deletedPaths []string, componentStatus *ComponentStatus, - backoff *ExpBackoff, -) (*time.Duration, error) { +) error { var ( devfilePath = odocontext.GetDevfilePath(ctx) path = filepath.Dir(devfilePath) @@ -453,7 +423,7 @@ func (o *WatchClient) processEvents( err := parameters.DevfileWatchHandler(ctx, pushParams, componentStatus) if err != nil { if isFatal(err) { - return nil, err + return err } klog.V(4).Infof("Error from Push: %v", err) // Log and output, but intentionally not exiting on error here. @@ -471,19 +441,17 @@ func (o *WatchClient) processEvents( if parameters.StartOptions.WatchFiles { fmt.Fprintf(out, "%s - %s\n\n", PushErrorString, err.Error()) } else { - return nil, err + return err } } - wait := backoff.Delay() - return &wait, nil + return nil } - backoff.Reset() if oldStatus.State != StateReady && componentStatus.State == StateReady || !reflect.DeepEqual(oldStatus.EndpointsForwarded, componentStatus.EndpointsForwarded) { PrintInfoMessage(out, path, parameters.StartOptions.WatchFiles, parameters.PromptMessage) } - return nil, nil + return nil } func shouldIgnoreEvent(event fsnotify.Event) (ignoreEvent bool) { diff --git a/pkg/watch/watch_test.go b/pkg/watch/watch_test.go index c72eb12715a..34ff8d08b08 100644 --- a/pkg/watch/watch_test.go +++ b/pkg/watch/watch_test.go @@ -42,9 +42,9 @@ func evaluateChangesHandler(events []fsnotify.Event, path string, fileIgnores [] return changedFiles, deletedPaths } -func processEventsHandler(ctx context.Context, params WatchParameters, changedFiles, deletedPaths []string, componentStatus *ComponentStatus, backo *ExpBackoff) (*time.Duration, error) { +func processEventsHandler(ctx context.Context, params WatchParameters, changedFiles, deletedPaths []string, componentStatus *ComponentStatus) error { fmt.Fprintf(params.StartOptions.Out, "changedFiles %s deletedPaths %s\n", changedFiles, deletedPaths) - return nil, nil + return nil } type fakeWatcher struct{} From 20b069a7765ff6f560b92d7e855f458ef63cbde0 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Fri, 28 Apr 2023 09:27:24 +0200 Subject: [PATCH 4/7] Integration tests --- pkg/dev/kubedev/kubedev.go | 2 +- pkg/dev/podmandev/reconcile.go | 1 + pkg/watch/watch.go | 7 +- tests/helper/helper_dev.go | 4 + tests/integration/cmd_dev_test.go | 161 +++++++++++++++++++----------- 5 files changed, 112 insertions(+), 63 deletions(-) diff --git a/pkg/dev/kubedev/kubedev.go b/pkg/dev/kubedev/kubedev.go index 5f176687536..034ea6821b7 100644 --- a/pkg/dev/kubedev/kubedev.go +++ b/pkg/dev/kubedev/kubedev.go @@ -108,7 +108,7 @@ func (o *DevClient) regenerateAdapterAndPush(ctx context.Context, pushParams com devObj, err := devfile.ParseAndValidateFromFileWithVariables(location.DevfileLocation(""), pushParams.StartOptions.Variables) if err != nil { - return fmt.Errorf("unable to generate component from watch parameters: %w", err) + return fmt.Errorf("unable to read devfile: %w", err) } pushParams.Devfile = devObj diff --git a/pkg/dev/podmandev/reconcile.go b/pkg/dev/podmandev/reconcile.go index 603669ad658..07f71933714 100644 --- a/pkg/dev/podmandev/reconcile.go +++ b/pkg/dev/podmandev/reconcile.go @@ -55,6 +55,7 @@ func (o *DevClient) reconcile( return err } o.deployedPod = pod + componentStatus.State = watch.StateReady execRequired, err := o.syncFiles(ctx, options, pod, path) if err != nil { diff --git a/pkg/watch/watch.go b/pkg/watch/watch.go index 0c8f37b9152..7d22ab1b824 100644 --- a/pkg/watch/watch.go +++ b/pkg/watch/watch.go @@ -438,11 +438,8 @@ func (o *WatchClient) processEvents( fmt.Fprintf(out, "Updated Kubernetes config\n") } } else { - if parameters.StartOptions.WatchFiles { - fmt.Fprintf(out, "%s - %s\n\n", PushErrorString, err.Error()) - } else { - return err - } + fmt.Fprintf(out, "%s - %s\n\n", PushErrorString, err.Error()) + PrintInfoMessage(out, path, parameters.StartOptions.WatchFiles, parameters.PromptMessage) } return nil } diff --git a/tests/helper/helper_dev.go b/tests/helper/helper_dev.go index 7c077563ae1..8c2820dfd27 100644 --- a/tests/helper/helper_dev.go +++ b/tests/helper/helper_dev.go @@ -120,6 +120,7 @@ type DevSessionOpts struct { RunOnPodman bool TimeoutInSeconds int NoRandomPorts bool + NoWatch bool } // StartDevMode starts a dev session with `odo dev` @@ -139,6 +140,9 @@ func StartDevMode(options DevSessionOpts) (devSession DevSession, out []byte, er if !options.NoRandomPorts { args = append(args, "--random-ports") } + if options.NoWatch { + args = append(args, "--no-watch") + } args = append(args, options.CmdlineArgs...) cmd := Cmd("odo", args...) cmd.Cmd.Stdin = c.Tty() diff --git a/tests/integration/cmd_dev_test.go b/tests/integration/cmd_dev_test.go index 7c3d5fb85fe..e760af95bbd 100644 --- a/tests/integration/cmd_dev_test.go +++ b/tests/integration/cmd_dev_test.go @@ -2447,31 +2447,48 @@ CMD ["npm", "start"] } for _, podman := range []bool{false, true} { - podman := podman - When("running odo dev --no-watch and build command throws an error", helper.LabelPodmanIf(podman, func() { - var stderr string - BeforeEach(func() { - helper.CopyExampleDevFile( - filepath.Join("source", "devfiles", "nodejs", "devfile.yaml"), - filepath.Join(commonVar.Context, "devfile.yaml"), - helper.DevfileMetadataNameSetter(cmpName)) - helper.ReplaceString(filepath.Join(commonVar.Context, "devfile.yaml"), "npm install", "npm install-does-not-exist") - args := []string{"dev", "--no-watch", "--random-ports"} - if podman { - args = append(args, "--platform", "podman") - } - cmd := helper.Cmd("odo", args...) - stderr = cmd.ShouldFail().Err() - }) + for _, noWatch := range []bool{false, true} { + podman := podman + noWatch := noWatch + noWatchFlag := "" + if noWatch { + noWatchFlag = " --no-watch" + } + title := fmt.Sprintf("running odo dev%s and build command throws an error", noWatchFlag) + When(title, helper.LabelPodmanIf(podman, func() { + var session helper.DevSession + var stdout, stderr []byte + BeforeEach(func() { + helper.CopyExampleDevFile( + filepath.Join("source", "devfiles", "nodejs", "devfile.yaml"), + filepath.Join(commonVar.Context, "devfile.yaml"), + helper.DevfileMetadataNameSetter(cmpName)) + helper.ReplaceString(filepath.Join(commonVar.Context, "devfile.yaml"), "npm install", "npm install-does-not-exist") - It("should error out with some log", func() { - helper.MatchAllInOutput(stderr, []string{ - "unable to exec command", - "Usage: npm ", - "Did you mean one of these?", + var err error + session, stdout, stderr, _, err = helper.StartDevMode(helper.DevSessionOpts{ + RunOnPodman: podman, + NoWatch: noWatch, + }) + Expect(err).ToNot(HaveOccurred()) }) - }) - })) + + AfterEach(func() { + session.Stop() + session.WaitEnd() + }) + + It("should error out with some log", func() { + helper.MatchAllInOutput(string(stdout), []string{ + "unable to exec command", + }) + helper.MatchAllInOutput(string(stderr), []string{ + "Usage: npm ", + "Did you mean one of these?", + }) + }) + })) + } } for _, podman := range []bool{false, true} { @@ -2644,24 +2661,33 @@ CMD ["npm", "start"] It("should error out on an invalid command", func() { By("calling with an invalid build command", func() { - args := []string{"dev", "--random-ports", "--build-command", "build-command-does-not-exist"} - if podman { - args = append(args, "--platform", "podman") - } - cmd := helper.Cmd("odo", args...) - output := cmd.ShouldFail().Err() - Expect(output).To(ContainSubstring("no build command with name \"build-command-does-not-exist\" found in Devfile")) + + session, stdout, _, _, err := helper.StartDevMode(helper.DevSessionOpts{ + RunOnPodman: podman, + CmdlineArgs: []string{"--build-command", "build-command-does-not-exist"}, + }) + Expect(err).ToNot(HaveOccurred()) + defer func() { + session.Stop() + session.WaitEnd() + }() + + Expect(string(stdout)).To(ContainSubstring("no build command with name \"build-command-does-not-exist\" found in Devfile")) }) By("calling with a command of another kind (not build)", func() { // devrun is a valid run command, not a build command - args := []string{"dev", "--random-ports", "--build-command", "devrun"} - if podman { - args = append(args, "--platform", "podman") - } - cmd := helper.Cmd("odo", args...) - output := cmd.ShouldFail().Err() - Expect(output).To(ContainSubstring("no build command with name \"devrun\" found in Devfile")) + session, stdout, _, _, err := helper.StartDevMode(helper.DevSessionOpts{ + RunOnPodman: podman, + CmdlineArgs: []string{"--build-command", "devrun"}, + }) + Expect(err).ToNot(HaveOccurred()) + defer func() { + session.Stop() + session.WaitEnd() + }() + + Expect(string(stdout)).To(ContainSubstring("no build command with name \"devrun\" found in Devfile")) }) }) @@ -2705,24 +2731,32 @@ CMD ["npm", "start"] It("should error out on an invalid command", func() { By("calling with an invalid run command", func() { - args := []string{"dev", "--random-ports", "--run-command", "run-command-does-not-exist"} - if podman { - args = append(args, "--platform", "podman") - } - cmd := helper.Cmd("odo", args...) - output := cmd.ShouldFail().Err() - Expect(output).To(ContainSubstring("no run command with name \"run-command-does-not-exist\" found in Devfile")) + session, stdout, _, _, err := helper.StartDevMode(helper.DevSessionOpts{ + RunOnPodman: podman, + CmdlineArgs: []string{"--run-command", "run-command-does-not-exist"}, + }) + Expect(err).ToNot(HaveOccurred()) + defer func() { + session.Stop() + session.WaitEnd() + }() + + Expect(string(stdout)).To(ContainSubstring("no run command with name \"run-command-does-not-exist\" found in Devfile")) }) By("calling with a command of another kind (not run)", func() { // devbuild is a valid build command, not a run command - args := []string{"dev", "--random-ports", "--run-command", "devbuild"} - if podman { - args = append(args, "--platform", "podman") - } - cmd := helper.Cmd("odo", args...) - output := cmd.ShouldFail().Err() - Expect(output).To(ContainSubstring("no run command with name \"devbuild\" found in Devfile")) + session, stdout, _, _, err := helper.StartDevMode(helper.DevSessionOpts{ + RunOnPodman: podman, + CmdlineArgs: []string{"--run-command", "devbuild"}, + }) + Expect(err).ToNot(HaveOccurred()) + defer func() { + session.Stop() + session.WaitEnd() + }() + + Expect(string(stdout)).To(ContainSubstring("no run command with name \"devbuild\" found in Devfile")) }) }) @@ -3665,11 +3699,17 @@ CMD ["npm", "start"] helper.DevfileMetadataNameSetter(cmpName)) helper.ReplaceString(filepath.Join(commonVar.Context, "devfile.yaml"), "registry.access.redhat.com/ubi8/nodejs", "registry.access.redhat.com/ubi8/nose") }) - It("should fail with an error and cleanup resources", func() { - errContents := helper.Cmd("odo", "dev", "--platform=podman").ShouldFail().Err() - helper.MatchAllInOutput(errContents, []string{"Complete Podman output", "registry.access.redhat.com/ubi8/nose", "Repo not found"}) - component := helper.NewComponent(cmpName, "app", labels.ComponentDevMode, commonVar.Project, commonVar.CliRunner) - component.ExpectIsNotDeployed() + It("should fail with an error", func() { + session, stdout, _, _, err := helper.StartDevMode(helper.DevSessionOpts{ + RunOnPodman: true, + }) + Expect(err).ToNot(HaveOccurred()) + defer func() { + session.Stop() + session.WaitEnd() + }() + + helper.MatchAllInOutput(string(stdout), []string{"Complete Podman output", "registry.access.redhat.com/ubi8/nose", "Repo not found"}) }) }) @@ -3765,7 +3805,14 @@ CMD ["npm", "start"] }) It("should error out if not ignoring localhost", func() { - stderr := helper.Cmd("odo", "dev", "--random-ports", "--platform", "podman").ShouldFail().Err() + session, _, stderr, _, err := helper.StartDevMode(helper.DevSessionOpts{ + RunOnPodman: true, + }) + Expect(err).ToNot(HaveOccurred()) + defer func() { + session.Stop() + session.WaitEnd() + }() Expect(stderr).Should(ContainSubstring("Detected that the following port(s) can be reached only via the container loopback interface: admin (3001)")) }) From 330d3184cc1d13a0b5a00d66da286efd8a1d12b3 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Fri, 28 Apr 2023 15:14:21 +0200 Subject: [PATCH 5/7] Move logs back to level 4 --- pkg/watch/watch.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/watch/watch.go b/pkg/watch/watch.go index 7d22ab1b824..b8ff3aaa40a 100644 --- a/pkg/watch/watch.go +++ b/pkg/watch/watch.go @@ -260,7 +260,7 @@ func (o *WatchClient) eventWatcher( case ev := <-o.deploymentWatcher.ResultChan(): switch obj := ev.Object.(type) { case *appsv1.Deployment: - klog.V(0).Infof("deployment watcher Event: Type: %s, name: %s, rv: %s, generation: %d, pods: %d\n", + klog.V(4).Infof("deployment watcher Event: Type: %s, name: %s, rv: %s, generation: %d, pods: %d\n", ev.Type, obj.GetName(), obj.GetResourceVersion(), obj.GetGeneration(), obj.Status.ReadyReplicas) if obj.GetGeneration() > o.deploymentGeneration || obj.Status.ReadyReplicas != o.readyReplicas { o.deploymentGeneration = obj.GetGeneration() From 47082714103dfc7234d63f0d0518693f544e27a2 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Sat, 29 Apr 2023 18:06:35 +0200 Subject: [PATCH 6/7] Remove unnecessary files --- .../parent-devfile-commands-only.yaml | 20 ------------ .../parent-devfile-components-only.yaml | 15 --------- .../testdata/parent-devfile-empty.yaml | 3 -- pkg/libdevfile/testdata/parent-devfile.yaml | 32 ------------------- 4 files changed, 70 deletions(-) delete mode 100644 pkg/libdevfile/testdata/parent-devfile-commands-only.yaml delete mode 100644 pkg/libdevfile/testdata/parent-devfile-components-only.yaml delete mode 100644 pkg/libdevfile/testdata/parent-devfile-empty.yaml delete mode 100644 pkg/libdevfile/testdata/parent-devfile.yaml diff --git a/pkg/libdevfile/testdata/parent-devfile-commands-only.yaml b/pkg/libdevfile/testdata/parent-devfile-commands-only.yaml deleted file mode 100644 index 206682178dc..00000000000 --- a/pkg/libdevfile/testdata/parent-devfile-commands-only.yaml +++ /dev/null @@ -1,20 +0,0 @@ -commands: -- exec: - commandLine: GOCACHE=${PROJECT_SOURCE}/.cache go build main.go - component: runtime - group: - isDefault: true - kind: build - workingDir: ${PROJECT_SOURCE} - id: build -- exec: - commandLine: ./main - component: runtime - group: - isDefault: true - kind: run - workingDir: ${PROJECT_SOURCE} - id: run -schemaVersion: 2.1.0 -metadata: - name: parent diff --git a/pkg/libdevfile/testdata/parent-devfile-components-only.yaml b/pkg/libdevfile/testdata/parent-devfile-components-only.yaml deleted file mode 100644 index 74d64ff35bf..00000000000 --- a/pkg/libdevfile/testdata/parent-devfile-components-only.yaml +++ /dev/null @@ -1,15 +0,0 @@ -components: -- container: - endpoints: - - name: http - targetPort: 8080 - image: quay.io/devfile/golang:latest - memoryLimit: 1024Mi - mountSources: true - name: runtime -- kubernetes: - uri: "manifest.yaml" - name: kube-cmp -metadata: - name: my-go-app -schemaVersion: 2.1.0 diff --git a/pkg/libdevfile/testdata/parent-devfile-empty.yaml b/pkg/libdevfile/testdata/parent-devfile-empty.yaml deleted file mode 100644 index a3e59f1af10..00000000000 --- a/pkg/libdevfile/testdata/parent-devfile-empty.yaml +++ /dev/null @@ -1,3 +0,0 @@ -metadata: - name: my-go-app -schemaVersion: 2.1.0 diff --git a/pkg/libdevfile/testdata/parent-devfile.yaml b/pkg/libdevfile/testdata/parent-devfile.yaml deleted file mode 100644 index 08824612b9b..00000000000 --- a/pkg/libdevfile/testdata/parent-devfile.yaml +++ /dev/null @@ -1,32 +0,0 @@ -commands: -- exec: - commandLine: GOCACHE=${PROJECT_SOURCE}/.cache go build main.go - component: runtime - group: - isDefault: true - kind: build - workingDir: ${PROJECT_SOURCE} - id: build -- exec: - commandLine: ./main - component: runtime - group: - isDefault: true - kind: run - workingDir: ${PROJECT_SOURCE} - id: run -components: -- container: - endpoints: - - name: http - targetPort: 8080 - image: quay.io/devfile/golang:latest - memoryLimit: 1024Mi - mountSources: true - name: runtime -- kubernetes: - uri: "manifest.yaml" - name: kube-cmp -metadata: - name: my-go-app -schemaVersion: 2.1.0 From 50cee0219c7d6a57c07af759443c06f71896a87c Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Tue, 2 May 2023 09:11:01 +0200 Subject: [PATCH 7/7] Log state + Set state to ready when build fails --- pkg/dev/kubedev/components.go | 12 ++++++------ pkg/dev/kubedev/innerloop.go | 8 +++++--- pkg/dev/podmandev/reconcile.go | 4 ++-- pkg/watch/status.go | 16 ++++++++++++++-- pkg/watch/watch.go | 10 +++++----- pkg/watch/watch_test.go | 5 ++--- 6 files changed, 34 insertions(+), 21 deletions(-) diff --git a/pkg/dev/kubedev/components.go b/pkg/dev/kubedev/components.go index 38f1cafc963..acbcb30a619 100644 --- a/pkg/dev/kubedev/components.go +++ b/pkg/dev/kubedev/components.go @@ -61,12 +61,12 @@ func (o *DevClient) createComponents(ctx context.Context, parameters common.Push return false, err } - if componentStatus.State == watch.StateSyncOutdated { + if componentStatus.GetState() == watch.StateSyncOutdated { // Clear the cache of image components already applied, hence forcing image components to be reapplied. componentStatus.ImageComponentsAutoApplied = make(map[string]devfilev1.ImageComponent) } - klog.V(4).Infof("component state: %q\n", componentStatus.State) + klog.V(4).Infof("component state: %q\n", componentStatus.GetState()) err = o.buildPushAutoImageComponents(ctx, o.filesystem, parameters.Devfile, componentStatus) if err != nil { return false, err @@ -78,7 +78,7 @@ func (o *DevClient) createComponents(ctx context.Context, parameters common.Push return false, err } - if componentStatus.State != watch.StateWaitDeployment && componentStatus.State != watch.StateReady { + if componentStatus.GetState() != watch.StateWaitDeployment && componentStatus.GetState() != watch.StateReady { log.SpinnerNoSpin("Waiting for Kubernetes resources") } @@ -132,14 +132,14 @@ func (o *DevClient) createComponents(ctx context.Context, parameters common.Push if updated { klog.V(4).Infof("Deployment has been updated to generation %d. Waiting new event...\n", deployment.GetGeneration()) - componentStatus.State = watch.StateWaitDeployment + componentStatus.SetState(watch.StateWaitDeployment) return false, nil } numberReplicas := deployment.Status.ReadyReplicas if numberReplicas != 1 { klog.V(4).Infof("Deployment has %d ready replicas. Waiting new event...\n", numberReplicas) - componentStatus.State = watch.StateWaitDeployment + componentStatus.SetState(watch.StateWaitDeployment) return false, nil } @@ -160,7 +160,7 @@ func (o *DevClient) createComponents(ctx context.Context, parameters common.Push } o.portsChanged = !reflect.DeepEqual(o.portsToForward, o.portForwardClient.GetForwardedPorts()) - if componentStatus.State == watch.StateReady && !o.portsChanged { + if componentStatus.GetState() == watch.StateReady && !o.portsChanged { // If the deployment is already in Ready State, no need to continue return false, nil } diff --git a/pkg/dev/kubedev/innerloop.go b/pkg/dev/kubedev/innerloop.go index 3b49fc12ed0..edffda1c66f 100644 --- a/pkg/dev/kubedev/innerloop.go +++ b/pkg/dev/kubedev/innerloop.go @@ -51,7 +51,7 @@ func (o *DevClient) innerloop(ctx context.Context, parameters common.PushParamet return fmt.Errorf("failed to validate devfile build and run commands: %w", err) } - podChanged := componentStatus.State == watch.StateWaitDeployment + podChanged := componentStatus.GetState() == watch.StateWaitDeployment // Get a sync adapter. Check if project files have changed and sync accordingly compInfo := sync.ComponentInfo{ @@ -82,7 +82,7 @@ func (o *DevClient) innerloop(ctx context.Context, parameters common.PushParamet execRequired, err := o.syncClient.SyncFiles(ctx, syncParams) if err != nil { - componentStatus.State = watch.StateReady + componentStatus.SetState(watch.StateReady) return fmt.Errorf("failed to sync to component with name %s: %w", componentName, err) } s.End(true) @@ -150,11 +150,13 @@ func (o *DevClient) innerloop(ctx context.Context, parameters common.PushParamet if running { if cmd.Exec == nil || !util.SafeGetBool(cmd.Exec.HotReloadCapable) { if err = doExecuteBuildCommand(); err != nil { + componentStatus.SetState(watch.StateReady) return err } } } else { if err = doExecuteBuildCommand(); err != nil { + componentStatus.SetState(watch.StateReady) return err } } @@ -184,7 +186,7 @@ func (o *DevClient) innerloop(ctx context.Context, parameters common.PushParamet } componentStatus.EndpointsForwarded = o.portForwardClient.GetForwardedPorts() - componentStatus.State = watch.StateReady + componentStatus.SetState(watch.StateReady) return nil } diff --git a/pkg/dev/podmandev/reconcile.go b/pkg/dev/podmandev/reconcile.go index 07f71933714..2629876f82f 100644 --- a/pkg/dev/podmandev/reconcile.go +++ b/pkg/dev/podmandev/reconcile.go @@ -55,7 +55,7 @@ func (o *DevClient) reconcile( return err } o.deployedPod = pod - componentStatus.State = watch.StateReady + componentStatus.SetState(watch.StateReady) execRequired, err := o.syncFiles(ctx, options, pod, path) if err != nil { @@ -157,7 +157,7 @@ func (o *DevClient) reconcile( return err } - componentStatus.State = watch.StateReady + componentStatus.SetState(watch.StateReady) return nil } diff --git a/pkg/watch/status.go b/pkg/watch/status.go index 0c78d87df83..4e034d02316 100644 --- a/pkg/watch/status.go +++ b/pkg/watch/status.go @@ -1,6 +1,9 @@ package watch -import "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" +import ( + "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" + "k8s.io/klog" +) type State string @@ -16,7 +19,7 @@ const ( ) type ComponentStatus struct { - State State + state State PostStartEventsDone bool // RunExecuted is set to true when the run command has been executed // Used for HotReload capability @@ -27,6 +30,15 @@ type ComponentStatus struct { ImageComponentsAutoApplied map[string]v1alpha2.ImageComponent } +func (o *ComponentStatus) SetState(s State) { + klog.V(4).Infof("setting inner loop State %q", s) + o.state = s +} + +func (o *ComponentStatus) GetState() State { + return o.state +} + func componentCanSyncFile(state State) bool { return state == StateReady } diff --git a/pkg/watch/watch.go b/pkg/watch/watch.go index b8ff3aaa40a..ceaff43715c 100644 --- a/pkg/watch/watch.go +++ b/pkg/watch/watch.go @@ -221,8 +221,8 @@ func (o *WatchClient) eventWatcher( case <-sourcesTimer.C: // timer has fired - if !componentCanSyncFile(componentStatus.State) { - klog.V(4).Infof("State of component is %q, don't sync sources", componentStatus.State) + if !componentCanSyncFile(componentStatus.GetState()) { + klog.V(4).Infof("State of component is %q, don't sync sources", componentStatus.GetState()) continue } @@ -236,7 +236,7 @@ func (o *WatchClient) eventWatcher( } } - componentStatus.State = StateSyncOutdated + componentStatus.SetState(StateSyncOutdated) fmt.Fprintf(out, "Pushing files...\n\n") err := processEventsHandler(ctx, parameters, changedFiles, deletedPaths, &componentStatus) o.forceSync = false @@ -244,7 +244,7 @@ func (o *WatchClient) eventWatcher( return err } // empty the events to receive new events - if componentStatus.State == StateReady { + if componentStatus.GetState() == StateReady { events = []fsnotify.Event{} // empty the events slice to capture new events } @@ -443,7 +443,7 @@ func (o *WatchClient) processEvents( } return nil } - if oldStatus.State != StateReady && componentStatus.State == StateReady || + if oldStatus.GetState() != StateReady && componentStatus.GetState() == StateReady || !reflect.DeepEqual(oldStatus.EndpointsForwarded, componentStatus.EndpointsForwarded) { PrintInfoMessage(out, path, parameters.StartOptions.WatchFiles, parameters.PromptMessage) diff --git a/pkg/watch/watch_test.go b/pkg/watch/watch_test.go index 34ff8d08b08..9b513af7de9 100644 --- a/pkg/watch/watch_test.go +++ b/pkg/watch/watch_test.go @@ -136,9 +136,8 @@ func Test_eventWatcher(t *testing.T) { cancel() }() - componentStatus := ComponentStatus{ - State: StateReady, - } + componentStatus := ComponentStatus{} + componentStatus.SetState(StateReady) o := WatchClient{ sourcesWatcher: watcher,