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.

Sign up for GitHub

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

Jump to bottom

Changed how GameServer POD names are generated #565

Merged
merged 1 commit into from
Feb 6, 2019
Merged

Changed how GameServer POD names are generated #565

merged 1 commit into from
Feb 6, 2019

Conversation

jkowalski
Copy link
Contributor

@jkowalski jkowalski commented Feb 6, 2019 •
edited
Loading

Instead of generating name based on GS prefix, we take GS name as-is.
This is safe, since Pods are owned by Gameservers, so their lifetime is a subset of that of GS, thus we won't have accidental pod name collisions.

This fixes #490

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 0c41fa66-6bfc-45bc-92e0-06eea5e9c26f

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

ilkercelikyilmaz
ilkercelikyilmaz reviewed Feb 6, 2019
View reviewed changes
pkg/apis/stable/v1alpha1/gameserver.go
pod.ObjectMeta.GenerateName = ""
// Pods inherit the name of their gameserver. It's safe since there's
// a guarantee that pod won't outlive its parent.
pod.ObjectMeta.Name = gs.ObjectMeta.Name
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested this locally and it seems work.

@markmandel
Copy link
Member

We will need to add a section here in the gameservers controller, such that if k8serrors.IsAlreadyExists(err) - then continue through, otherwise we're going to end up in an infinite backoff if the pod already exists, as the error will keep getting rethrown.

@markmandel markmandel added the kind/bug These are bugs. label Feb 6, 2019
@markmandel markmandel added this to the 0.8.0 milestone Feb 6, 2019
@jkowalski
Changed how GameServer POD names are generated:
a0425a1 
Instead of generating name based on GS prefix, we take GS name as-is.
This is safe, since Pods are owned by Gameservers, so their lifetime is a subset of that of GS, thus we won't have accidental pod name collisions.
@jkowalski
Copy link
Contributor Author

We will need to add a section here in the gameservers controller, such that if k8serrors.IsAlreadyExists(err) - then continue through, otherwise we're going to end up in an infinite backoff if the pod already exists, as the error will keep getting rethrown.

Done.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 2109198a-6f83-4f35-b52f-621b8467b613

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/565/head:pr_565 && git checkout pr_565
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.8.0-a0425a1

markmandel
markmandel approved these changes Feb 6, 2019
View reviewed changes
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.

🎆 Noice 🎆

@jkowalski jkowalski merged commit 2211878 into googleforgames:master Feb 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilkercelikyilmaz ilkercelikyilmaz ilkercelikyilmaz left review comments

@markmandel markmandel markmandel approved these changes

Assignees
No one assigned
Labels
kind/bug These are bugs.
Projects
None yet
Milestone
0.8.0
Development

Successfully merging this pull request may close these issues.

Investigate why we sometimes have multiple pods per gameserver
4 participants
@jkowalski @agones-bot @markmandel @ilkercelikyilmaz