Fix error handling when trying to delete remote resources not present in Devfile by rm3l · Pull Request #6659 · redhat-developer/odo

rm3l · 2023-03-14T11:09:08Z

What type of PR is this:

/kind bug

What does this PR do / why we need it:
Stumbled upon this issue recently, and found a mistake in the if condition used to skip non-deletable resources (like PodMetrics).

We had this previously, which would actually never match:

if !kerrors.IsNotFound(err) || !kerrors.IsMethodNotSupported(err) {

After adding some unit tests, I also noticed that the way any error is detected might lead to a race condition: multiple Goroutines are trying to assign a shared error variable, and we are using that to determine the return value.
This is solved by using errgroup instead, which looks more appropriate to simplify goroutine error handling.

Which issue(s) this PR fixes:
Fixes #6475

PR acceptance criteria:

Unit test
Integration test
Documentation

How to test changes / Special notes to the reviewer:

…t present in Devfile

… not present in Devfile Several Goroutines could potentially modify the same shared error variable, leading to weird behaviour, caught by the unit tests. Using errgroup allows simplifying goroutine error handling.

…s not present in Devfile Replace with a klog message, as the spinner does not add much value to the end user. Plus, it keeps displaying because we are regenerating the adapter which calls this function indefinitely.

netlify · 2023-03-14T11:09:14Z

✅ Deploy Preview for odo-docusaurus-preview canceled.

Name	Link
🔨 Latest commit	`afbd4d6`
🔍 Latest deploy log	https://app.netlify.com/sites/odo-docusaurus-preview/deploys/641055d64efc7000088264bd

sonarqubecloud · 2023-03-14T11:10:32Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

odo-robot · 2023-03-14T11:15:44Z

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

odo-robot · 2023-03-14T11:16:02Z

NoCluster Tests on commit finished successfully.
View logs: TXT HTML

odo-robot · 2023-03-14T11:17:25Z

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

odo-robot · 2023-03-14T11:27:04Z

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

odo-robot · 2023-03-14T11:32:37Z

Windows Tests (OCP) on commit finished with errors.
View logs: TXT HTML

odo-robot · 2023-03-14T11:32:53Z

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

odo-robot · 2023-03-14T11:38:34Z

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

odo-robot · 2023-03-14T11:43:39Z

Kubernetes Docs Tests on commit 8a002e1 finished with errors.
View logs: TXT HTML

valaparthvi · 2023-03-15T12:40:03Z

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

 	}

-	var err error
-	spinner := log.Spinnerf("Deleting resources not present in the Devfile: %s", strings.Join(resources, ", "))


I assume we're sacrificing the spinner message to be able to use errgroup and make sure we don't run into race conditions, yes?

We could have used errgroup with the spinner, as the spinner is started outside of the group and its status is updated after all goroutines have exited.
The problem is that odo will continuously try to delete such resources that it couldn't delete (and display the spinner over and over again), which adds too much clutter to the terminal, IMO.
Also, I found the final status of the spinner to be a bit misleading by displaying ✓, because it ignored PodMetrics resources (in the example below), but it didn't delete them actually as it will try again to delete them :-).

Sample output with the spinner

