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

GameServer Pod: Stable Network ID #2826

Merged
merged 3 commits into from
Dec 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ KIND_CONTAINER_NAME=$(KIND_PROFILE)-control-plane
GS_TEST_IMAGE ?= us-docker.pkg.dev/agones-images/examples/simple-game-server:0.14

# Enable all alpha feature gates. Keep in sync with `false` (alpha) entries in pkg/util/runtime/features.go:featureDefaults
ALPHA_FEATURE_GATES ?= "PlayerAllocationFilter=true&PlayerTracking=true&ResetMetricsOnDelete=true&SafeToEvict=true&Example=true"
ALPHA_FEATURE_GATES ?= "PlayerAllocationFilter=true&PlayerTracking=true&ResetMetricsOnDelete=true&SafeToEvict=true&PodHostname=true&Example=true"

# Build with Windows support
WITH_WINDOWS=1
Expand Down
2 changes: 1 addition & 1 deletion cloudbuild.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ steps:

- name: 'e2e-runner'
args:
- 'CustomFasSyncInterval=false&SDKGracefulTermination=false&StateAllocationFilter=false&PlayerAllocationFilter=true&PlayerTracking=true&ResetMetricsOnDelete=true&SafeToEvict=true&Example=true'
- 'CustomFasSyncInterval=false&SDKGracefulTermination=false&StateAllocationFilter=false&PlayerAllocationFilter=true&PlayerTracking=true&ResetMetricsOnDelete=true&SafeToEvict=true&PodHostname=true&Example=true'
- 'e2e-test-cluster'
- "${_REGISTRY}"
id: e2e-feature-gates
Expand Down
1 change: 1 addition & 0 deletions install/helm/agones/defaultfeaturegates.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ PlayerAllocationFilter: false
PlayerTracking: false
ResetMetricsOnDelete: false
SafeToEvict: false
PodHostname: false

# Pre-Alpha features
SplitControllerAndExtensions: false
Expand Down
6 changes: 6 additions & 0 deletions pkg/apis/agones/v1/gameserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"encoding/json"
"fmt"
"net"
"strings"

"agones.dev/agones/pkg"
"agones.dev/agones/pkg/apis"
Expand Down Expand Up @@ -598,6 +599,11 @@ func (gs *GameServer) Pod(sidecars ...corev1.Container) (*corev1.Pod, error) {
Spec: *gs.Spec.Template.Spec.DeepCopy(),
}

if len(pod.Spec.Hostname) == 0 {
// replace . with - since it must match RFC 1123
pod.Spec.Hostname = strings.ReplaceAll(gs.ObjectMeta.Name, ".", "-")
}

gs.podObjectMeta(pod)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just adding this as a formal review comment. I think since this is altering the generated pod in a possibly significant way, we should add a feature gate. It can go straight to beta, IMO - but especially in a world where we're releasing straight from main, I think we should be a little cautious.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good to me - I'll make the change.

Copy link
Member Author

Choose a reason for hiding this comment

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

...and done!

