fix: avoid gs status sync delay when network failure (#45)

Signed-off-by: ChrisLiu <[email protected]>

cloudprovider/utils/network_manager.go

@@ -47,11 +47,11 @@ func (nm *NetworkManager) SetNetworkState(disabled bool) error {
 	}
 
 	// Initial annotations if necessary
-	if patchPod.Annotations == nil {
-		patchPod.Annotations = make(map[string]string)
+	if patchPod.Labels == nil {
+		patchPod.Labels = make(map[string]string)
 	}
 
-	patchPod.Annotations[v1alpha1.GameServerNetworkDisabled] = strconv.FormatBool(disabled)
+	patchPod.Labels[v1alpha1.GameServerNetworkDisabled] = strconv.FormatBool(disabled)
 	patch := client.MergeFrom(patchPod)
 	return nm.client.Patch(context.Background(), nm.pod, patch, nil)
 }
@@ -126,7 +126,7 @@ func NewNetworkManager(pod *corev1.Pod, client client.Client) *NetworkManager {
 	}
 
 	var networkDisabled bool
-	if networkDisabledStr, ok := pod.Annotations[v1alpha1.GameServerNetworkDisabled]; ok {
+	if networkDisabledStr, ok := pod.Labels[v1alpha1.GameServerNetworkDisabled]; ok {
 		networkDisabled, err = strconv.ParseBool(networkDisabledStr)
 		if err != nil {
 			log.Warningf("Pod %s has invalid network disabled option, err: %s", pod.Name, err.Error())

pkg/controllers/gameserver/gameserver_controller.go

@@ -202,8 +202,8 @@ func (r *GameServerReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		return reconcile.Result{}, err
 	}
 
-	podUpdated, err := gsm.SyncGsToPod()
-	if err != nil || podUpdated {
+	err = gsm.SyncGsToPod()
+	if err != nil {
 		return reconcile.Result{RequeueAfter: 3 * time.Second}, err
 	}
 

pkg/controllers/gameserver/gameserver_manager.go

@@ -48,9 +48,8 @@ const (
 
 type Control interface {
 	// SyncGsToPod compares the pod with GameServer, and decide whether to update the pod based on the results.
-	// When the fields of the pod is different from that of GameServer,
-	// pod will be updated and true is returned; otherwise, the pod won't be updated and false is returned.
-	SyncGsToPod() (bool, error)
+	// When the fields of the pod is different from that of GameServer, pod will be updated.
+	SyncGsToPod() error
 	// SyncPodToGs compares the GameServer with pod, and update the GameServer.
 	SyncPodToGs(*gameKruiseV1alpha1.GameServerSet) error
 	// WaitOrNot compare the current game server network status to decide whether to re-queue.
@@ -64,31 +63,29 @@ type GameServerManager struct {
 	eventRecorder record.EventRecorder
 }
 
-func (manager GameServerManager) SyncGsToPod() (bool, error) {
+func (manager GameServerManager) SyncGsToPod() error {
 	pod := manager.pod
 	gs := manager.gameServer
 	podLabels := pod.GetLabels()
 	podDeletePriority := podLabels[gameKruiseV1alpha1.GameServerDeletePriorityKey]
 	podUpdatePriority := podLabels[gameKruiseV1alpha1.GameServerUpdatePriorityKey]
 	podGsOpsState := podLabels[gameKruiseV1alpha1.GameServerOpsStateKey]
 	podGsState := podLabels[gameKruiseV1alpha1.GameServerStateKey]
+	podNetworkDisabled := podLabels[gameKruiseV1alpha1.GameServerNetworkDisabled]
 
-	updated := false
 	newLabels := make(map[string]string)
 	newAnnotations := make(map[string]string)
 	if gs.Spec.DeletionPriority.String() != podDeletePriority {
 		newLabels[gameKruiseV1alpha1.GameServerDeletePriorityKey] = gs.Spec.DeletionPriority.String()
 		if podDeletePriority != "" {
 			manager.eventRecorder.Eventf(gs, corev1.EventTypeNormal, StateReason, "DeletionPriority turn from %s to %s ", podDeletePriority, gs.Spec.DeletionPriority.String())
 		}
-		updated = true
 	}
 	if gs.Spec.UpdatePriority.String() != podUpdatePriority {
 		newLabels[gameKruiseV1alpha1.GameServerUpdatePriorityKey] = gs.Spec.UpdatePriority.String()
 		if podUpdatePriority != "" {
 			manager.eventRecorder.Eventf(gs, corev1.EventTypeNormal, StateReason, "UpdatePriority turn from %s to %s ", podUpdatePriority, gs.Spec.UpdatePriority.String())
 		}
-		updated = true
 	}
 	if string(gs.Spec.OpsState) != podGsOpsState {
 		newLabels[gameKruiseV1alpha1.GameServerOpsStateKey] = string(gs.Spec.OpsState)
@@ -99,7 +96,12 @@ func (manager GameServerManager) SyncGsToPod() (bool, error) {
 			}
 			manager.eventRecorder.Eventf(gs, eventType, StateReason, "OpsState turn from %s to %s ", podGsOpsState, string(gs.Spec.OpsState))
 		}
-		updated = true
+	}
+	if podNetworkDisabled != strconv.FormatBool(gs.Spec.NetworkDisabled) {
+		newLabels[gameKruiseV1alpha1.GameServerNetworkDisabled] = strconv.FormatBool(gs.Spec.NetworkDisabled)
+		if podNetworkDisabled != "" {
+			manager.eventRecorder.Eventf(gs, corev1.EventTypeNormal, StateReason, "NetworkDisabled turn from %s to %s ", podNetworkDisabled, strconv.FormatBool(gs.Spec.NetworkDisabled))
+		}
 	}
 
 	var gsState gameKruiseV1alpha1.GameServerState
@@ -142,36 +144,29 @@ func (manager GameServerManager) SyncGsToPod() (bool, error) {
 			}
 			manager.eventRecorder.Eventf(gs, eventType, StateReason, "State turn from %s to %s ", podGsState, string(gsState))
 		}
-		updated = true
 	}
 
 	if gsState == gameKruiseV1alpha1.Ready && pod.Annotations[gameKruiseV1alpha1.GameServerNetworkType] != "" {
-		if pod.Annotations[gameKruiseV1alpha1.GameServerNetworkDisabled] != strconv.FormatBool(gs.Spec.NetworkDisabled) {
-			newAnnotations[gameKruiseV1alpha1.GameServerNetworkDisabled] = strconv.FormatBool(gs.Spec.NetworkDisabled)
-			updated = true
-		}
-
 		oldTime, err := time.Parse(TimeFormat, pod.Annotations[gameKruiseV1alpha1.GameServerNetworkTriggerTime])
-		if (err == nil && time.Since(oldTime) > NetworkIntervalTime) || (pod.Annotations[gameKruiseV1alpha1.GameServerNetworkTriggerTime] == "") {
+		if (err == nil && time.Since(oldTime) > NetworkIntervalTime && time.Since(gs.Status.NetworkStatus.LastTransitionTime.Time) < NetworkTotalWaitTime) || (pod.Annotations[gameKruiseV1alpha1.GameServerNetworkTriggerTime] == "") {
 			newAnnotations[gameKruiseV1alpha1.GameServerNetworkTriggerTime] = time.Now().Format(TimeFormat)
-			updated = true
 		}
 	}
 
-	if updated {
+	if len(newLabels) != 0 || len(newAnnotations) != 0 {
 		patchPod := map[string]interface{}{"metadata": map[string]map[string]string{"labels": newLabels, "annotations": newAnnotations}}
 		patchPodBytes, err := json.Marshal(patchPod)
 		if err != nil {
-			return updated, err
+			return err
 		}
 		err = manager.client.Patch(context.TODO(), pod, client.RawPatch(types.StrategicMergePatchType, patchPodBytes))
 		if err != nil && !errors.IsNotFound(err) {
 			klog.Errorf("failed to patch Pod %s in %s,because of %s.", pod.GetName(), pod.GetNamespace(), err.Error())
-			return updated, err
+			return err
 		}
 	}
 
-	return updated, nil
+	return nil
 }
 
 func (manager GameServerManager) SyncPodToGs(gss *gameKruiseV1alpha1.GameServerSet) error {

pkg/controllers/gameserver/gameserver_manager_test.go

@@ -1,18 +1,21 @@
 package gameserver
 
 import (
+	"context"
 	kruiseV1alpha1 "github.com/openkruise/kruise-api/apps/v1alpha1"
 	kruiseV1beta1 "github.com/openkruise/kruise-api/apps/v1beta1"
 	gameKruiseV1alpha1 "github.com/openkruise/kruise-game/apis/v1alpha1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
 	"reflect"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"strconv"
 	"testing"
 )
 
@@ -302,9 +305,8 @@ func TestSyncGsToPod(t *testing.T) {
 	up := intstr.FromInt(20)
 	dp := intstr.FromInt(10)
 	tests := []struct {
-		gs       *gameKruiseV1alpha1.GameServer
-		pod      *corev1.Pod
-		isUpdate bool
+		gs  *gameKruiseV1alpha1.GameServer
+		pod *corev1.Pod
 	}{
 		{
 			gs: &gameKruiseV1alpha1.GameServer{
@@ -330,7 +332,6 @@ func TestSyncGsToPod(t *testing.T) {
 					Phase: corev1.PodPending,
 				},
 			},
-			isUpdate: true,
 		},
 
 		{
@@ -366,7 +367,6 @@ func TestSyncGsToPod(t *testing.T) {
 					Phase: corev1.PodPending,
 				},
 			},
-			isUpdate: false,
 		},
 	}
 
@@ -379,13 +379,32 @@ func TestSyncGsToPod(t *testing.T) {
 			pod:        test.pod,
 		}
 
-		isUpdate, err := manager.SyncGsToPod()
-		if err != nil {
+		if err := manager.SyncGsToPod(); err != nil {
 			t.Error(err)
 		}
 
-		if isUpdate != test.isUpdate {
-			t.Errorf("expect isUpdate is %v ,but actually is %v", test.isUpdate, isUpdate)
+		pod := &corev1.Pod{}
+		if err := manager.client.Get(context.TODO(), types.NamespacedName{
+			Namespace: test.gs.Namespace,
+			Name:      test.gs.Name,
+		}, pod); err != nil {
+			t.Error(err)
+		}
+
+		if pod.Labels[gameKruiseV1alpha1.GameServerOpsStateKey] != string(test.gs.Spec.OpsState) {
+			t.Errorf("expect opsState is %s ,but actually is %s", string(test.gs.Spec.OpsState), pod.Labels[gameKruiseV1alpha1.GameServerOpsStateKey])
+		}
+
+		if pod.Labels[gameKruiseV1alpha1.GameServerUpdatePriorityKey] != test.gs.Spec.UpdatePriority.String() {
+			t.Errorf("expect UpdatePriority is %s ,but actually is %s", test.gs.Spec.UpdatePriority.String(), pod.Labels[gameKruiseV1alpha1.GameServerUpdatePriorityKey])
+		}
+
+		if pod.Labels[gameKruiseV1alpha1.GameServerDeletePriorityKey] != test.gs.Spec.DeletionPriority.String() {
+			t.Errorf("expect DeletionPriority is %s ,but actually is %s", test.gs.Spec.DeletionPriority.String(), pod.Labels[gameKruiseV1alpha1.GameServerDeletePriorityKey])
+		}
+
+		if pod.Labels[gameKruiseV1alpha1.GameServerNetworkDisabled] != strconv.FormatBool(test.gs.Spec.NetworkDisabled) {
+			t.Errorf("expect NetworkDisabled is %s ,but actually is %s", strconv.FormatBool(test.gs.Spec.NetworkDisabled), pod.Labels[gameKruiseV1alpha1.GameServerNetworkDisabled])
 		}
 	}
 }