Make sure to run parallel commands part of a composite command in parallel #7075

rm3l · 2023-09-06T09:13:51Z

What type of PR is this:

/kind bug
/area devfile-spec

What does this PR do / why we need it:

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

PR acceptance criteria:

Unit test
Integration test
Documentation

How to test changes / Special notes to the reviewer:
See the repro steps in #6681. With the changes in this PR, the output of odo dev should reflect that the commands are running in parallel:

$ odo init \
  --name my-component-with-parallel-commands \
  --devfile-path https://gist.githubusercontent.com/rm3l/5691e926a4cfe37e369d93695187bdd6/raw/95abc768e83bd83e7acda035a6053f57b20f4939/devfile-with-parallel-commands.yaml

$ odo dev

[...]
 •  Waiting for Kubernetes resources  ...
 ⚠  Pod is Pending
 ✓  Pod is Running
 ✓  Syncing files into the container [57ms]
 ✓  Building your application in container (command: exec-3) [40ms]
 ✓  Building your application in container (command: exec-2) [41ms]
 ✓  Building your application in container (command: exec-1) [5s]
 •  Executing the application (command: run-1)  ...
 •  Executing the application (command: run-3)  ...
 •  Executing the application (command: run-2)  ...
 ✓  Finished executing the application (command: run-3) [145ms]
 ✓  Finished executing the application (command: run-2) [145ms]
 ✓  Finished executing the application (command: run-1) [5s]

p

Pushing files...

 •  Waiting for Kubernetes resources  ...
 ✓  Syncing files into the container 
 ✓  Building your application in container (command: exec-2) [30ms]
 ✓  Building your application in container (command: exec-3) [30ms]
 ✓  Building your application in container (command: exec-1) [5s]
 •  Executing the application (command: run-2)  ...
 •  Executing the application (command: run-3)  ...
 •  Executing the application (command: run-1)  ...
 ✓  Finished executing the application (command: run-3) [2s]
 ✓  Finished executing the application (command: run-2) [2s]
 ✓  Finished executing the application (command: run-1) [7s]
[...]

…allel

netlify · 2023-09-06T09:13:56Z

✅ Deploy Preview for odo-docusaurus-preview canceled.

Name	Link
🔨 Latest commit	`39a6b54`
🔍 Latest deploy log	https://app.netlify.com/sites/odo-docusaurus-preview/deploys/64f8a601cfe6a50008298116

odo-robot · 2023-09-06T09:20:25Z

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

odo-robot · 2023-09-06T09:21:19Z

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

odo-robot · 2023-09-06T09:21:35Z

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

odo-robot · 2023-09-06T09:24:29Z

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

odo-robot · 2023-09-06T09:30:06Z

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

odo-robot · 2023-09-06T09:41:59Z

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

odo-robot · 2023-09-06T09:44:15Z

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

feloy · 2023-09-06T14:23:16Z

I can see this error happening in the different platforms, buu I cannot see the relation with the changes made in this PR:

[FAILED] Expected
      <string>: Searching resources to delete, please wait...
      This will delete "nodejs" from the namespace "cmd-delete-test517hno".
       â€¢  The following resources will get deleted from cluster:
       â€¢  	- Deployment: nodejs-app
      
      This will also delete the following files and directories:
      	- /tmp/1997028204/.odo
      	- /tmp/1997028204/devfile.yaml
       â€¢  Deleting resources from cluster  ...
      ↵
 âœ“  Deleting resources from cluster [108ms]
      The component "nodejs" is successfully deleted from namespace "cmd-delete-test517hno"
      
      
  to contain substring
      <string>: Executing pre-stop command in container (command: myprestop)
  In [It] at: /go/odo_1/tests/integration/cmd_delete_test.go:520 @ 09/06/23 09:22:03.405
------------------------------

rm3l · 2023-09-06T14:33:48Z

I can see this error happening in the different platforms, buu I cannot see the relation with the changes made in this PR:

[FAILED] Expected
      <string>: Searching resources to delete, please wait...
      This will delete "nodejs" from the namespace "cmd-delete-test517hno".
       â€¢  The following resources will get deleted from cluster:
       â€¢  	- Deployment: nodejs-app
      
      This will also delete the following files and directories:
      	- /tmp/1997028204/.odo
      	- /tmp/1997028204/devfile.yaml
       â€¢  Deleting resources from cluster  ...
      ↵
 âœ“  Deleting resources from cluster [108ms]
      The component "nodejs" is successfully deleted from namespace "cmd-delete-test517hno"
      
      
  to contain substring
      <string>: Executing pre-stop command in container (command: myprestop)
  In [It] at: /go/odo_1/tests/integration/cmd_delete_test.go:520 @ 09/06/23 09:22:03.405
------------------------------

I was also able to reproduce the same failure locally; not sure either how it is related to the changes here.
I'm taking a look..

…case of the sub-command names Since this passed the Devfile validation logic, we should use the same logic as in command_composite.go

rm3l · 2023-09-06T16:16:09Z

I can see this error happening in the different platforms, buu I cannot see the relation with the changes made in this PR:

