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

CI: Adding E2E cluster name as a parameter for CloudBuild #1611

Merged
merged 5 commits into from Jun 23, 2020

Conversation

aLekSer
Copy link
Collaborator

@aLekSer aLekSer commented Jun 3, 2020

This is needed to make E2E tests in parallel on a stable version and with FeatureGates.
Remove dependencies of E2E cloud builds steps,
because we are using TestCluster name as prefix.

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking
/kind bug

/kind cleanup

/kind documentation
/kind feature
/kind hotfix

What this PR does / Why we need it:

Which issue(s) this PR fixes:

For #1411

Special notes for your reviewer:

Requested here: #1546

@aLekSer aLekSer added the area/tests Unit tests, e2e tests, anything to make sure things don't break label Jun 3, 2020
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 317eff69-332d-41de-b19a-4b0151c88f93

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/1611/head:pr_1611 && git checkout pr_1611
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-2574e7a

@aLekSer aLekSer requested a review from markmandel June 3, 2020 11:49
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 536a4a9a-7599-497c-8bca-52ee0089cc36

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/1611/head:pr_1611 && git checkout pr_1611
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-07f1ac6

@aLekSer aLekSer force-pushed the e2e/add-cluster-var branch 2 times, most recently from 51c2342 to 6738d48 Compare June 3, 2020 12:09
@aLekSer aLekSer added the kind/breaking Breaking change label Jun 3, 2020
@aLekSer
Copy link
Collaborator Author

aLekSer commented Jun 3, 2020

This one Pull Request would break parallel E2E test builds in other PR. But after merging this to master and to all PRs this issue would be gone.
As an option I can add one more like if [ name = test-cluster-e2e] use LockE2E as a prefix for consul lock.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 151886e2-22b4-43a0-8b20-93cc16d7e445

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/1611/head:pr_1611 && git checkout pr_1611
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-6738d48

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 43f0d06f-d2cd-45df-bcba-c33262d69fc8

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/1611/head:pr_1611 && git checkout pr_1611
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-51c2342

--zone=us-west1-c --project=agones-images
kubectl port-forward statefulset/consul 8500:8500 &
echo "Waiting consul port-forward to launch on 8500..."
timeout 60 bash -c 'until printf "" 2>>/dev/null >>/dev/tcp/$0/$1; do sleep 1; done' 127.0.0.1 8500
echo "consul port-forward launched. Starting e2e tests..."
echo "consul lock -child-exit-code=true -timeout 30m -try 30m -verbose LockE2E '/root/e2e.sh "$FEATURES""
consul lock -child-exit-code=true -timeout 30m -try 30m -verbose LockE2E '/root/e2e.sh "'$FEATURES'"'
echo "consul lock -child-exit-code=true -timeout 30m -try 30m -verbose "$TEST_CLUSTER_NAME" '/root/e2e.sh "$FEATURES""
Copy link
Member

Choose a reason for hiding this comment

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

Since we're going to either run e2e tests in serial in the same cluster, or in parallel on different clusters, do we need to change the name? (not that is likely matters), just curious?

Copy link
Collaborator Author

@aLekSer aLekSer Jun 4, 2020

Choose a reason for hiding this comment

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

I can add Lock- prefix to the cluster name. This approach would work for both options - serial and parallel. If someone acquire the lock by his cluster name, others would wait. What name do you expect here?

Copy link
Member

Choose a reason for hiding this comment

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

oh wait! I think I understand - we only have one consul in one cluster, yes? So we use that for both locks?

I think I was assuming there was a consul in each cluster, and it would lock against each one.

Copy link
Collaborator Author

@aLekSer aLekSer Jun 10, 2020

Choose a reason for hiding this comment

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

This option with a separate name would work for both cases.
But yes we have a consul per Cluster, so no need to change this prefix parameter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

helm install --wait --set Replicas=1,uiService.type=ClusterIP --name consul stable/consul

Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking that a consul per cluster will be easier, otherwise if you are running a seperate build on a different cluster, you need to know about 2 clusters, not just one
-- and proxy to another while sending commands to the 2nd. Could get complicated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it is a consul per cluster in this design. I have overseen this before, return back.

The next step would be to remove dependencies of cloud builds step,
because we are using TestCluster name as prefix.
Add cluster name for make targets.
--zone=us-west1-c --project=agones-images
kubectl port-forward statefulset/consul 8500:8500 &
echo "Waiting consul port-forward to launch on 8500..."
timeout 60 bash -c 'until printf "" 2>>/dev/null >>/dev/tcp/$0/$1; do sleep 1; done' 127.0.0.1 8500
echo "consul port-forward launched. Starting e2e tests..."
echo "consul lock -child-exit-code=true -timeout 30m -try 30m -verbose LockE2E '/root/e2e.sh "$FEATURES""
echo "consul lock -child-exit-code=true -timeout 30m -try 30m -verbose LockE2E '/root/e2e.sh "$FEATURES"'"
Copy link
Member

Choose a reason for hiding this comment

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

Is this apostrophe correct? Should there be a partner somewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It should result in '/root/e2e.sh $FEATURES'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that this is echo command. From Cloud build:

Step #22 - "e2e-stable": Handling connection for 8500
Step #22 - "e2e-stable": consul port-forward launched. Starting e2e tests...
Step #22 - "e2e-stable": consul lock -child-exit-code=true -timeout 30m -try 30m -verbose LockE2E '/root/e2e.sh '
Step #21 - "e2e-feature-gates": Handling connection for 8500
Step #21 - "e2e-feature-gates": consul port-forward launched. Starting e2e tests...
Step #21 - "e2e-feature-gates": consul lock -child-exit-code=true -timeout 30m -try 30m -verbose LockE2E '/root/e2e.sh PlayerTracking=true&ContainerPortAllocation=true'

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.

LGTM!

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aLekSer, markmandel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@markmandel markmandel added this to the 1.7.0 milestone Jun 22, 2020
@google-oss-robot
Copy link

New changes are detected. LGTM label has been removed.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 295b34bf-5b89-4841-841c-752be53a0813

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: a43b3ece-61e8-4b81-b32b-9dc2fd7a6c37

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/googleforgames/agones.git pull/1611/head:pr_1611 && git checkout pr_1611
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-9e50640

@markmandel markmandel merged commit 91a9d9a into googleforgames:master Jun 23, 2020
@markmandel markmandel added kind/feature New features for Agones and removed kind/breaking Breaking change labels Jun 30, 2020
ilkercelikyilmaz pushed a commit to ilkercelikyilmaz/agones that referenced this pull request Oct 23, 2020
…ames#1611)

* Adding name of a cluster as a parameter

The next step would be to remove dependencies of cloud builds step,
because we are using TestCluster name as prefix.

* Remove dependency of E2E stable run from features

* Fix Consul lock variable name.

Add cluster name for make targets.

Co-authored-by: Mark Mandel <markmandel@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/tests Unit tests, e2e tests, anything to make sure things don't break cla: yes kind/feature New features for Agones size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants