Change allocator gRPC response state to gRPC error status (googleforg…

…ames#1516)

Co-authored-by: Mark Mandel <markmandel@google.com>

cmd/allocator/main.go

@@ -277,8 +277,8 @@ func (h *serviceHandler) Allocate(ctx context.Context, in *pb.AllocationRequest)
 		logger.Errorf("internal server error - Bad GSA format %v", resultObj)
 		return nil, status.Errorf(codes.Internal, "internal server error- Bad GSA format %v", resultObj)
 	}
-	response := converters.ConvertGSAV1ToAllocationResponseV1Alpha1(allocatedGsa)
-	logger.WithField("response", response).Infof("allocation response is being sent")
+	response, err := converters.ConvertGSAV1ToAllocationResponseV1Alpha1(allocatedGsa)
+	logger.WithField("response", response).WithError(err).Infof("allocation response is being sent")
 
-	return response, nil
+	return response, err
 }

cmd/allocator/main_test.go

@@ -55,10 +55,14 @@ func TestAllocateHandler(t *testing.T) {
 	}
 
 	response, err := h.Allocate(context.Background(), request)
-	if !assert.NoError(t, err) {
+	if !assert.Nil(t, response) {
 		return
 	}
-	assert.Equal(t, pb.AllocationResponse_Contention, response.State)
+	st, ok := status.FromError(err)
+	if !assert.True(t, ok) {
+		return
+	}
+	assert.Equal(t, st.Code(), codes.Aborted)
 }
 
 func TestAllocateHandlerReturnsError(t *testing.T) {

pkg/allocation/converters/converter.go

@@ -20,6 +20,8 @@ import (
 	"agones.dev/agones/pkg/apis"
 	agonesv1 "agones.dev/agones/pkg/apis/agones/v1"
 	allocationv1 "agones.dev/agones/pkg/apis/allocation/v1"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -145,18 +147,21 @@ func convertLabelSelectorsToInternalLabelSelectors(in []*pb.LabelSelector) []met
 }
 
 // ConvertGSAV1ToAllocationResponseV1Alpha1 converts GameServerAllocation V1 (GSA) to AllocationResponse
-func ConvertGSAV1ToAllocationResponseV1Alpha1(in *allocationv1.GameServerAllocation) *pb.AllocationResponse {
+func ConvertGSAV1ToAllocationResponseV1Alpha1(in *allocationv1.GameServerAllocation) (*pb.AllocationResponse, error) {
 	if in == nil {
-		return nil
+		return nil, nil
+	}
+
+	if err := convertStateV1ToError(in.Status.State); err != nil {
+		return nil, err
 	}
 
 	return &pb.AllocationResponse{
-		State:          convertStateV1ToAllocationStateV1Alpha1(in.Status.State),
 		GameServerName: in.Status.GameServerName,
 		Address:        in.Status.Address,
 		NodeName:       in.Status.NodeName,
 		Ports:          convertAgonesPortsV1ToAllocationPortsV1Alpha1(in.Status.Ports),
-	}
+	}, nil
 }
 
 // ConvertAllocationResponseV1Alpha1ToGSAV1 converts AllocationResponse to GameServerAllocation V1 (GSA)
@@ -167,15 +172,14 @@ func ConvertAllocationResponseV1Alpha1ToGSAV1(in *pb.AllocationResponse) *alloca
 
 	out := &allocationv1.GameServerAllocation{
 		Status: allocationv1.GameServerAllocationStatus{
+			State:          allocationv1.GameServerAllocationAllocated,
 			GameServerName: in.GameServerName,
 			Address:        in.Address,
 			NodeName:       in.NodeName,
 			Ports:          convertAllocationPortsV1Alpha1ToAgonesPortsV1(in.Ports),
 		},
 	}
-	if state, isMatch := convertAllocationStateV1Alpha1ToStateV1(in.State); isMatch {
-		out.Status.State = state
-	}
+
 	return out
 }
 
@@ -206,28 +210,14 @@ func convertAllocationPortsV1Alpha1ToAgonesPortsV1(in []*pb.AllocationResponse_G
 }
 
 // convertStateV1ToAllocationStateV1Alpha1 converts GameServerAllocationState V1 (GSA) to AllocationResponse_GameServerAllocationState
