odo dev: react to changes as soon as possible #5933

feloy · 2022-07-08T09:50:39Z

What type of PR is this:

/kind feature

What does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes partially #5867

PR acceptance criteria:

Unit test
Integration test
Documentation

How to test changes / Special notes to the reviewer:

Acceptance Criteria

It should handle Devfile changes when a Pod is in an Error state
It should not wait for a specific state in the reconcile loop
It should watch for all interesting resources (Deployment, ServiceBindings, ...) permanently, i.e. as long as odo dev is running
It should notify users of the current application state
If odo dev is running with the no-watch flag, it should not watch for files but still continue to watch cluster resources

netlify · 2022-07-08T09:50:43Z

✅ Deploy Preview for odo-docusaurus-preview canceled.

Name	Link
🔨 Latest commit	`053cdd1`
🔍 Latest deploy log	https://app.netlify.com/sites/odo-docusaurus-preview/deploys/62f0b277a7c4e200080a8fd1

odo-robot · 2022-07-08T09:55:45Z

Validate Tests on commit a0efdb1 finished successfully.
View logs: TXT HTML

odo-robot · 2022-07-08T09:58:20Z

Unit Tests on commit a0efdb1 finished successfully.
View logs: TXT HTML

odo-robot · 2022-07-08T10:04:26Z

Windows Tests (OCP) on commit a0efdb1 finished successfully.
View logs: TXT HTML

odo-robot · 2022-07-08T10:06:44Z

Kubernetes Tests on commit a0efdb1 finished successfully.
View logs: TXT HTML

odo-robot · 2022-07-08T10:07:35Z

OpenShift Tests on commit a0efdb1 finished successfully.
View logs: TXT HTML

pkg/watch/watch.go

dharmit

Was surely challending to review commit-by-commit. Not sure if I would want to do it regularly. 😕

pkg/devfile/adapters/kubernetes/component/push.go

pkg/devfile/adapters/kubernetes/component/adapter.go

pkg/devfile/adapters/kubernetes/component/push.go

pkg/watch/watch.go

pkg/service/link.go

dharmit · 2022-07-13T10:18:01Z

pkg/devfile/adapters/kubernetes/storage/utils.go

+	for k := range volumeNameToVolInfo {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)


Should we put this in a function since we are doing it twice in the same file?

I would even go as far and suggest we combine the two functions GetEphemeralVolumesAndVolumeMounts, and GetPersistentVolumesAndVolumeMounts into one, since the only thing that sets them apart is their use of getEmptyDir, and getPVC respectively.

The parameters volumeNameToVolInfo/ephemerals do not have the same type, it makes it difficult to replace with a unique function

pkg/portForward/interface.go

pkg/portForward/portForward.go

pkg/dev/dev.go

feloy · 2022-08-08T06:42:13Z

It should handle Devfile changes when a Pod is in an Error state

How do you suggest I test this scenario? I modified the container component to use an invalid container image, which caused the Pod to go into ErrImagePull state, and then corrected it. It worked fine.

Anything else?

You can set a high memory request (1Ti for example), so the scheduler won't be able to find it a node

It should not wait for a specific state in the reconcile loop

How do I test this functionality? What am I supposed to look for?

This is pore an implementation detail, it is not visible at the user level, but you can check in the sources we are not waiting anymore for anything.

It should watch for all interesting resources (Deployment, ServiceBindings, ...) permanently, i.e. as long as odo dev is running

How do I test this? Should I do kubectl edit deployment and see? Same for ServiceBindings? What else does "interesting resources" encompass?

It should notify users of the current application state

As a user, what should I expect here?

You should not ask yourself what is teh state of the app at any moment, and you should not have to use kubectl or other tool to beb able to understand what's the state of the component

sonarqubecloud · 2022-08-08T06:52:46Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
11 Code Smells

No Coverage information
0.0% Duplication

dharmit · 2022-08-08T07:00:41Z

This is pore an implementation detail, it is not visible at the user level, but you can check in the sources we are not waiting anymore for anything.
You should not ask yourself what is teh state of the app at any moment, and you should not have to use kubectl or other tool to beb able to understand what's the state of the component

The thing is that these items are in the acceptance criteria. If, as a reviewer, I can't test it, how can I approve it?

feloy · 2022-08-08T07:25:56Z

This is pore an implementation detail, it is not visible at the user level, but you can check in the sources we are not waiting anymore for anything.
You should not ask yourself what is teh state of the app at any moment, and you should not have to use kubectl or other tool to beb able to understand what's the state of the component

The thing is that these items are in the acceptance criteria. If, as a reviewer, I can't test it, how can I approve it?

I'm not sure acceptance criteria are for user level only.