$ odo dev __ / \__ Developing using the "my-node-app" Devfile \__/ \ Namespace: default / \__/ odo version: v3.8.0 \__/ ↪ Running on the cluster in Dev mode • Waiting for Kubernetes resources ... ✓ Creating resource Pod/k8s-deploybydefault-true-and-not-referenced ✓ Creating resource Pod/k8s-deploybydefault-false-and-not-referenced ✓ Creating resource Pod/ocp-deploybydefault-true-and-not-referenced ✓ Creating resource Pod/k8s-deploybydefault-not-set-and-not-referenced ✓ Creating resource Pod/ocp-deploybydefault-false-and-not-referenced ✓ Creating resource Pod/ocp-deploybydefault-not-set-and-not-referenced ⚠ Pod is Pending ✓ Pod is Running ✓ Deleting resources not present in the Devfile: PodMetrics/k8s-deploybydefault-false-and-not-referenced, PodMetrics/k8s-deploybydefault-not-set-and-not-referenced, PodMetrics/k8s-deploybydefault-true-and-not-referenced, PodMetrics/ocp-deploybydefault-false-and-not-referenced, PodMetrics/ocp-deploybydefault-not-set-and-not-referenced, PodMetrics/ocp-deploybydefault-true-and-not-referenced [186ms] ✓ Syncing files into the container [101ms] ✓ Building your application in container (command: build) [3s] • Executing the application (command: start-app) ... - Forwarding from 127.0.0.1:20001 -> 3000 ↪ Dev mode Status: Watching for changes in the current directory /home/asoro/work/tmp/5694-implement-autobuild-and-deploybydefault Keyboard Commands: [Ctrl+c] - Exit and delete resources from the cluster [p] - Manually apply local changes to the application on the cluster ✓ Deleting resources not present in the Devfile: PodMetrics/k8s-deploybydefault-false-and-not-referenced, PodMetrics/k8s-deploybydefault-not-set-and-not-referenced, PodMetrics/k8s-deploybydefault-true-and-not-referenced, PodMetrics/ocp-deploybydefault-false-and-not-referenced, PodMetrics/ocp-deploybydefault-not-set-and-not-referenced, PodMetrics/ocp-deploybydefault-true-and-not-referenced [215ms] ✓ Deleting resources not present in the Devfile: PodMetrics/k8s-deploybydefault-false-and-not-referenced, PodMetrics/k8s-deploybydefault-not-set-and-not-referenced, PodMetrics/k8s-deploybydefault-true-and-not-referenced, PodMetrics/ocp-deploybydefault-false-and-not-referenced, PodMetrics/ocp-deploybydefault-not-set-and-not-referenced, PodMetrics/ocp-deploybydefault-true-and-not-referenced [839ms] ✓ Deleting resources not present in the Devfile: PodMetrics/k8s-deploybydefault-false-and-not-referenced, PodMetrics/k8s-deploybydefault-not-set-and-not-referenced, PodMetrics/k8s-deploybydefault-true-and-not-referenced, PodMetrics/ocp-deploybydefault-false-and-not-referenced, PodMetrics/ocp-deploybydefault-not-set-and-not-referenced, PodMetrics/ocp-deploybydefault-true-and-not-referenced [860ms] ✓ Deleting resources not present in the Devfile: PodMetrics/k8s-deploybydefault-false-and-not-referenced, PodMetrics/k8s-deploybydefault-not-set-and-not-referenced, PodMetrics/k8s-deploybydefault-true-and-not-r eferenced, PodMetrics/ocp-deploybydefault-false-and-not-referenced, PodMetrics/ocp-deploybydefault-not-set-and-not-referenced, PodMetrics/ocp-deploybydefault-true-and-not-referenced [853ms] ✓ Deleting resources not present in the Devfile: PodMetrics/k8s-deploybydefault-false-and-not-referenced, PodMetrics/k8s-deploybydefault-not-set-and-not-referenced, PodMetrics/k8s-deploybydefault-true-and-not-r eferenced, PodMetrics/ocp-deploybydefault-false-and-not-referenced, PodMetrics/ocp-deploybydefault-not-set-and-not-referenced, PodMetrics/ocp-deploybydefault-true-and-not-referenced [849ms] ✓ Deleting resources not present in the Devfile: PodMetrics/k8s-deploybydefault-false-and-not-referenced, PodMetrics/k8s-deploybydefault-not-set-and-not-referenced, PodMetrics/k8s-deploybydefault-true-and-not-r eferenced, PodMetrics/ocp-deploybydefault-false-and-not-referenced, PodMetrics/ocp-deploybydefault-not-set-and-not-referenced, PodMetrics/ocp-deploybydefault-true-and-not-referenced [855ms] ...

This is why I thought it would make sense to klog this instead.