[FAILED] Expected
      <string>: Searching resources to delete, please wait...
      This will delete "nodejs" from the namespace "cmd-delete-test517hno".
       â€¢  The following resources will get deleted from cluster:
       â€¢  	- Deployment: nodejs-app
      
      This will also delete the following files and directories:
      	- /tmp/1997028204/.odo
      	- /tmp/1997028204/devfile.yaml
       â€¢  Deleting resources from cluster  ...
      ↵
 âœ“  Deleting resources from cluster [108ms]
      The component "nodejs" is successfully deleted from namespace "cmd-delete-test517hno"
      
      
  to contain substring
      <string>: Executing pre-stop command in container (command: myprestop)
  In [It] at: /go/odo_1/tests/integration/cmd_delete_test.go:520 @ 09/06/23 09:22:03.405
------------------------------

I was also able to reproduce the same failure locally; not sure either how it is related to the changes here. I'm taking a look..

Okay, I think this is a bit related, because the failing test is using a devfile with a pre-stop event trying to execute a composite command with parallel sub-commands. And with the changes here, we should now be using the actual composite parallel implementation.

From the job logs, I noticed the following message:

Failed to execute "preStop" event commands for component "nodejs", cause: unable to execute devfile command "mycompcmd": unknown command type

After digging, the sub-commands in the Devfile have an upper case, which seems to be a valid Devfile (even if all command IDs are expected to be lower-case).

odo/tests/examples/source/devfiles/nodejs/devfile-with-valid-events.yaml

Lines 70 to 76 in adc9699

    
           - id: mycompcmd 
        
             composite: 
        
               label: Build and Mkdir 
        
               commands: 
        
                 - secondpreStop 
        
                 - thirdpreStop 
        
               parallel: true

The composite implementation lowers the case of the sub-commands, while the composite parallel implementation does not.
We need to use the same logic in both implementations. I'll update the implementation in command_composite_parallel.go accordingly.

odo-robot · 2023-09-06T16:57:37Z

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

sonarqubecloud · 2023-09-06T18:28:36Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

Make sure to run parallel commands part of a composite command in par…

77462c7

…allel

rm3l added kind/bug Categorizes issue or PR as related to a bug. area/devfile-spec Issues or PRs related to the Devfile specification and how odo handles and interprets it. labels Sep 6, 2023

rm3l requested a review from feloy September 6, 2023 09:16

feloy closed this Sep 6, 2023

feloy reopened this Sep 6, 2023

rm3l added 2 commits September 6, 2023 18:06

Display warnings in case there are errors when executing pre-stop events

858860f

Fix the command_composite_parallel.go implementation by lowering the …

39a6b54

…case of the sub-command names Since this passed the Devfile validation logic, we should use the same logic as in command_composite.go

feloy approved these changes Sep 6, 2023

View reviewed changes

openshift-ci bot assigned feloy Sep 6, 2023

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

feloy closed this Sep 6, 2023

feloy reopened this Sep 6, 2023

openshift-merge-robot merged commit 00d3988 into redhat-developer:main Sep 6, 2023

rm3l deleted the 6681-parallel-commands-in-composite-command-seem-to-be-run-sequentially branch September 7, 2023 07:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure to run parallel commands part of a composite command in parallel #7075

Make sure to run parallel commands part of a composite command in parallel #7075

rm3l commented Sep 6, 2023

netlify bot commented Sep 6, 2023 •

edited

Loading

odo-robot bot commented Sep 6, 2023 •

edited

Loading

odo-robot bot commented Sep 6, 2023 •

edited

Loading

odo-robot bot commented Sep 6, 2023 •

edited

Loading

odo-robot bot commented Sep 6, 2023 •

edited

Loading

odo-robot bot commented Sep 6, 2023 •

edited

Loading

odo-robot bot commented Sep 6, 2023 •

edited

Loading

odo-robot bot commented Sep 6, 2023 •

edited

Loading

feloy commented Sep 6, 2023

rm3l commented Sep 6, 2023

rm3l commented Sep 6, 2023

odo-robot bot commented Sep 6, 2023 •

edited

Loading

sonarqubecloud bot commented Sep 6, 2023

Make sure to run parallel commands part of a composite command in parallel #7075

Make sure to run parallel commands part of a composite command in parallel #7075

Conversation

rm3l commented Sep 6, 2023

netlify bot commented Sep 6, 2023 • edited Loading

✅ Deploy Preview for odo-docusaurus-preview canceled.

odo-robot bot commented Sep 6, 2023 • edited Loading

odo-robot bot commented Sep 6, 2023 • edited Loading

odo-robot bot commented Sep 6, 2023 • edited Loading

odo-robot bot commented Sep 6, 2023 • edited Loading

odo-robot bot commented Sep 6, 2023 • edited Loading

odo-robot bot commented Sep 6, 2023 • edited Loading

odo-robot bot commented Sep 6, 2023 • edited Loading

feloy commented Sep 6, 2023

rm3l commented Sep 6, 2023

rm3l commented Sep 6, 2023

odo-robot bot commented Sep 6, 2023 • edited Loading

sonarqubecloud bot commented Sep 6, 2023

netlify bot commented Sep 6, 2023 •

edited

Loading

odo-robot bot commented Sep 6, 2023 •

edited

Loading

odo-robot bot commented Sep 6, 2023 •

edited

Loading

odo-robot bot commented Sep 6, 2023 •

edited

Loading

odo-robot bot commented Sep 6, 2023 •

edited

Loading

odo-robot bot commented Sep 6, 2023 •

edited

Loading

odo-robot bot commented Sep 6, 2023 •

edited

Loading

odo-robot bot commented Sep 6, 2023 •

edited

Loading

odo-robot bot commented Sep 6, 2023 •

edited

Loading