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

Simple Game Server Example: Upgrade Docker to 24.0.6 #3531

Merged
merged 9 commits into from
Dec 14, 2023

Conversation

Kalaiselvi84
Copy link
Contributor

@Kalaiselvi84 Kalaiselvi84 commented Dec 5, 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:

Work on #3530

Special notes for your reviewer:

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

@Kalaiselvi84
Copy link
Contributor Author

Here's the failed cloud-build log - https://gist.github.com/Kalaiselvi84/2abc327822338b70e3e082da45ff42fa

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 5edcf548-6ed5-4233-8ff9-02d689e60c21

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/3531/head:pr_3531 && git checkout pr_3531
  • 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.37.0-dev-7548748-amd64

@Kalaiselvi84
Copy link
Contributor Author

Build has passed with --use flag in this line: -DOCKER_CLI_EXPERIMENTAL=enabled docker buildx create --name=$(BUILDX_WINDOWS_BUILDER) --use

Log - https://gist.github.com/Kalaiselvi84/6b02ec26b5c8d4c0362060f81e31cd29

If this looks good, let me know the next steps please.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: b1be2b1e-67be-4393-bed0-a127b498a5c9

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/3531/head:pr_3531 && git checkout pr_3531
  • 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.37.0-dev-094afda-amd64

@markmandel markmandel changed the title Upgrade Docker to 24.0.6 Xonotic Example: Upgrade Docker to 24.0.6 Dec 5, 2023
@markmandel markmandel changed the title Xonotic Example: Upgrade Docker to 24.0.6 Simple Game Server Example: Upgrade Docker to 24.0.6 Dec 5, 2023
@markmandel
Copy link
Member

Build has passed with --use flag in this line: -DOCKER_CLI_EXPERIMENTAL=enabled docker buildx create --name=$(BUILDX_WINDOWS_BUILDER) --use

Log - https://gist.github.com/Kalaiselvi84/6b02ec26b5c8d4c0362060f81e31cd29

If this looks good, let me know the next steps please.

Yes! Absolutely good catch. In fact as part of this change, we should remove all DOCKER_CLI_EXPERIMENTAL=enabled from commands, since they are no longer required (and we should be able to do all this stuff without this change).

Re --use, all buildx commands should specify the --builder command instead to specify which builder they are using (--use sets all following commands to use that named builder). For example, we do this properly here:

docker buildx build --platform windows/amd64 --builder $(BUILDX_WINDOWS_BUILDER) -f $(project_path)Dockerfile.windows \
--tag=$(server_tag)-windows_amd64-$* --build-arg WINDOWS_VERSION=$* . $(WINDOWS_DOCKER_PUSH_ARGS)

Lemme know if that doesn't make sense - I'll also make a note on the parent issue (I edited the description)

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: bd3f5a4e-825a-4bbe-a02e-0c196d15d97a

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/3531/head:pr_3531 && git checkout pr_3531
  • 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.37.0-dev-4bef126-amd64

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.

Approach looks good - is this successfully building the image and container?

@@ -87,13 +87,9 @@ endif
ifeq ($(WITH_ARM64), 1)
push: push-linux-image-arm64
endif
-docker manifest rm $(server_tag)
Copy link
Member

Choose a reason for hiding this comment

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

Just noting we'll want to increment the version number as well in here, since this will be a new build (then we will want to update the simple-game-server version everywhere as well - likely in a separate PR).

@Kalaiselvi84
Copy link
Contributor Author

Kalaiselvi84 commented Dec 7, 2023

Approach looks good - is this successfully building the image and container?

Build has passed with https://github.com/googleforgames/agones/blob/main/examples/simple-game-server/Makefile#L122

Failed Scenarios:

  1. The build failed without using the --use flag and DOCKER_CLI_EXPERIMENTAL=enabled in https://github.com/googleforgames/agones/blob/main/examples/simple-game-server/Makefile#L122. Log: https://gist.github.com/Kalaiselvi84/38a273a6dbb7e308fdb4be166fc597fd

  2. Without --use flag: https://gist.github.com/Kalaiselvi84/2abc327822338b70e3e082da45ff42fa

  3. Without DOCKER_CLI_EXPERIMENTAL=enabled: https://gist.github.com/Kalaiselvi84/0d0b5fcc8de6fb9f31ac0d8588f40fda

====================

We are using DOCKER_CLI_EXPERIMENTAL=enabled in https://github.com/search?q=repo%3Agoogleforgames%2Fagones%20DOCKER_CLI_EXPERIMENTAL%3Denabled&type=code

