Skip to content

Commit

Permalink
Fix for data race in heartBeatTimeChecker tests (#354)
Browse files Browse the repository at this point in the history 
* Fix for data race in heartBeatTimeChecker tests

* some comments
  • Loading branch information
@javier-op
javier-op committed Jul 29, 2022
1 parent 88b325c commit 5ab9198
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 21 deletions.
2 changes: 1 addition & 1 deletion cmd/nodeagent/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func main() {
log.Fatal(err)
}

n := NewNodeAgentManager(dynamicClient, nodeName, logEveryHeartbeat, ignoreHealthFromHeartbeat, time.Now)
n := NewNodeAgentManager(dynamicClient, nodeName, logEveryHeartbeat, ignoreHealthFromHeartbeat, time.Now, true)
log.Debug("Starting HTTP server")
http.HandleFunc("/v1/sessionHosts/", n.heartbeatHandler)
http.HandleFunc("/healthz", healthzHandler)
Expand Down
10 changes: 6 additions & 4 deletions cmd/nodeagent/nodeagentmanager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ type NodeAgentManager struct {
firstHeartbeatTimeout int64 // the first heartbeat gets a longer window considering initialization time
}

func NewNodeAgentManager(dynamicClient dynamic.Interface, nodeName string, logEveryHeartbeat bool, ignoreHealthFromHeartbeat bool, now func() time.Time) *NodeAgentManager {
func NewNodeAgentManager(dynamicClient dynamic.Interface, nodeName string, logEveryHeartbeat bool, ignoreHealthFromHeartbeat bool, now func() time.Time, withHeartbeatTimeChecker bool) *NodeAgentManager {
n := &NodeAgentManager{
dynamicClient: dynamicClient,
watchStopper: make(chan struct{}),
Expand All @@ -72,7 +72,11 @@ func NewNodeAgentManager(dynamicClient dynamic.Interface, nodeName string, logEv
nowFunc: now,
}
n.runWatch()
n.runHeartbeatTimeCheckerLoop()
n.firstHeartbeatTimeout = ParseInt64FromEnv("FIRST_HEARTBEAT_TIMEOUT", 60000)
n.heartbeatTimeout = ParseInt64FromEnv("HEARTBEAT_TIMEOUT", 5000)
if withHeartbeatTimeChecker {
n.runHeartbeatTimeCheckerLoop()
}
return n
}

Expand All @@ -96,8 +100,6 @@ func (n *NodeAgentManager) runWatch() {

// runHeartbeatTimeCheckerLoop runs HeartbeatTimeChecker on an infinite loop
func (n *NodeAgentManager) runHeartbeatTimeCheckerLoop() {
n.firstHeartbeatTimeout = ParseInt64FromEnv("FIRST_HEARTBEAT_TIMEOUT", 60000)
n.heartbeatTimeout = ParseInt64FromEnv("HEARTBEAT_TIMEOUT", 5000)
go func() {
for {
n.HeartbeatTimeChecker()
Expand Down
41 changes: 25 additions & 16 deletions cmd/nodeagent/nodeagentmanager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ var _ = Describe("nodeagent tests", func() {
It("heartbeat with empty body should return error", func() {
req := httptest.NewRequest(http.MethodPost, "/v1/sessionHosts/sessionHostID", nil)
w := httptest.NewRecorder()
n := NewNodeAgentManager(newDynamicInterface(), testNodeName, false, false, time.Now)
n := NewNodeAgentManager(newDynamicInterface(), testNodeName, false, false, time.Now, true)
n.heartbeatHandler(w, req)
res := w.Result()
defer res.Body.Close()
Expand All @@ -54,7 +54,7 @@ var _ = Describe("nodeagent tests", func() {
b, _ := json.Marshal(hb)
req := httptest.NewRequest(http.MethodPost, "/v1/sessionHosts/sessionHostID", bytes.NewReader(b))
w := httptest.NewRecorder()
n := NewNodeAgentManager(newDynamicInterface(), testNodeName, false, false, time.Now)
n := NewNodeAgentManager(newDynamicInterface(), testNodeName, false, false, time.Now, true)
n.heartbeatHandler(w, req)
res := w.Result()
defer res.Body.Close()
Expand All @@ -72,7 +72,7 @@ var _ = Describe("nodeagent tests", func() {
w := httptest.NewRecorder()
dynamicClient := newDynamicInterface()

n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, time.Now)
n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, time.Now, true)
gs := createUnstructuredTestGameServer(testGameServerName, testGameServerNamespace)

_, err := dynamicClient.Resource(gameserverGVR).Namespace(testGameServerNamespace).Create(context.Background(), gs, metav1.CreateOptions{})
Expand All @@ -99,7 +99,7 @@ var _ = Describe("nodeagent tests", func() {
It("should transition properly from standingBy to Active", FlakeAttempts(numberOfAttemps), func() {
dynamicClient := newDynamicInterface()

n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, time.Now)
n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, time.Now, true)
gs := createUnstructuredTestGameServer(testGameServerName, testGameServerNamespace)

_, err := dynamicClient.Resource(gameserverGVR).Namespace(testGameServerNamespace).Create(context.Background(), gs, metav1.CreateOptions{})
Expand Down Expand Up @@ -177,7 +177,7 @@ var _ = Describe("nodeagent tests", func() {
It("should not create a GameServerDetail if the server is not Active", FlakeAttempts(numberOfAttemps), func() {
dynamicClient := newDynamicInterface()

n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, time.Now)
n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, time.Now, true)
gs := createUnstructuredTestGameServer(testGameServerName, testGameServerNamespace)

_, err := dynamicClient.Resource(gameserverGVR).Namespace(testGameServerNamespace).Create(context.Background(), gs, metav1.CreateOptions{})
Expand Down Expand Up @@ -219,7 +219,7 @@ var _ = Describe("nodeagent tests", func() {
It("should delete the GameServer from the cache when it's deleted from Kubernetes", FlakeAttempts(numberOfAttemps), func() {
dynamicClient := newDynamicInterface()

n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, time.Now)
n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, time.Now, true)
gs := createUnstructuredTestGameServer(testGameServerName, testGameServerNamespace)

_, err := dynamicClient.Resource(gameserverGVR).Namespace(testGameServerNamespace).Create(context.Background(), gs, metav1.CreateOptions{})
Expand All @@ -244,7 +244,7 @@ var _ = Describe("nodeagent tests", func() {
It("should create a GameServerDetail when the GameServer transitions to Active", FlakeAttempts(numberOfAttemps), func() {
dynamicClient := newDynamicInterface()

n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, time.Now)
n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, time.Now, true)
gs := createUnstructuredTestGameServer(testGameServerName, testGameServerNamespace)

_, err := dynamicClient.Resource(gameserverGVR).Namespace(testGameServerNamespace).Create(context.Background(), gs, metav1.CreateOptions{})
Expand Down Expand Up @@ -290,7 +290,7 @@ var _ = Describe("nodeagent tests", func() {
It("should not create a GameServerDetail when an Unhealthy GameServer transitions to Active", FlakeAttempts(numberOfAttemps), func() {
dynamicClient := newDynamicInterface()

n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, time.Now)
n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, time.Now, true)
gs := createUnstructuredTestGameServer(testGameServerName, testGameServerNamespace)

_, err := dynamicClient.Resource(gameserverGVR).Namespace(testGameServerNamespace).Create(context.Background(), gs, metav1.CreateOptions{})
Expand Down Expand Up @@ -337,7 +337,7 @@ var _ = Describe("nodeagent tests", func() {
It("should create a GameServerDetail on subsequent heartbeats, if it fails on the first time", FlakeAttempts(numberOfAttemps), func() {
dynamicClient := newDynamicInterface()

n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, time.Now)
n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, time.Now, true)
gs := createUnstructuredTestGameServer(testGameServerName, testGameServerNamespace)

_, err := dynamicClient.Resource(gameserverGVR).Namespace(testGameServerNamespace).Create(context.Background(), gs, metav1.CreateOptions{})
Expand Down Expand Up @@ -454,7 +454,7 @@ var _ = Describe("nodeagent tests", func() {

var wg sync.WaitGroup
dynamicClient := newDynamicInterface()
n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, time.Now)
n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, time.Now, true)
for i := 0; i < 100; i++ {
wg.Add(1)
go func(randomGameServerName string) {
Expand Down Expand Up @@ -551,7 +551,7 @@ var _ = Describe("nodeagent tests", func() {
It("should set CreationTime value when registering a new game server", FlakeAttempts(numberOfAttemps), func() {
dynamicClient := newDynamicInterface()

n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, time.Now)
n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, time.Now, true)
gs := createUnstructuredTestGameServer(testGameServerName, testGameServerNamespace)

_, err := dynamicClient.Resource(gameserverGVR).Namespace(testGameServerNamespace).Create(context.Background(), gs, metav1.CreateOptions{})
Expand All @@ -571,7 +571,7 @@ var _ = Describe("nodeagent tests", func() {
It("should set LastHeartbeatTime value when receiving a heartbeat from a game server", FlakeAttempts(numberOfAttemps), func() {
dynamicClient := newDynamicInterface()

n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, time.Now)
n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, time.Now, true)
gs := createUnstructuredTestGameServer(testGameServerName, testGameServerNamespace)

_, err := dynamicClient.Resource(gameserverGVR).Namespace(testGameServerNamespace).Create(context.Background(), gs, metav1.CreateOptions{})
Expand Down Expand Up @@ -617,7 +617,10 @@ var _ = Describe("nodeagent tests", func() {
value, _ := time.Parse(layout, "Tue, 26 Apr 2022 10:00:00 PST")
return value
}
n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, customNow)

// in this test we run HeartbeatTimeChecker manually, so we turn off the loop that does it
// automatically to avoid them running at the same time
n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, customNow, false)
gs := createUnstructuredTestGameServer(testGameServerName, testGameServerNamespace)

// create the game server
Expand Down Expand Up @@ -659,7 +662,10 @@ var _ = Describe("nodeagent tests", func() {
value, _ := time.Parse(layout, "Tue, 26 Apr 2022 10:00:00 PST")
return value
}
n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, customNow)

// in this test we run HeartbeatTimeChecker manually, so we turn off the loop that does it
// automatically to avoid them running at the same time
n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, customNow, false)
gs := createUnstructuredTestGameServer(testGameServerName, testGameServerNamespace)

// create the game server
Expand Down Expand Up @@ -727,7 +733,10 @@ var _ = Describe("nodeagent tests", func() {
value, _ := time.Parse(layout, "Tue, 26 Apr 2022 10:00:00 PST")
return value
}
n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, customNow)

// in this test we run HeartbeatTimeChecker manually, so we turn off the loop that does it
// automatically to avoid them running at the same time
n := NewNodeAgentManager(dynamicClient, testNodeName, false, false, customNow, false)
gs := createUnstructuredTestGameServer(testGameServerName, testGameServerNamespace)

// create the game server
Expand Down Expand Up @@ -809,7 +818,7 @@ var _ = Describe("nodeagent tests", func() {
// change time to be 6 seconds later again
customNow = func() time.Time {
layout := "Mon, 02 Jan 2006 15:04:05 MST"
value, _ := time.Parse(layout, "Tue, 26 Apr 2022 10:00:06 PST")
value, _ := time.Parse(layout, "Tue, 26 Apr 2022 10:00:12 PST")
return value
}
n.nowFunc = customNow
Expand Down

0 comments on commit 5ab9198

Please sign in to comment.