-func convertStateV1ToAllocationStateV1Alpha1(in allocationv1.GameServerAllocationState) pb.AllocationResponse_GameServerAllocationState {
+func convertStateV1ToError(in allocationv1.GameServerAllocationState) error {
 	switch in {
 	case allocationv1.GameServerAllocationAllocated:
-		return pb.AllocationResponse_Allocated
+		return nil
 	case allocationv1.GameServerAllocationUnAllocated:
-		return pb.AllocationResponse_UnAllocated
+		return status.Error(codes.ResourceExhausted, "there is no available GameServer to allocate")
 	case allocationv1.GameServerAllocationContention:
-		return pb.AllocationResponse_Contention
+		return status.Error(codes.Aborted, "too many concurrent requests have overwhelmed the system")
 	}
-	return pb.AllocationResponse_Unknown
-}
-
-// convertAllocationStateV1Alpha1ToStateV1 converts AllocationResponse_GameServerAllocationState to GameServerAllocationState V1 (GSA)
-// if the conversion cannot happen, it returns false.
-func convertAllocationStateV1Alpha1ToStateV1(in pb.AllocationResponse_GameServerAllocationState) (allocationv1.GameServerAllocationState, bool) {
-	switch in {
-	case pb.AllocationResponse_Allocated:
-		return allocationv1.GameServerAllocationAllocated, true
-	case pb.AllocationResponse_UnAllocated:
-		return allocationv1.GameServerAllocationUnAllocated, true
-	case pb.AllocationResponse_Contention:
-		return allocationv1.GameServerAllocationContention, true
-	}
-	return allocationv1.GameServerAllocationUnAllocated, false
+	return status.Error(codes.Unknown, "unknown issue")
 }

pkg/allocation/converters/converter_test.go

@@ -22,6 +22,8 @@ import (
 	agonesv1 "agones.dev/agones/pkg/apis/agones/v1"
 	allocationv1 "agones.dev/agones/pkg/apis/allocation/v1"
 	"github.com/stretchr/testify/assert"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -223,13 +225,14 @@ func TestConvertGSAV1ToAllocationResponseV1Alpha1(t *testing.T) {
 		name             string
 		in               *allocationv1.GameServerAllocation
 		want             *pb.AllocationResponse
+		wantErrCode      codes.Code
 		skipConvertToGSA bool
 	}{
 		{
-			name: "status field is set",
+			name: "status state is set to allocated",
 			in: &allocationv1.GameServerAllocation{
 				Status: allocationv1.GameServerAllocationStatus{
-					State:          allocationv1.GameServerAllocationUnAllocated,
+					State:          allocationv1.GameServerAllocationAllocated,
 					GameServerName: "GSN",
 					Ports: []agonesv1.GameServerStatusPort{
 						{
@@ -244,7 +247,6 @@ func TestConvertGSAV1ToAllocationResponseV1Alpha1(t *testing.T) {
 				},
 			},
 			want: &pb.AllocationResponse{
-				State:          pb.AllocationResponse_UnAllocated,
 				GameServerName: "GSN",
 				Address:        "address",
 				NodeName:       "node-name",
@@ -259,15 +261,25 @@ func TestConvertGSAV1ToAllocationResponseV1Alpha1(t *testing.T) {
 			},
 		},
 		{
-			name: "status state is set to allocated",
+			name: "status field is set to unallocated",
 			in: &allocationv1.GameServerAllocation{
 				Status: allocationv1.GameServerAllocationStatus{
-					State: allocationv1.GameServerAllocationAllocated,
+					State:          allocationv1.GameServerAllocationUnAllocated,
+					GameServerName: "GSN",
+					Ports: []agonesv1.GameServerStatusPort{
+						{
+							Port: 123,
+						},
+						{
+							Name: "port-name",
+						},
+					},
+					Address:  "address",
+					NodeName: "node-name",
 				},
 			},
-			want: &pb.AllocationResponse{
-				State: pb.AllocationResponse_Allocated,
-			},
+			wantErrCode:      codes.ResourceExhausted,
+			skipConvertToGSA: true,
 		},
 		{
 			name: "status state is set to contention",
@@ -276,9 +288,8 @@ func TestConvertGSAV1ToAllocationResponseV1Alpha1(t *testing.T) {
 					State: allocationv1.GameServerAllocationContention,
 				},
 			},
-			want: &pb.AllocationResponse{
-				State: pb.AllocationResponse_Contention,
-			},
+			wantErrCode:      codes.Aborted,
+			skipConvertToGSA: true,
 		},
 		{
 			name: "Empty fields",
@@ -287,14 +298,16 @@ func TestConvertGSAV1ToAllocationResponseV1Alpha1(t *testing.T) {
 					Ports: []agonesv1.GameServerStatusPort{},
 				},
 			},
-			want: &pb.AllocationResponse{
-				State: pb.AllocationResponse_Unknown,
-			},
+			wantErrCode:      codes.Unknown,
 			skipConvertToGSA: true,
 		},
 		{
 			name: "Empty objects",
-			in:   &allocationv1.GameServerAllocation{},
+			in: &allocationv1.GameServerAllocation{
+				Status: allocationv1.GameServerAllocationStatus{
+					State: allocationv1.GameServerAllocationAllocated,
+				},
+			},
 			want: &pb.AllocationResponse{},
 		},
 		{
@@ -308,7 +321,14 @@ func TestConvertGSAV1ToAllocationResponseV1Alpha1(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
 
-			out := ConvertGSAV1ToAllocationResponseV1Alpha1(tc.in)
+			out, err := ConvertGSAV1ToAllocationResponseV1Alpha1(tc.in)
+			if tc.wantErrCode != 0 {
+				st, ok := status.FromError(err)
+				if !assert.True(t, ok) {
+					return
+				}
+				assert.Equal(t, tc.wantErrCode, st.Code())
+			}
 			if !assert.Equal(t, tc.want, out) {
 				t.Errorf("mismatch with want after conversion: \"%s\"", tc.name)
 			}
@@ -332,11 +352,12 @@ func TestConvertAllocationResponseV1Alpha1ToGSAV1(t *testing.T) {
 		{
 			name: "Empty fields",
 			in: &pb.AllocationResponse{
-				State: pb.AllocationResponse_Unknown,
 				Ports: []*pb.AllocationResponse_GameServerStatusPort{},
 			},
 			want: &allocationv1.GameServerAllocation{
-				Status: allocationv1.GameServerAllocationStatus{},
+				Status: allocationv1.GameServerAllocationStatus{
+					State: allocationv1.GameServerAllocationAllocated,
+				},
 			},
 		},
 	}