For the first point, you can check in the sources how it is implemented, and check that odo is not waiting anymore, like it was waiting before for a Pod to be started.

For the second point, this is quite subjective, but you can make lots of manipulations, and check that you understand at any moment what is the state of the component. If, at any moment, you don't know if the component is running or not, the acceptance creteria is not respected

valaparthvi · 2022-08-08T10:16:00Z

/lgtm

@dharmit agrees.

valaparthvi · 2022-08-08T10:16:15Z

/override ci/prow/v4.10-integration-e2e

openshift-ci · 2022-08-08T10:16:23Z

@valaparthvi: Overrode contexts on behalf of valaparthvi: ci/prow/v4.10-integration-e2e

In response to this:

/override ci/prow/v4.10-integration-e2e

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

feloy · 2022-08-08T10:17:02Z

/override ci/prow/v4.10-integration-e2e

Tests pass on IBM Cloud

unable to download starter project "nodejs-starter": unexpected client error: unexpected requesting "https://github.com/odo-devfiles/nodejs-ex.git/info/refs?service=git-upload-pack" status code: 429

openshift-ci · 2022-08-08T10:17:06Z

@feloy: Overrode contexts on behalf of feloy: ci/prow/v4.10-integration-e2e

In response to this:

/override ci/prow/v4.10-integration-e2e

Tests pass on IBM Cloud

unable to download starter project "nodejs-starter": unexpected client error: unexpected requesting "https://github.com/odo-devfiles/nodejs-ex.git/info/refs?service=git-upload-pack" status code: 429

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

* Create auxialiary functions to find current deployment and pod name * Create auxialiary function for pushing Kubernetes components to cluster * Auxiliary function getPushDevfileCommands and remocve RunModeChanged flag as it is not possible anymore with long running dev command + move prelimiray tests at the beginning * Create auxiliary function updatePVCsOwnerReferences + move isMainStorageEphemeral inside aux. function * Watch for deployments in addition to watching for files during odo dev * - Check the Deployment Generation to return earlier when generation changed - Return earlier when pod is not ready * Push receives a Status that it can update, to indicate the status of the component when the function returns. This status is passed from Start to Watch * Use status to decide to sync files * Simplify getting pod * Status for PostStartEvents * - Touch .gitignore earlier so it is not synced - Add devstate.jso file to .gitignore * Fix the integration tests. * Make --no-watch work again * Get smaller volumes for tests * Quit when fail to exec port forwarding * Exponential delay after an error * Fix: get running pod * Remove testing odo dev stops when the build command fails. odo dev now gives a change to the user to fix the problem * Fix order or volumes and volume mounts in pod spec * Use server side apply for updating PVC when supported * Watch Devfile separately * Adapt tests for devfile handled separately * Update forwarded ports when necessary * Display kubernetes resources created only when created/updated * Do not set ResourceVersion when patching * TEMP: Add more logs on failing integrattests * Tests: select the *running* pod * Use forward slashes for .gitignore content, even on Windows * Rename GetOnePodFromSelector to GetRunningPodFromSelector * Cleanup logs * Fix 'odo log' integration test * Add log "Waiting for Kubernetes resources" * Fail if service bindings are not injected * Rename sources related watcher, timer * Fix delay after error * Display info about Pod * Disambiguate error messages * Display events related to components pod * Remove now unused functions used to wait for Deployment / Pod * Typos + function renaming * Do not watch devfile with --no-watch * Use type switch for Kubernertes watch event * Do not fail when watching Events is Forbidden * Do not fail when servicebinding resources are forbidden * Exit when build command fails on no-watch mode * Sync devfile.yaml by default, workaround when commands change on devfile * Rename to warningsWatcher * Fix comment

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation labels Jul 8, 2022

openshift-ci bot requested review from anandrkskd and dharmit July 8, 2022 09:51

feloy force-pushed the feature-5867/dev-watch-deployment-v2 branch from dd5940d to 30c723f Compare July 8, 2022 09:59

feloy force-pushed the feature-5867/dev-watch-deployment-v2 branch 5 times, most recently from 4cfb015 to 1bc1cc7 Compare July 8, 2022 18:09

feloy commented Jul 9, 2022

View reviewed changes

pkg/watch/watch.go Outdated Show resolved Hide resolved

feloy force-pushed the feature-5867/dev-watch-deployment-v2 branch 6 times, most recently from 1a4111c to 4639f47 Compare July 12, 2022 08:56

feloy changed the title ~~[wip] odo dev: react to changes as soon as possible~~ odo dev: react to changes as soon as possible Jul 12, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jul 12, 2022

feloy force-pushed the feature-5867/dev-watch-deployment-v2 branch from 4639f47 to 577fa4f Compare July 13, 2022 05:42

dharmit suggested changes Jul 13, 2022

