[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6357: Add t3c retrying app lock

[GitBox <gi...@apache.org>]

ocket8888 commented on a change in pull request #6357:
URL: https://github.com/apache/trafficcontrol/pull/6357#discussion_r752429233



##########
File path: cache-config/testing/ort-tests/t3c-lockfile_test.go
##########
@@ -0,0 +1,113 @@
+package orttest
+
+/*
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+   http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+import (
+	"strings"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/apache/trafficcontrol/cache-config/testing/ort-tests/tcdata"
+)
+
+func TestLockfile(t *testing.T) {
+	testName := "TestLockfile"
+	t.Logf("------------- Starting " + testName + " ---------------")
+	tcd.WithObjs(t, []tcdata.TCObj{
+		tcdata.CDNs, tcdata.Types, tcdata.Tenants, tcdata.Parameters,
+		tcdata.Profiles, tcdata.ProfileParameters, tcdata.Statuses,
+		tcdata.Divisions, tcdata.Regions, tcdata.PhysLocations,
+		tcdata.CacheGroups, tcdata.Servers, tcdata.Topologies,
+		tcdata.DeliveryServices}, func() {
+
+		hostName := "atlanta-edge-03"
+		const fileName = `records.config`
+
+		firstOut := ""
+		firstCode := 0
+		wg := sync.WaitGroup{}
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			t.Logf("TestLockFile first t3c starting %v", time.Now())
+			firstOut, firstCode = t3cUpdateReload(hostName, "badass")
+			t.Logf("TestLockFile first t3c finished %v", time.Now())
+		}()
+
+		time.Sleep(time.Millisecond * 100) // sleep long enough to ensure the concurrent t3c starts
+		t.Logf("TestLockFile second t3c starting %v", time.Now())
+		out, code := t3cUpdateReload(hostName, "badass")
+		t.Logf("TestLockFile second t3c finished %v", time.Now())
+		if code != 0 {
+			t.Fatalf("second t3c apply badass failed: output '''%v''' code %v\n", out, code)
+		}
+
+		wg.Wait()
+		if firstCode != 0 {
+			t.Fatalf("first t3c apply badass failed: output '''%v''' code %v\n", firstOut, firstCode)
+		}
+
+		outStr := string(out)
+		outLines := strings.Split(outStr, "\n")
+		acquireStartLine := ""
+		acquireEndLine := ""
+		for _, line := range outLines {
+			if strings.Contains(line, "Trying to acquire app lock") {
+				acquireStartLine = line
+			} else if strings.Contains(line, "Acquired app lock") {
+				acquireEndLine = line
+			}
+			if acquireStartLine != "" && acquireEndLine != "" {
+				break
+			}
+		}
+
+		if acquireStartLine == "" || acquireEndLine == "" {
+			t.Fatalf("t3c apply output expected to contain 'Trying to acquire app lock' and 'Acquired app lock', actual: '''%v'''", out)
+		}
+
+		acquireStart := parseLogLineTime(acquireStartLine)
+		if acquireStart == nil {
+			t.Fatalf("t3c apply acquire line failed to parse time, line '" + acquireStartLine + "'")
+		}
+		acquireEnd := parseLogLineTime(acquireEndLine)
+		if acquireEnd == nil {
+			t.Fatalf("t3c apply acquire line failed to parse time, line '" + acquireEndLine + "'")
+		}
+
+		minDiff := time.Second * 1 // checking the file lock should never take 1s, so that's enough to verify it was hit
+		if diff := acquireEnd.Sub(*acquireStart); diff < minDiff {
+			t.Fatalf("t3c apply expected time to acquire while another t3c is running to be at least %v, actual %v start line '%v' end line '%v'", minDiff, diff, acquireStartLine, acquireEndLine)
+		}
+
+		t.Logf(testName + " succeeded")

Review comment:
       Is this necessary? The output of `go test` informs you whether the test succeeded or failed

##########
File path: cache-config/testing/ort-tests/t3c-lockfile_test.go
##########
@@ -0,0 +1,113 @@
+package orttest
+
+/*
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+   http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+import (
+	"strings"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/apache/trafficcontrol/cache-config/testing/ort-tests/tcdata"
+)
+
+func TestLockfile(t *testing.T) {
+	testName := "TestLockfile"
+	t.Logf("------------- Starting " + testName + " ---------------")
+	tcd.WithObjs(t, []tcdata.TCObj{
+		tcdata.CDNs, tcdata.Types, tcdata.Tenants, tcdata.Parameters,
+		tcdata.Profiles, tcdata.ProfileParameters, tcdata.Statuses,
+		tcdata.Divisions, tcdata.Regions, tcdata.PhysLocations,
+		tcdata.CacheGroups, tcdata.Servers, tcdata.Topologies,
+		tcdata.DeliveryServices}, func() {
+
+		hostName := "atlanta-edge-03"
+		const fileName = `records.config`
+
+		firstOut := ""
+		firstCode := 0
+		wg := sync.WaitGroup{}
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			t.Logf("TestLockFile first t3c starting %v", time.Now())
+			firstOut, firstCode = t3cUpdateReload(hostName, "badass")
+			t.Logf("TestLockFile first t3c finished %v", time.Now())
+		}()
+
+		time.Sleep(time.Millisecond * 100) // sleep long enough to ensure the concurrent t3c starts
+		t.Logf("TestLockFile second t3c starting %v", time.Now())
+		out, code := t3cUpdateReload(hostName, "badass")
+		t.Logf("TestLockFile second t3c finished %v", time.Now())
+		if code != 0 {
+			t.Fatalf("second t3c apply badass failed: output '''%v''' code %v\n", out, code)

Review comment:
       I have two nits here:
   - you don't need the `\n`, the logging makes sure it ends with a newline character for you
   - using `%v` instead of a more specific formatting parameter for primitive types/`fmt.Stringer`s makes it harder to catch obscure issues like printing a memory address instead of the value stored there, so I encourage people to be more specific when possible. Not that you're printing pointer values on this particular line, or anything. Just a good practice IMO.

##########
File path: cache-config/testing/ort-tests/t3c-lockfile_test.go
##########
@@ -0,0 +1,113 @@
+package orttest
+
+/*
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+   http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+import (
+	"strings"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/apache/trafficcontrol/cache-config/testing/ort-tests/tcdata"
+)
+
+func TestLockfile(t *testing.T) {
+	testName := "TestLockfile"
+	t.Logf("------------- Starting " + testName + " ---------------")
+	tcd.WithObjs(t, []tcdata.TCObj{
+		tcdata.CDNs, tcdata.Types, tcdata.Tenants, tcdata.Parameters,
+		tcdata.Profiles, tcdata.ProfileParameters, tcdata.Statuses,
+		tcdata.Divisions, tcdata.Regions, tcdata.PhysLocations,
+		tcdata.CacheGroups, tcdata.Servers, tcdata.Topologies,
+		tcdata.DeliveryServices}, func() {
+
+		hostName := "atlanta-edge-03"
+		const fileName = `records.config`
+
+		firstOut := ""
+		firstCode := 0
+		wg := sync.WaitGroup{}
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			t.Logf("TestLockFile first t3c starting %v", time.Now())
+			firstOut, firstCode = t3cUpdateReload(hostName, "badass")
+			t.Logf("TestLockFile first t3c finished %v", time.Now())
+		}()
+
+		time.Sleep(time.Millisecond * 100) // sleep long enough to ensure the concurrent t3c starts
+		t.Logf("TestLockFile second t3c starting %v", time.Now())
+		out, code := t3cUpdateReload(hostName, "badass")
+		t.Logf("TestLockFile second t3c finished %v", time.Now())

Review comment:
       Should timing stuff be in a benchmark?

##########
File path: cache-config/testing/ort-tests/t3c-lockfile_test.go
##########
@@ -0,0 +1,113 @@
+package orttest
+
+/*
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+   http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+import (
+	"strings"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/apache/trafficcontrol/cache-config/testing/ort-tests/tcdata"
+)
+
+func TestLockfile(t *testing.T) {
+	testName := "TestLockfile"
+	t.Logf("------------- Starting " + testName + " ---------------")

Review comment:
       Is this (and the corresponding ending line) necessary? `go test` prints the name of the test when that test starts when you use `-v` or when the test fails (which are the only times you'll see `testing.T.Log` output anyway).

##########
File path: cache-config/testing/ort-tests/t3c-lockfile_test.go
##########
@@ -0,0 +1,113 @@
+package orttest
+
+/*
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+   http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+import (
+	"strings"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/apache/trafficcontrol/cache-config/testing/ort-tests/tcdata"
+)
+
+func TestLockfile(t *testing.T) {
+	testName := "TestLockfile"
+	t.Logf("------------- Starting " + testName + " ---------------")
+	tcd.WithObjs(t, []tcdata.TCObj{
+		tcdata.CDNs, tcdata.Types, tcdata.Tenants, tcdata.Parameters,
+		tcdata.Profiles, tcdata.ProfileParameters, tcdata.Statuses,
+		tcdata.Divisions, tcdata.Regions, tcdata.PhysLocations,
+		tcdata.CacheGroups, tcdata.Servers, tcdata.Topologies,
+		tcdata.DeliveryServices}, func() {
+
+		hostName := "atlanta-edge-03"
+		const fileName = `records.config`

Review comment:
       why is one of these `const` but not the other? The both look like they aren't modified.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org