for _, p := range gs.Spec.Ports {
cp := corev1.ContainerPort{
Expand Down
21 changes: 21 additions & 0 deletions pkg/apis/agones/v1/gameserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"agones.dev/agones/pkg/apis/agones"
"agones.dev/agones/pkg/util/runtime"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -1154,6 +1155,7 @@ func TestGameServerPodNoErrors(t *testing.T) {
pod, err := fixture.Pod()
assert.Nil(t, err, "Pod should not return an error")
assert.Equal(t, fixture.ObjectMeta.Name, pod.ObjectMeta.Name)
assert.Equal(t, fixture.ObjectMeta.Name, pod.Spec.Hostname)
assert.Equal(t, fixture.ObjectMeta.Namespace, pod.ObjectMeta.Namespace)
assert.Equal(t, "gameserver", pod.ObjectMeta.Labels[agones.GroupName+"/role"])
assert.Equal(t, fixture.ObjectMeta.Name, pod.ObjectMeta.Labels[GameServerPodLabel])
Expand All @@ -1165,6 +1167,25 @@ func TestGameServerPodNoErrors(t *testing.T) {
assert.True(t, metav1.IsControlledBy(pod, fixture))
}

func TestGameServerPodHostName(t *testing.T) {
t.Parallel()

fixture := defaultGameServer()
fixture.ObjectMeta.Name = "test-1.0"
fixture.ApplyDefaults()
pod, err := fixture.Pod()
require.NoError(t, err)
assert.Equal(t, "test-1-0", pod.Spec.Hostname)

fixture = defaultGameServer()
fixture.ApplyDefaults()
expected := "ORANGE"
fixture.Spec.Template.Spec.Hostname = expected
pod, err = fixture.Pod()
require.NoError(t, err)
assert.Equal(t, expected, pod.Spec.Hostname)
}

func TestGameServerPodContainerNotFoundErrReturned(t *testing.T) {
t.Parallel()

Expand Down
4 changes: 4 additions & 0 deletions pkg/util/runtime/features.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ const (
// FeatureSafeToEvict enables the `SafeToEvict` API to specify disruption tolerance.
FeatureSafeToEvict Feature = "SafeToEvict"

// FeaturePodHostname enables the Pod Hostname being assigned the name of the GameServer
FeaturePodHostname = "PodHostname"

////////////////
// "Pre"-Alpha features

Expand Down Expand Up @@ -107,6 +110,7 @@ var (
FeaturePlayerTracking: false,
FeatureResetMetricsOnDelete: false,
FeatureSafeToEvict: false,
FeaturePodHostname: false,

// Pre-Alpha features
FeatureSplitControllerAndExtensions: false,
Expand Down
1 change: 1 addition & 0 deletions site/content/en/docs/Guides/feature-stages.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ The current set of `alpha` and `beta` feature gates:
| [GameServer player capacity filtering on GameServerAllocations](https://github.com/googleforgames/agones/issues/1239) | `PlayerAllocationFilter` | Disabled | `Alpha` | 1.14.0 |
| [Player Tracking]({{< ref "/docs/Guides/player-tracking.md" >}}) | `PlayerTracking` | Disabled | `Alpha` | 1.6.0 |
| [Reset Metric Export on Fleet / Autoscaler deletion]({{% relref "./metrics.md#dropping-metric-labels" %}}) | `ResetMetricsOnDelete` | Disabled | `Alpha` | 1.26.0 |
| [GameServer Stable Network ID]({{% ref "/docs/Reference/gameserver.md#stable-network-id" %}}) | `PodHostname` | Disabled | `Alpha` | 1.29.0 |
| [GameServer `SafeToEvict` API](https://github.com/googleforgames/agones/issues/2794) | `SafeToEvict` | Disabled | `Alpha` | 1.29.0 |
| Example Gate (not in use) | `Example` | Disabled | None | 0.13.0 |
{{% /feature %}}
Expand Down
19 changes: 19 additions & 0 deletions site/content/en/docs/Reference/gameserver.md
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,25 @@ The `spec` field is the actual GameServer specification and it is composed as fo
The GameServer resource does not support updates. If you need to make regular updates to the GameServer spec, consider using a [Fleet]({{< ref "/docs/Reference/fleet.md" >}}).
{{< /alert >}}

## Stable Network ID

{{< alpha title="Stable Network ID" gate="PodHostname" >}}

Each Pod attached to a `GameServer` derives its hostname from the name of the `GameServer`.
A group of `Pods` attached to `GameServers` can use a
[Headless Service](https://kubernetes.io/docs/concepts/services-networking/service/#headless-services) to control
the domain of the Pods, along with providing
a [`subdomain` value to the `GameServer` `PodTemplateSpec`](https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/#pod-s-hostname-and-subdomain-fields)
Copy link
Member

Choose a reason for hiding this comment

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

This documentation says:

Currently when a Pod is created, its hostname is the Pod's metadata.name value.

Which sounds like k8s already does exactly what you are adding in this PR. Do we need to explicitly set the hostname field?

Copy link
Member Author

Choose a reason for hiding this comment

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

I went back and forth on this as well, but then I read this part:
https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/#pod-s-hostname-and-subdomain-fields:~:text=Note%3A%20Because,on%20the%20Service.

Note: Because A or AAAA records are not created for Pod names, hostname is required for the Pod's A or AAAA record to be created. A Pod with no hostname but with subdomain will only create the A or AAAA record for the headless Service (default-subdomain.my-namespace.svc.cluster-domain.example), pointing to the Pod's IP address. Also, Pod needs to become ready in order to have a record unless publishNotReadyAddresses=True is set on the Service.

So TL;DR - to have a A record for the pod, it has to have a hostname specified.

This isn't surprising to me - since the (general) formatting for a resource name allows characters and lengths of strings that aren't allowed in host names for DNS entries.

I think where they say "hostname" above, what they mean is the internal hostname to the container - not an external DNS entry.

to provide all the required details such that Kubernetes will create a DNS record for each Pod behind the Service.

You are also responsible for setting the labels on the `GameServer.Spec.Template.Metadata` to set the labels on the
created Pods and creating the Headless Service responsible for the network identity of the pods, Agones will not do
this for you, as a stable DNS record is not required for all use cases.

To ensure that the `hostName` value matches
[RFC 1123](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-label-names), any `.` values
in the `GameServer` name are replaced by `-` when setting the `hostName` value.

## GameServer State Diagram

The following diagram shows the lifecycle of a `GameServer`.
Expand Down
51 changes: 51 additions & 0 deletions test/e2e/gameserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,57 @@ func TestCreateConnect(t *testing.T) {
assert.Equal(t, "ACK: Hello World !\n", reply)
}

func TestHostName(t *testing.T) {
t.Parallel()

pods := framework.KubeClient.CoreV1().Pods(framework.Namespace)

fixtures := map[string]struct {
setup func(gs *agonesv1.GameServer)
test func(gs *agonesv1.GameServer, pod *corev1.Pod)
}{
"standard hostname": {
setup: func(_ *agonesv1.GameServer) {},
test: func(gs *agonesv1.GameServer, pod *corev1.Pod) {
assert.Equal(t, gs.ObjectMeta.Name, pod.Spec.Hostname)
},
},
"a . in the name": {
setup: func(gs *agonesv1.GameServer) {
gs.ObjectMeta.GenerateName = "game-server-1.0-"
},
test: func(gs *agonesv1.GameServer, pod *corev1.Pod) {
expected := "game-server-1-0-"
// since it's a generated name, we just check the beginning.
assert.Equal(t, expected, pod.Spec.Hostname[:len(expected)])
},
},
// generated name will automatically truncate to 63 chars.
"generated with > 63 chars": {
setup: func(gs *agonesv1.GameServer) {
gs.ObjectMeta.GenerateName = "game-server-" + strings.Repeat("n", 100)
},
test: func(gs *agonesv1.GameServer, pod *corev1.Pod) {
assert.Equal(t, gs.ObjectMeta.Name, pod.Spec.Hostname)
},
},
// Note: no need to test for a gs.ObjectMeta.Name > 63 chars, as it will be rejected as invalid
}

for k, v := range fixtures {
t.Run(k, func(t *testing.T) {
gs := framework.DefaultGameServer(framework.Namespace)
gs.Spec.Template.Spec.Subdomain = "default"
v.setup(gs)
readyGs, err := framework.CreateGameServerAndWaitUntilReady(t, framework.Namespace, gs)
require.NoError(t, err)
pod, err := pods.Get(context.Background(), readyGs.ObjectMeta.Name, metav1.GetOptions{})
require.NoError(t, err)
v.test(readyGs, pod)
})
}
}

// nolint:dupl
func TestSDKSetLabel(t *testing.T) {
t.Parallel()
Expand Down