Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to parrallel execution of SDK commands #742

Merged
merged 1 commit into from
Apr 30, 2019

Conversation

aLekSer
Copy link
Collaborator

@aLekSer aLekSer commented Apr 24, 2019

Use subshell execution of the run-all-sdk-command make target.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 7ad13ad2-04bc-4cd2-ab8c-ea3826632791

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/742/head:pr_742 && git checkout pr_742
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.10.0-b74206b

@aLekSer aLekSer force-pushed the run-sdk-in-parallel branch 3 times, most recently from 2232de4 to f5c26d1 Compare April 24, 2019 11:01
@aLekSer aLekSer marked this pull request as ready for review April 24, 2019 11:04
@aLekSer
Copy link
Collaborator Author

aLekSer commented Apr 24, 2019

I have added wait right after the loop, if one process fail we would get the error and a message.
Tested with make test-sdks it spawns two docker containers for go and node.

And tested errors scenario for instance if we add exit 1 to test.sh script. That would result in:

17 specs, 0 failures
Finished in 0.087 seconds
Failed to run SDK command
make: *** [run-all-sdk-command] Error 255

Note that we only have one build.sh script (currently for cpp only). So running make build-sdks would run only one container.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 7e353e99-3d43-45a1-bccf-099a02d6ae78

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/742/head:pr_742 && git checkout pr_742
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.10.0-6711531

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 251a76ce-c16e-41a2-b6f0-d2e33abac867

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/742/head:pr_742 && git checkout pr_742
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.10.0-2232de4

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 41249e09-c5c0-498a-988c-e0461fdbf362

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/742/head:pr_742 && git checkout pr_742
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.10.0-f5c26d1

@@ -55,17 +55,22 @@ gen-all-sdk-grpc: run-all-sdk-command
gen-sdk-grpc: COMMAND := gen
gen-sdk-grpc: run-sdk-command

# Runs a command on all supported languages, use COMMAND variable to select which command.
# Runs a command on all supported languages in parallel, use COMMAND variable to select which command.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about using native parallelism built into make:

run-all-sdk-command: run-sdk-command-go run-sdk-command-rust run-sdk-command-cpp

run-sdk-command-cpp:
  $(MAKE) run-sdk-command SDK_FOLDER=cpp

run-sdk-command-rust:
  $(MAKE) run-sdk-command SDK_FOLDER=rust

run-sdk-command-go:
  $(MAKE) run-sdk-command SDK_FOLDER=go

Then the user can do make -j4 run-all-sdk-command COMMAND=gen

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooh that's a really interesting idea. I wonder if we could also come up with a pattern rule so we don't have to define each rule individually?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That native way looks more natural and simple, updating this Pull Request, removing initial and next waiting for loop:

	for job in `jobs -p` ; \
	do \
		wait $$job || let "FAIL+=1"; \
	done ; \

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkowalski
The only thing that we don't have build.sh in each directory, that's why we have such check that file exists:
if [ "$${d%?}" != "tool" ] && [ -f $$d/$(COMMAND).sh ];

@markmandel
Copy link
Member

/cc @Kuqd any thoughts?

@aLekSer aLekSer force-pushed the run-sdk-in-parallel branch 2 times, most recently from 68b65f8 to 9cab1b0 Compare April 25, 2019 13:02
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: d1ff4a7e-44c2-451d-a5b9-50004a0f2508

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/742/head:pr_742 && git checkout pr_742
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.10.0-68b65f8

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 4c207497-f260-46fc-983c-fdf77a407093

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/742/head:pr_742 && git checkout pr_742
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.10.0-9cab1b0

@aLekSer
Copy link
Collaborator Author

aLekSer commented Apr 25, 2019

Updated cloudbuild-yaml also. Now if I run:
make -j 5 test
I would see 5 parallel running images as expected (docker ps --format "table {{.Image}}\t{{.Command}}"):

IMAGE                              COMMAND                                                                                                                  
agones-build-sdk-go:c168fcc53f     "/root/entrypoint.sh…"                                                                                             
agones-build-sdk-node:c168fcc53f   "/root/entrypoint.sh…"                                                                                             
agones-build:c168fcc53f            "bash -c /go/src/ago…"                                                                                             
agones-build:c168fcc53f            "go test -mod=vendor…"                                                                                             
agones-build:c168fcc53f            "bash -c /go/src/ago…"

@markmandel markmandel added area/build-tools Development tooling. I.e. pretty much everything in the `build` directory. kind/cleanup Refactoring code, fixing up documentation, etc labels Apr 27, 2019
Combine multiple targets into one `run-all-sdk-command` make target.
Now we can run make -j 2 test-sdks.
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 94f5a06c-a30c-44c2-888e-8575ad434dbe

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/742/head:pr_742 && git checkout pr_742
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.10.0-2e1a2cb

Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@markmandel markmandel merged commit 1cc5339 into googleforgames:master Apr 30, 2019
@markmandel markmandel added this to the 0.10.0 milestone May 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build-tools Development tooling. I.e. pretty much everything in the `build` directory. kind/cleanup Refactoring code, fixing up documentation, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants