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

Cloud Build Script For Xonotic #3286

Closed

Conversation

Kalaiselvi84
Copy link
Contributor

@Kalaiselvi84 Kalaiselvi84 commented Jul 25, 2023

What type of PR is this?

Uncomment only one /kind <> line, press 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
/kind release

What this PR does / Why we need it:

Which issue(s) this PR fixes:

Closes #3281

Special notes for your reviewer:

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Kalaiselvi84
Once this PR has been reviewed and has the lgtm label, please assign alekser for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 8314bb93-d789-4189-83f9-d27e0229b2fb

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

@markmandel
Copy link
Member

Flake:

{"error":null,"message":"Allocate (public): failed allocation","severity":"info","test":"TestAllocatorAllocateOnGameServerUpdateError","time":"2023-07-25T23:15:56.853535967Z"}
--- FAIL: TestAllocatorAllocateOnGameServerUpdateError (22.99s)
    allocator_test.go:503: 
        	Error Trace:	/go/src/agones.dev/agones/pkg/gameserverallocations/allocator_test.go:503
        	Error:      	An error is expected but got nil.
        	Test:       	TestAllocatorAllocateOnGameServerUpdateError

Is this PR up to date with main? If not, this flake is not gone 😡


mkfile_path := $(abspath $(lastword $(MAKEFILE_LIST)))
project_path := $(dir $(mkfile_path))
root_path := $(realpath $(project_path)/../..)
image_tag = $(REPOSITORY)/xonotic-example:1.1
image_tag = xonotic-example:1.1
Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure this will work:

Suggested change
image_tag = xonotic-example:1.1
image_tag = xonotic-example:1.1
ifneq ($(REPOSITORY),)
image_tag := $(REPOSITORY)/${image_tag)
endif

Then you don't have to add it all the way through, and also, a tag can't start with "/".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an if condition for image_tag


# check if hosted on Google Artifact Registry
gar-check:
gcloud container images describe $(image_tag)
gcloud container images describe $(REPOSITORY)/$(image_tag)
Copy link
Member

Choose a reason for hiding this comment

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

I think we'll need something like a PRODUCTION_REPOSITORY variable (or something like that) so that gar-check and echo-image-tag still works as it did previously (since they are used for the release process).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Included PROD_REPO variable with agones-images but I'm unsure about the behavior of gar-check and echo-image-tag targets🤔

dir: "."
env:
- REPOSITORY=${_REPOSITORY}
- IMAGE_TAG=${_IMAGE_TAG}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to pass in the IMAGE_TAG, since we already have that defined in the Makefile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense. Removed IMAGE_TAG👍🏻

args: ["-j", "1", "push"]

substitutions:
_REPOSITORY: 'us-docker.pkg.dev/agones-images/examples'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_REPOSITORY: 'us-docker.pkg.dev/agones-images/examples'
_REPOSITORY: 'us-docker.pkg.dev/${PROJECT_ID}/examples'

Then you can easily test it against whatever project you like, which is super nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially, I gave it a shot with ${PROJECT_ID}, but unfortunately, the build didn't fetch my project, leading to the following error:

"build": docker build -f /workspace//Dockerfile --tag=us-docker.pkg.dev//examples/xonotic-example:1.1 .
"build": invalid argument "us-docker.pkg.dev//examples/xonotic-example:1.1" for "-t, --tag" flag: invalid reference format

Since I added PROD_REPO ?= us-docker.pkg.dev/agones-images/examples, the issue seems to be resolved.

@Kalaiselvi84
Copy link
Contributor Author

Is this PR up to date with main?
Yes, this branch is up to date. can I create a new PR if it is required?

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 8e4a43ef-86a7-43d0-9b8d-e9672869d28b

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/3286/head:pr_3286 && git checkout pr_3286
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.34.0-dev-e25a9b7-amd64

@markmandel
Copy link
Member

Yes, this branch is up to date. can I create a new PR if it is required?

No, it just means I have to work out why that flake is still happening. I thought I had fixed it.

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.

Almost there!

Also - just checking with the new changes, have you checked the cloud build script against your personal dev Google Cloud project?

ifeq ($(REPOSITORY),)
image_tag := xonotic-example:1.1
else
image_tag := $(PROD_REPO)/xonotic-example:1.1
Copy link
Member