rm3l · 2023-03-15T13:31:06Z

  user-guides/quickstart/docs-mdx/java/java_odo_dev_output.mdx
  Expected
      <string>:   (
        	"""
        	... // 10 identical lines
        	âœ“  Added storage m2 to component
        	âš   Pod is Pending
      - 	âœ“  Pod is Running
        	âœ“  Syncing files into the container [1s]
        	âœ“  Building your application in container (command: build) [1s]
        	... // 9 identical lines
        	[Ctrl+c] - Exit and delete resources from the cluster
        	[p] - Manually apply local changes to the application on the cluster
      + 	âœ“  Pod is Running
        	```
        	"""
        )
      
  to be empty
  In [It] at: /go/odo_1/tests/documentation/user-guides/doc_user_guides_quickstart_test.go:256

To be addressed by #6545

/override Kubernetes-Integration-Tests/Kubernetes-Docs-Integration-Tests

openshift-ci · 2023-03-15T13:31:10Z

@rm3l: Overrode contexts on behalf of rm3l: Kubernetes-Integration-Tests/Kubernetes-Docs-Integration-Tests

Details

In response to this:

 user-guides/quickstart/docs-mdx/java/java_odo_dev_output.mdx
 Expected
     <string>:   (
       	"""
       	... // 10 identical lines
       	âœ“  Added storage m2 to component
       	âš   Pod is Pending
     - 	âœ“  Pod is Running
       	âœ“  Syncing files into the container [1s]
       	âœ“  Building your application in container (command: build) [1s]
       	... // 9 identical lines
       	[Ctrl+c] - Exit and delete resources from the cluster
       	[p] - Manually apply local changes to the application on the cluster
     + 	âœ“  Pod is Running
       	```
       	"""
       )
     
 to be empty
 In [It] at: /go/odo_1/tests/documentation/user-guides/doc_user_guides_quickstart_test.go:256

To be addressed by #6545

/override Kubernetes-Integration-Tests/Kubernetes-Docs-Integration-Tests

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.

rm3l · 2023-03-16T14:41:16Z

Flaky tests.

/override windows-integration-test/Windows-test

openshift-ci · 2023-03-16T14:41:20Z

@rm3l: Overrode contexts on behalf of rm3l: windows-integration-test/Windows-test

Details

In response to this:

Flaky tests.

/override windows-integration-test/Windows-test

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.

rm3l added 4 commits March 14, 2023 10:35

Fix condition for checking errors while trying to delete resources no…

fdb6b34

…t present in Devfile

Add unit tests

cc8479f

openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Mar 14, 2023

openshift-ci bot requested review from feloy and kadel March 14, 2023 11:09

rm3l requested review from valaparthvi and removed request for kadel March 14, 2023 11:09

valaparthvi approved these changes Mar 15, 2023

View reviewed changes

openshift-ci bot assigned valaparthvi Mar 15, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Mar 15, 2023

openshift-merge-robot merged commit bffb9b0 into redhat-developer:main Mar 16, 2023

rm3l deleted the 6475-podmetrics-is-not-a-deletable-resource-and-needs-to-handled-properly-by-odo-dev branch March 16, 2023 14:46

Conversation

rm3l commented Mar 14, 2023

Uh oh!

netlify bot commented Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for odo-docusaurus-preview canceled.

Uh oh!

sonarqubecloud bot commented Mar 14, 2023

Uh oh!

odo-robot bot commented Mar 14, 2023

Uh oh!

odo-robot bot commented Mar 14, 2023

Uh oh!

odo-robot bot commented Mar 14, 2023

Uh oh!

odo-robot bot commented Mar 14, 2023

Uh oh!

odo-robot bot commented Mar 14, 2023

Uh oh!

odo-robot bot commented Mar 14, 2023

Uh oh!

odo-robot bot commented Mar 14, 2023

Uh oh!

odo-robot bot commented Mar 14, 2023

Uh oh!

valaparthvi Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

rm3l Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

rm3l commented Mar 15, 2023

Uh oh!

openshift-ci bot commented Mar 15, 2023

Uh oh!

rm3l commented Mar 16, 2023

Uh oh!

openshift-ci bot commented Mar 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

netlify bot commented Mar 14, 2023 •

edited

Loading