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 AllocationEndpoint to array of endpoints #830

Merged
merged 1 commit into from
Jun 17, 2019
Merged

Changed AllocationEndpoint to array of endpoints #830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 6 additions & 3 deletions install/helm/agones/templates/crds/gameserverallocationpolicy.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,16 @@ spec:
properties:
secretName:
type: string
allocationEndpoint:
type: string
allocationEndpoints:
items:
type: string
type: array
minItems: 1
clusterName:
type: string
required:
- clusterName
- allocationEndpoint
- allocationEndpoints
- secretName
type: object
priority:
Expand Down
9 changes: 6 additions & 3 deletions install/yaml/install.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -738,13 +738,16 @@ spec:
properties:
secretName:
type: string
allocationEndpoint:
type: string
allocationEndpoints:
items:
type: string
type: array
minItems: 1
clusterName:
type: string
required:
- clusterName
- allocationEndpoint
- allocationEndpoints
- secretName
type: object
priority:
Expand Down
6 changes: 3 additions & 3 deletions pkg/apis/multicluster/v1alpha1/gameserverallocationpolicy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ type GameServerAllocationPolicySpec struct {

// ClusterConnectionInfo defines the connection information for a cluster
type ClusterConnectionInfo struct {
ClusterName string `json:"clusterName"`
AllocationEndpoint string `json:"allocationEndpoint"`
SecretName string `json:"secretName"`
ClusterName string `json:"clusterName"`
AllocationEndpoints []string `json:"allocationEndpoints"`
SecretName string `json:"secretName"`
}

// +genclient
Expand Down
35 changes: 29 additions & 6 deletions pkg/apis/multicluster/v1alpha1/gameserverallocationpolicy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,18 +35,18 @@ func TestConnectionInfoIterator(t *testing.T) {
Priority: 1,
Weight: 100,
ConnectionInfo: ClusterConnectionInfo{
ClusterName: "cluster1",
SecretName: "secret-name",
AllocationEndpoint: "allocation-endpoint",
ClusterName: "cluster1",
SecretName: "secret-name",
AllocationEndpoints: []string{"allocation-endpoint"},
},
},
},
},
want: []ClusterConnectionInfo{
{
ClusterName: "cluster1",
SecretName: "secret-name",
AllocationEndpoint: "allocation-endpoint",
ClusterName: "cluster1",
SecretName: "secret-name",
AllocationEndpoints: []string{"allocation-endpoint"},
},
},
},
Expand Down Expand Up @@ -219,6 +219,29 @@ func TestConnectionInfoIterator(t *testing.T) {
},
},
},
{
name: "Multiple allocation endpoints test",
in: []*GameServerAllocationPolicy{
{
Spec: GameServerAllocationPolicySpec{
Priority: 1,
Weight: 100,
ConnectionInfo: ClusterConnectionInfo{
ClusterName: "cluster1",
SecretName: "secret-name",
AllocationEndpoints: []string{"alloc1", "alloc2"},
},
},
},
},
want: []ClusterConnectionInfo{
{
ClusterName: "cluster1",
SecretName: "secret-name",
AllocationEndpoints: []string{"alloc1", "alloc2"},
},
},
},
{
name: "Empty policy list",
in: []*GameServerAllocationPolicy{},
Expand Down
41 changes: 24 additions & 17 deletions pkg/gameserverallocations/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,26 +398,33 @@ func (c *Controller) allocateFromRemoteCluster(gsa v1alpha1.GameServerAllocation
}

// TODO: Retry on transient error --> response.StatusCode >= 500
response, err := client.Post(connectionInfo.AllocationEndpoint, "application/json", bytes.NewBuffer(body))
if err != nil {
return nil, err
}
defer response.Body.Close() // nolint: errcheck
for i, endpoint := range connectionInfo.AllocationEndpoints {
markmandel marked this conversation as resolved.
Show resolved Hide resolved
response, err := client.Post(endpoint, "application/json", bytes.NewBuffer(body))
if err != nil {
return nil, err
}
defer response.Body.Close() // nolint: errcheck

data, err := ioutil.ReadAll(response.Body)
if err != nil {
return nil, err
}
if response.StatusCode >= 400 {
// For error responses return the body without deserializing to an object.
return nil, errors.New(string(data))
}
data, err := ioutil.ReadAll(response.Body)
if err != nil {
return nil, err
}
if response.StatusCode >= 500 && (i+1) < len(connectionInfo.AllocationEndpoints) {
Copy link
Collaborator

@aLekSer aLekSer Jun 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
if response.StatusCode >= 500 && (i+1) < len(connectionInfo.AllocationEndpoints) {
if response.StatusCode >= 500 {

I think that this check is redundant for loop will not continue if we are out of range connectionInfo.AllocationEndpoints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. This condition says if the there is any other endpoint try sending the request with the new endpoint. Otherwise return the error. If I remove the condition, no error will be returned

markmandel marked this conversation as resolved.
Show resolved Hide resolved
// If there is a server error try a different endpoint
c.baseLogger.WithError(err).WithField("endpoint", endpoint).Warn("The request sent failed, trying next endpoint")
continue
}
if response.StatusCode >= 400 {
Copy link
Member

Choose a reason for hiding this comment

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

Curious - why stop at a 400, why not continue to the next endpoint like a 500?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

4xx means there is an error from client. We only retry with new endpoint if there is an error from server e.g. 5xx.

Copy link
Member

Choose a reason for hiding this comment

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

My concern would be, that if after I set everything up, and all is well - if something weird happens to one of my clusters, such that they return a 404, that could potentially stop all/a large chunk of multicluster allocations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

404 means the client is asking gameserverallocation for a resource that is not available. It is a client error and should be handled by client. If a cluster is out of gameservers to allocate, then it returns 500. Does it make sense?

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah totally 👍 I'm just thinking through worst case scenarios.

Like someone accidentally puts a different service in place of where the allocator used to be setup 😨

Not incredibly likely, but just thinking through it.

// For error responses return the body without deserializing to an object.
return nil, errors.New(string(data))
}

err = json.Unmarshal(data, &gsaResult)
if err != nil {
return nil, err
err = json.Unmarshal(data, &gsaResult)
if err != nil {
return nil, err
}
break
}

return &gsaResult, nil
}

Expand Down
109 changes: 98 additions & 11 deletions pkg/gameserverallocations/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -788,9 +788,9 @@ func TestMultiClusterAllocationFromLocal(t *testing.T) {
Priority: 1,
Weight: 200,
ConnectionInfo: multiclusterv1alpha1.ClusterConnectionInfo{
AllocationEndpoint: "localhost",
ClusterName: "multicluster",
SecretName: "localhostsecret",
AllocationEndpoints: []string{"localhost"},
ClusterName: "multicluster",
SecretName: "localhostsecret",
},
},
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -883,6 +883,7 @@ func TestMultiClusterAllocationFromLocal(t *testing.T) {
}

func TestMultiClusterAllocationFromRemote(t *testing.T) {
const clusterName = "remotecluster"
t.Parallel()
t.Run("Handle allocation request remotely", func(t *testing.T) {
c, m := newFakeController()
Expand All @@ -908,7 +909,6 @@ func TestMultiClusterAllocationFromRemote(t *testing.T) {
server.TLS.ClientAuth = tls.RequireAndVerifyClientCert

// Allocation policy reactor
clusterName := "remotecluster"
secretName := clusterName + "secret"
m.AgonesClient.AddReactor("list", "gameserverallocationpolicies", func(action k8stesting.Action) (bool, k8sruntime.Object, error) {
return true, &multiclusterv1alpha1.GameServerAllocationPolicyList{
Expand All @@ -918,9 +918,9 @@ func TestMultiClusterAllocationFromRemote(t *testing.T) {
Priority: 1,
Weight: 200,
ConnectionInfo: multiclusterv1alpha1.ClusterConnectionInfo{
AllocationEndpoint: server.URL,
ClusterName: clusterName,
SecretName: secretName,
AllocationEndpoints: []string{server.URL, "non-existing"},
ClusterName: clusterName,
SecretName: secretName,
},
},
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -983,7 +983,6 @@ func TestMultiClusterAllocationFromRemote(t *testing.T) {
server.TLS.ClientAuth = tls.RequireAndVerifyClientCert

// Allocation policy reactor
clusterName := "remotecluster"
secretName := clusterName + "secret"
m.AgonesClient.AddReactor("list", "gameserverallocationpolicies", func(action k8stesting.Action) (bool, k8sruntime.Object, error) {
return true, &multiclusterv1alpha1.GameServerAllocationPolicyList{
Expand All @@ -993,9 +992,9 @@ func TestMultiClusterAllocationFromRemote(t *testing.T) {
Priority: 1,
Weight: 200,
ConnectionInfo: multiclusterv1alpha1.ClusterConnectionInfo{
AllocationEndpoint: server.URL,
ClusterName: clusterName,
SecretName: secretName,
AllocationEndpoints: []string{server.URL},
ClusterName: clusterName,
SecretName: secretName,
},
},
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -1039,6 +1038,94 @@ func TestMultiClusterAllocationFromRemote(t *testing.T) {
assert.Error(t, err)
assert.Contains(t, err.Error(), "test error message")
})

t.Run("First server fails and second server succeeds", func(t *testing.T) {
c, m := newFakeController()
fleetName := addReactorForGameServer(&m)

// Mock server to return error
unhealthyServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
http.Error(w, "test error message", 500)
}))
defer unhealthyServer.Close()

// Set client CA for unhealthy server
certpool := x509.NewCertPool()
certpool.AppendCertsFromPEM(clientCert)
unhealthyServer.TLS.ClientCAs = certpool
unhealthyServer.TLS.ClientAuth = tls.RequireAndVerifyClientCert

// Mock healthyServer
expectedGSAName := "mocked"
healthyServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
serverResponse := v1alpha1.GameServerAllocation{
ObjectMeta: metav1.ObjectMeta{
Name: expectedGSAName,
},
}
response, _ := json.Marshal(serverResponse)
_, _ = w.Write(response)
}))
defer healthyServer.Close()
healthyServer.TLS = unhealthyServer.TLS

// Allocation policy reactor
secretName := clusterName + "secret"
m.AgonesClient.AddReactor("list", "gameserverallocationpolicies", func(action k8stesting.Action) (bool, k8sruntime.Object, error) {
return true, &multiclusterv1alpha1.GameServerAllocationPolicyList{
Items: []multiclusterv1alpha1.GameServerAllocationPolicy{
{
Spec: multiclusterv1alpha1.GameServerAllocationPolicySpec{
Priority: 1,
Weight: 200,
ConnectionInfo: multiclusterv1alpha1.ClusterConnectionInfo{
AllocationEndpoints: []string{unhealthyServer.URL, healthyServer.URL},
ClusterName: clusterName,
SecretName: secretName,
},
},
ObjectMeta: metav1.ObjectMeta{
Namespace: defaultNs,
},
},
},
}, nil
})

m.KubeClient.AddReactor("list", "secrets",
func(action k8stesting.Action) (bool, k8sruntime.Object, error) {
return true, getTestSecret(secretName, unhealthyServer.TLS.Certificates[0].Certificate[0]), nil
})

stop, cancel := agtesting.StartInformers(m, c.allocationPolicySynced, c.secretSynced, c.gameServerSynced)
defer cancel()

// This call initializes the cache
err := c.syncReadyGSServerCache()
assert.Nil(t, err)

err = c.counter.Run(0, stop)
assert.Nil(t, err)

gsa := &v1alpha1.GameServerAllocation{
ObjectMeta: metav1.ObjectMeta{
Namespace: defaultNs,
Name: "alloc1",
ClusterName: "localcluster",
},
Spec: v1alpha1.GameServerAllocationSpec{
MultiClusterSetting: v1alpha1.MultiClusterSetting{
Enabled: true,
},
Required: metav1.LabelSelector{MatchLabels: map[string]string{stablev1alpha1.FleetNameLabel: fleetName}},
},
}

result, err := executeAllocation(gsa, c)
if assert.NoError(t, err) {
assert.Equal(t, expectedGSAName, result.ObjectMeta.Name)
}
})
}

func TestCreateRestClientError(t *testing.T) {
Expand Down
18 changes: 9 additions & 9 deletions test/e2e/gameserverallocation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,9 @@ func TestMultiClusterAllocationOnLocalCluster(t *testing.T) {
Priority: 1,
Weight: 100,
ConnectionInfo: multiclusterv1alpha1.ClusterConnectionInfo{
AllocationEndpoint: "localhost",
ClusterName: "multicluster1",
SecretName: "sec1",
AllocationEndpoints: []string{"localhost"},
ilkercelikyilmaz marked this conversation as resolved.
Show resolved Hide resolved
ClusterName: "multicluster1",
SecretName: "sec1",
},
},
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -106,9 +106,9 @@ func TestMultiClusterAllocationOnLocalCluster(t *testing.T) {
Priority: 2,
Weight: 100,
ConnectionInfo: multiclusterv1alpha1.ClusterConnectionInfo{
AllocationEndpoint: "another-endpoint",
ClusterName: "multicluster2",
SecretName: "sec1",
AllocationEndpoints: []string{"another-endpoint"},
ClusterName: "multicluster2",
SecretName: "sec1",
},
},
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -128,9 +128,9 @@ func TestMultiClusterAllocationOnLocalCluster(t *testing.T) {
Priority: 1,
Weight: 10,
ConnectionInfo: multiclusterv1alpha1.ClusterConnectionInfo{
AllocationEndpoint: "another-endpoint",
ClusterName: "multicluster3",
SecretName: "sec1",
AllocationEndpoints: []string{"another-endpoint"},
ClusterName: "multicluster3",
SecretName: "sec1",
},
},
ObjectMeta: metav1.ObjectMeta{
Expand Down