View reviewed changes

openshift-ci bot assigned dharmit Jul 13, 2022

feloy force-pushed the feature-5867/dev-watch-deployment-v2 branch 2 times, most recently from 5102b4e to 73988b7 Compare July 13, 2022 16:13

feloy added 8 commits August 8, 2022 08:17

Remove now unused functions used to wait for Deployment / Pod

9d2c19d

Typos + function renaming

6a90ad0

Do not watch devfile with --no-watch

6fe113b

Use type switch for Kubernertes watch event

364a6b4

Do not fail when watching Events is Forbidden

a010172

Do not fail when servicebinding resources are forbidden

1df9856

Exit when build command fails on no-watch mode

263a815

Sync devfile.yaml by default, workaround when commands change on devfile

2d2d7e9

feloy force-pushed the feature-5867/dev-watch-deployment-v2 branch from de7defb to 2d2d7e9 Compare August 8, 2022 06:19

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Aug 8, 2022

feloy added 2 commits August 8, 2022 08:29

Rename to warningsWatcher

d25fcd2

Fix comment

053cdd1

feloy requested review from dharmit and valaparthvi August 8, 2022 06:43

openshift-ci bot assigned valaparthvi Aug 8, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Aug 8, 2022

openshift-merge-robot merged commit f22c3f8 into redhat-developer:main Aug 8, 2022

rm3l linked an issue Aug 8, 2022 that may be closed by this pull request

odo dev: react to changes as soon as possible #5867

Closed

6 tasks

dharmit mentioned this pull request Aug 9, 2022

Restart port forwarding on failure #6013

Merged

3 tasks

dharmit mentioned this pull request Aug 17, 2022

odo dev: Cleanup after displaying the error #6024

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

odo dev: react to changes as soon as possible #5933

odo dev: react to changes as soon as possible #5933

feloy commented Jul 8, 2022 •

edited

Loading

netlify bot commented Jul 8, 2022 •

edited

Loading

odo-robot bot commented Jul 8, 2022 •

edited

Loading

odo-robot bot commented Jul 8, 2022 •

edited

Loading

odo-robot bot commented Jul 8, 2022 •

edited

Loading

odo-robot bot commented Jul 8, 2022 •

edited

Loading

odo-robot bot commented Jul 8, 2022 •

edited

Loading

dharmit left a comment

dharmit Jul 13, 2022

valaparthvi Jul 21, 2022

feloy Aug 1, 2022

feloy commented Aug 8, 2022

sonarqubecloud bot commented Aug 8, 2022

dharmit commented Aug 8, 2022

feloy commented Aug 8, 2022 •

edited

Loading

valaparthvi commented Aug 8, 2022

valaparthvi commented Aug 8, 2022

openshift-ci bot commented Aug 8, 2022

feloy commented Aug 8, 2022

openshift-ci bot commented Aug 8, 2022

odo dev: react to changes as soon as possible #5933

odo dev: react to changes as soon as possible #5933

Conversation

feloy commented Jul 8, 2022 • edited Loading

Acceptance Criteria

netlify bot commented Jul 8, 2022 • edited Loading

✅ Deploy Preview for odo-docusaurus-preview canceled.

odo-robot bot commented Jul 8, 2022 • edited Loading

odo-robot bot commented Jul 8, 2022 • edited Loading

odo-robot bot commented Jul 8, 2022 • edited Loading

odo-robot bot commented Jul 8, 2022 • edited Loading

odo-robot bot commented Jul 8, 2022 • edited Loading

dharmit left a comment

Choose a reason for hiding this comment

dharmit Jul 13, 2022

Choose a reason for hiding this comment

valaparthvi Jul 21, 2022

Choose a reason for hiding this comment

feloy Aug 1, 2022

Choose a reason for hiding this comment

feloy commented Aug 8, 2022

sonarqubecloud bot commented Aug 8, 2022

dharmit commented Aug 8, 2022

feloy commented Aug 8, 2022 • edited Loading

valaparthvi commented Aug 8, 2022

valaparthvi commented Aug 8, 2022

openshift-ci bot commented Aug 8, 2022

feloy commented Aug 8, 2022

openshift-ci bot commented Aug 8, 2022

feloy commented Jul 8, 2022 •

edited

Loading

netlify bot commented Jul 8, 2022 •

edited

Loading

odo-robot bot commented Jul 8, 2022 •

edited

Loading

odo-robot bot commented Jul 8, 2022 •

edited

Loading

odo-robot bot commented Jul 8, 2022 •

edited

Loading

odo-robot bot commented Jul 8, 2022 •

edited

Loading

odo-robot bot commented Jul 8, 2022 •

edited

Loading

feloy commented Aug 8, 2022 •

edited

Loading