Skip to content

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

Merged
openshift-merge-robot merged 3 commits intoredhat-developer:mainfrom
rm3l:6681-parallel-commands-in-composite-command-seem-to-be-run-sequentially
Sep 6, 2023
Merged

Make sure to run parallel commands part of a composite command in parallel#7075
openshift-merge-robot merged 3 commits intoredhat-developer:mainfrom
rm3l:6681-parallel-commands-in-composite-command-seem-to-be-run-sequentially

Conversation

@rm3l
Copy link
Member

@rm3l rm3l commented Sep 6, 2023

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]
[...]

@netlify
Copy link

netlify bot commented Sep 6, 2023 •
edited
Loading

✅ 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

@rm3l 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 rm3l requested a review from feloy September 6, 2023 09:16
@odo-robot
Copy link

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

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

@odo-robot
Copy link

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

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

@odo-robot
Copy link

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

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

@odo-robot
Copy link

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

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

@odo-robot
Copy link

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

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

@odo-robot
Copy link

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

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

@odo-robot
Copy link

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

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

@feloy
Copy link
Contributor

feloy commented Sep 6, 2023

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
------------------------------

@feloy feloy closed this Sep 6, 2023
@feloy feloy reopened this Sep 6, 2023
@rm3l
Copy link
Member Author

rm3l commented Sep 6, 2023

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..

rm3l added 2 commits September 6, 2023 18:06
@rm3l
Display warnings in case there are errors when executing pre-stop events
858860f
@rm3l
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
@rm3l
Copy link
Member Author

rm3l commented Sep 6, 2023

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
Copy link

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

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

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 6, 2023
@feloy feloy closed this Sep 6, 2023
@feloy feloy reopened this Sep 6, 2023
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@openshift-merge-robot openshift-merge-robot merged commit 00d3988 into redhat-developer:main Sep 6, 2023
@rm3l rm3l deleted the 6681-parallel-commands-in-composite-command-seem-to-be-run-sequentially branch September 7, 2023 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@feloy feloy feloy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@feloy feloy

Labels

area/devfile-spec Issues or PRs related to the Devfile specification and how odo handles and interprets it. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. Required by Prow.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Parallel commands in composite command seem to be run sequentially

3 participants

@rm3l @feloy @openshift-merge-robot