Choose a reason for hiding this comment

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

So my original thought was to do the following:

Suggested change
image_tag := $(PROD_REPO)/xonotic-example:1.1
image_tag := $(REPOSITORY)/xonotic-example:1.1

That would mean that echo-image-tag would need then to be:

echo-image-tag:
	@echo $(PROD_REPO)/$(image_tag)

Which I think is fine? WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated it as per your suggestion

args:
- "bash"
- "-c"
- "echo 'FROM gcr.io/cloud-builders/docker\nRUN apt-get install make\nENTRYPOINT [\"/usr/bin/make\"]' > Dockerfile.build"
Copy link
Member

Choose a reason for hiding this comment

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

Non blocking, but this could actually now use the new script element in Cloud Build:

https://cloud.google.com/build/docs/configuring-builds/run-bash-scripts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

script block added in the steps

@Kalaiselvi84
Copy link
Contributor Author

Also - just checking with the new changes, have you checked the cloud build script against your personal dev Google Cloud project?

Yes, you can find the successful log here

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: af20177c-d04d-41d8-b0b8-e68d668c16c7

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/3286/head:pr_3286 && git checkout pr_3286
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.34.0-dev-943b624-amd64

@google-oss-prow google-oss-prow bot added size/L and removed size/M labels Jul 27, 2023
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 18ab7c79-d909-4b71-80b5-29675f052112

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


# build and push the autoscaler-webhook image with specified tag
cloud-build:
gcloud builds submit --config=../cloudbuild.yaml
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While working on the autoscaler-webhook image, I encountered the following error:

Step #2 - "build": Already have image: make-docker
Step #2 - "build": cd / && docker build -f /workspace//Dockerfile --tag=us-docker.pkg.dev/agones-mangalpalli/examples/autoscaler-webhook:0.7 .
Step #2 - "build": error checking context: 'no permission to read from '/dev/mem''.
Step #2 - "build": make: *** [Makefile:47: build] Error 1
It seems to be related to permission issues while trying to access '/dev/mem' during the build process. Could you please advise if there's a specific condition we need to add to the build process to avoid this error?

Copy link
Member

Choose a reason for hiding this comment

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

I think we'll need something similar to what we have in make build, which will be something like:

Suggested change
gcloud builds submit --config=../cloudbuild.yaml
cd $(root_path) && gcloud builds submit --config=./examples/autoscaler-webhook/cloudbuild.yaml

And then will likely need up update some of the paths to work from /workspace/examples/autoscaler-webhook/

entrypoint: "docker"
args: ["build", "-f", "Dockerfile.build", "-t", "make-docker", "."]

# build xonotic image to GCR
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to build first, we only need to push.

@google-oss-prow google-oss-prow bot added size/M and removed size/L labels Jul 27, 2023
@Kalaiselvi84 Kalaiselvi84 changed the title cloud build script for xonotic Cloud Build Script For Xonotic Jul 27, 2023
@Kalaiselvi84
Copy link
Contributor Author

Built succeeded for Xonotic image in my project

@Kalaiselvi84 Kalaiselvi84 marked this pull request as ready for review July 27, 2023 18:20
@Kalaiselvi84 Kalaiselvi84 added kind/feature New features for Agones and removed kind/other labels Jul 27, 2023
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 3a295f85-5761-43ab-a8c0-050136ff5838

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 76a1e1dc-4447-47bf-a3cd-114f92bf7f28

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

@markmandel
Copy link
Member

Just testing #3273 first, will probably merge that first, then we can do a double check on this PR to make everything still works as expected.

@markmandel
Copy link
Member

#3273 is merged - shall we either update this to just include the Xonotic changes, or create a new PR?

Also, we should make sure the changes in #3273 still work with the Cloud Build script.

@Kalaiselvi84
Copy link
Contributor Author

#3273 is merged - shall we either update this to just include the Xonotic changes, or create a new PR?

Also, we should make sure the changes in #3273 still work with the Cloud Build script.

will create a new PR.

@Kalaiselvi84 Kalaiselvi84 deleted the cloud-build-script branch March 15, 2024 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New features for Agones size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cloud Build script for each Example Image
3 participants