Is it fine to remove this flag from build/Makefile?

@markmandel
Copy link
Member

My apologies, I'm not following - does it work as it is currently written in the PR?

@Kalaiselvi84
Copy link
Contributor Author

My apologies, I'm not following - does it work as it is currently written in the PR?

Sorry for the confusion, it isn't working

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 213999ec-2960-4f7e-8b2e-25e0135cfa70

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

@markmandel
Copy link
Member

Just playing with this myself - there is definitely some weird stuff going on...

For future reference:

Locally I'm running Docker 24.x (client and server), but on Cloud Build, we have a 24.x Client on a 20.x Docker server.

I think that is why we get weirdness on Cloud Build, when it seems to be fine building locally.

The build failed without using the --use flag and DOCKER_CLI_EXPERIMENTAL=enabled in https://github.com/googleforgames/agones/blob/main/examples/simple-game-server/Makefile#L122

I think I've just ended up in exactly what you were originally trying to say 🤦🏻 just doing some tests to ensure it works across both versions.

@Kalaiselvi84
Copy link
Contributor Author

Locally I'm running Docker 24.x (client and server), but on Cloud Build, we have a 24.x Client on a 20.x Docker server.

Got it..
Does the Google Cloud team have any plans to upgrade the Docker server to version 24.x in the Cloud Build env?
Next step please?

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.

This is what I ended up with that worked across local Docker, 24.x and Cloud Build -- does this work for you too? If so, let's run with it, and update the issue for these instructions.

@@ -109,17 +105,14 @@ push-windows-image-%:
$(MAKE) WINDOWS_DOCKER_PUSH_ARGS=--push build-windows-image-$*

build-windows-image-%: ensure-windows-buildx
cd $(root_path) && DOCKER_CLI_EXPERIMENTAL=enabled \
cd $(root_path) && \
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
cd $(root_path) && \
cd $(root_path) && DOCKER_CLI_EXPERIMENTAL=enabled \

-DOCKER_CLI_EXPERIMENTAL=enabled docker buildx create --name=$(BUILDX_WINDOWS_BUILDER) --use
# Windows image builds must be directed to a specific buildx context.
# The default context does not support building cross platform images.
-docker buildx create --name=$(BUILDX_WINDOWS_BUILDER)
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
-docker buildx create --name=$(BUILDX_WINDOWS_BUILDER)
-docker buildx create --name=$(BUILDX_WINDOWS_BUILDER) --use

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to follow what you're suggesting. Just a thought – should we aim for uniformity between the simple-game-server and Xonotic?
https://github.com/googleforgames/agones/blob/main/examples/xonotic/Makefile#L86-L93

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@markmandel
Copy link
Member

Just also making a note that after this change (and pushing the new image to prod), will need to change all the other references to simple-game-server:0.22 - such as the website, the e2e framework, etc.

@Kalaiselvi84
Copy link
Contributor Author

Just also making a note that after this change (and pushing the new image to prod), will need to change all the other references to simple-game-server:0.22 - such as the website, the e2e framework, etc.

Shall I push the latest tag to prod now or should I do it after this PR is merged?

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: e0e9b072-cbd1-4fb3-8e62-cbbad6ce1df0

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/3531/head:pr_3531 && git checkout pr_3531
  • 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.37.0-dev-8f0a39d-amd64

@Kalaiselvi84 Kalaiselvi84 marked this pull request as ready for review December 11, 2023 23:08
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: af2c8db8-87e6-48aa-beef-48a5bafbc107

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/3531/head:pr_3531 && git checkout pr_3531
  • 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.37.0-dev-81de148-amd64

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 0a5fb708-b433-4e95-a858-fc4299c96263

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/3531/head:pr_3531 && git checkout pr_3531
  • 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.37.0-dev-b08830a-amd64

@markmandel
Copy link
Member

Just also making a note that after this change (and pushing the new image to prod), will need to change all the other references to simple-game-server:0.22 - such as the website, the e2e framework, etc.

Shall I push the latest tag to prod now or should I do it after this PR is merged?

Either or. I'll approve this now, so push it when it's ready, and then can do the update to the rest in a separate PR.

@markmandel markmandel merged commit 75fc23a into googleforgames:main Dec 14, 2023
4 checks passed
@Kalaiselvi84 Kalaiselvi84 added kind/cleanup Refactoring code, fixing up documentation, etc and removed do-not-merge/work-in-progress kind/other labels Dec 18, 2023
@Kalaiselvi84 Kalaiselvi84 deleted the latest-docker branch March 15, 2024 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Refactoring code, fixing up documentation, etc size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants