Merge pull request #5495 from onflow/khalil/6949-improve-network-test-ci

Fix Flakey Networking Tests and Improve CI

.github/workflows/ci.yml

@@ -89,7 +89,43 @@ jobs:
           cache: true
       - name: Set Test Matrix
         id: set-test-matrix
-        run: go run utils/test_matrix/test_matrix.go admin cmd consensus engine/access engine/collection engine/common engine/consensus engine/execution/ingestion:buildjet-8vcpu-ubuntu-2204 engine/execution/computation engine/execution engine/verification engine:buildjet-4vcpu-ubuntu-2204 fvm ledger module/dkg module:buildjet-4vcpu-ubuntu-2204 network/alsp network/test/cohort1:buildjet-16vcpu-ubuntu-2204 network/test/cohort2:buildjet-4vcpu-ubuntu-2204 network/p2p/connection network/p2p/node:buildjet-4vcpu-ubuntu-2204 network/p2p/scoring network/p2p network state storage utils
+        run: go run tools/test_matrix_generator/matrix.go
+
+  create-insecure-dynamic-test-matrix:
+    name: Create Dynamic Unit Test Insecure Package Matrix
+    runs-on: ubuntu-latest
+    outputs:
+      dynamic-matrix: ${{ steps.set-test-matrix.outputs.dynamicMatrix }}
+    steps:
+      - name: Checkout repo
+        uses: actions/checkout@v3
+      - name: Setup Go
+        uses: actions/setup-go@v4
+        timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time
+        with:
+          go-version: ${{ env.GO_VERSION }}
+          cache: true
+      - name: Set Test Matrix
+        id: set-test-matrix
+        run: go run tools/test_matrix_generator/matrix.go -c insecure
+
+  create-integration-dynamic-test-matrix:
+    name: Create Dynamic Integration Test Package Matrix
+    runs-on: ubuntu-latest
+    outputs:
+      dynamic-matrix: ${{ steps.set-test-matrix.outputs.dynamicMatrix }}
+    steps:
+      - name: Checkout repo
+        uses: actions/checkout@v3
+      - name: Setup Go
+        uses: actions/setup-go@v4
+        timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time
+        with:
+          go-version: ${{ env.GO_VERSION }}
+          cache: true
+      - name: Set Test Matrix
+        id: set-test-matrix
+        run: go run tools/test_matrix_generator/matrix.go -c integration
 
   unit-test:
     name: Unit Tests (${{ matrix.targets.name }})
@@ -127,23 +163,15 @@ jobs:
         flags: unittests
         name: codecov-umbrella
 
-  unit-test-modules:
-    name: Unit Tests (Modules)
+  unit-test-insecure:
+    name: Unit Tests Insecure (${{ matrix.targets.name }})
+    needs: create-insecure-dynamic-test-matrix
     strategy:
       fail-fast: false
       matrix:
-        include:
-          - name: insecure
-            setup: install-tools
-            retries: 5
-            race: 0
-            runner: buildjet-4vcpu-ubuntu-2204
-          - name: integration
-            setup: install-tools
-            retries: 5
-            race: 0
-            runner: buildjet-4vcpu-ubuntu-2204
-    runs-on: ${{ matrix.runner }}
+        targets: ${{ fromJSON(needs.create-insecure-dynamic-test-matrix.outputs.dynamic-matrix)}}
+    ## need to set image explicitly due to GitHub logging issue as described in https://github.com/onflow/flow-go/pull/3087#issuecomment-1234383202
+    runs-on: ${{ matrix.targets.runner }}
     steps:
       - name: Checkout repo
         uses: actions/checkout@v3
@@ -153,17 +181,17 @@ jobs:
         with:
           go-version: ${{ env.GO_VERSION }}
           cache: true
-      - name: Setup tests (${{ matrix.name }})
-        run: make ${{ matrix.setup }}
-      - name: Run tests (${{ matrix.name }})
-        env:
-          RACE_DETECTOR: ${{ matrix.race }}
+      - name: Setup tests (${{ matrix.targets.name }})
+        run: VERBOSE=1 make -e GO_TEST_PACKAGES="${{ matrix.targets.packages }}" install-tools
+      - name: Run tests (${{ matrix.targets.name }})
         uses: nick-fields/retry@v2
         with:
           timeout_minutes: 35
-          max_attempts: ${{ matrix.retries }}
-          # run test target inside each module's root
-          command: VERBOSE=1 make -C ${{ matrix.name }} test
+          max_attempts: 5
+          command: VERBOSE=1 make -C ./insecure -e GO_TEST_PACKAGES="${{ matrix.targets.packages }}" test
+        # TODO(rbtz): re-enable when we fix exisiting races.
+        #env:
+        #  RACE_DETECTOR: 1
       - name: Upload coverage report
         uses: codecov/codecov-action@v3
         with:
@@ -212,6 +240,42 @@ jobs:
           # use the workflow run id as part of the cache key to ensure these docker images will only be used for a single workflow run
           key: flow-docker-images-${{ hashFiles('**/Dockerfile') }}-${{ github.run_id }}
 
+  integration-test-others:
+    name: Integration Tests Others (${{ matrix.targets.name }})
+    needs: create-integration-dynamic-test-matrix
+    strategy:
+      fail-fast: false
+      matrix:
+        targets: ${{ fromJSON(needs.create-integration-dynamic-test-matrix.outputs.dynamic-matrix)}}
+    ## need to set image explicitly due to GitHub logging issue as described in https://github.com/onflow/flow-go/pull/3087#issuecomment-1234383202
+    runs-on: ${{ matrix.targets.runner }}
+    steps:
+      - name: Checkout repo
+        uses: actions/checkout@v3
+      - name: Setup Go
+        uses: actions/setup-go@v4
+        timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time
+        with:
+          go-version: ${{ env.GO_VERSION }}
+          cache: true
+      - name: Setup tests (${{ matrix.targets.name }})
+        run: VERBOSE=1 make -e GO_TEST_PACKAGES="${{ matrix.targets.packages }}" install-tools
+      - name: Run tests (${{ matrix.targets.name }})
+        uses: nick-fields/retry@v2
+        with:
+          timeout_minutes: 35
+          max_attempts: 5
+          command: VERBOSE=1 make -C ./integration -e GO_TEST_PACKAGES="${{ matrix.targets.packages }}" test
+        # TODO(rbtz): re-enable when we fix exisiting races.
+        #env:
+        #  RACE_DETECTOR: 1
+      - name: Upload coverage report
+        uses: codecov/codecov-action@v3
+        with:
+          file: ./coverage.txt
+          flags: unittests
+          name: codecov-umbrella
+
   integration-test:
     name: Integration Tests
     needs: docker-build

insecure/Makefile

@@ -1,6 +1,10 @@
 # Name of the cover profile
 COVER_PROFILE := cover.out
 
+# By default, this will run all tests in all packages, but we have a way to override this in CI so that we can
+# dynamically split up CI jobs into smaller jobs that can be run in parallel
+GO_TEST_PACKAGES := ./...
+
 # allows CI to specify whether to have race detection on / off
 ifeq ($(RACE_DETECTOR),1)
 	RACE_FLAG := -race
@@ -15,7 +19,7 @@ CGO_FLAG := CGO_CFLAGS=$(CRYPTO_FLAG)
 # runs all unit tests of the insecure module
 .PHONY: test
 test:
-	$(CGO_FLAG) go test $(if $(VERBOSE),-v,) -coverprofile=$(COVER_PROFILE) $(RACE_FLAG) $(if $(JSON_OUTPUT),-json,) $(if $(NUM_RUNS),-count $(NUM_RUNS),) ./...
+	$(CGO_FLAG) go test $(if $(VERBOSE),-v,) -coverprofile=$(COVER_PROFILE) $(RACE_FLAG) $(if $(JSON_OUTPUT),-json,) $(if $(NUM_RUNS),-count $(NUM_RUNS),) $(GO_TEST_PACKAGES)
 
 .PHONY: lint
 lint: tidy

insecure/integration/functional/test/gossipsub/rpc_inspector/validation_inspector_test.go

@@ -957,23 +957,19 @@ func TestValidationInspector_InspectRpcPublishMessages(t *testing.T) {
 	require.Equal(t, uint64(1), notificationCount.Load())
 }
 
-// TestGossipSubSpamMitigationIntegration tests that the spam mitigation feature of GossipSub is working as expected.
-// The test puts toghether the spam detection (through the GossipSubInspector) and the spam mitigation (through the
-// scoring system) and ensures that the mitigation is triggered when the spam detection detects spam.
-// The test scenario involves a spammer node that sends a large number of control messages to a victim node.
-// The victim node is configured to use the GossipSubInspector to detect spam and the scoring system to mitigate spam.
-// The test ensures that the victim node is disconnected from the spammer node on the GossipSub mesh after the spam detection is triggered.
-func TestGossipSubSpamMitigationIntegration(t *testing.T) {
-	unittest.SkipUnless(t, unittest.TEST_FLAKY, "https://github.com/dapperlabs/flow-go/issues/6949")
-	t.Run("gossipsub spam mitigation invalid grafts", func(t *testing.T) {
-		testGossipSubSpamMitigationIntegration(t, p2pmsg.CtrlMsgGraft)
-	})
-	t.Run("gossipsub spam mitigation invalid prunes", func(t *testing.T) {
-		testGossipSubSpamMitigationIntegration(t, p2pmsg.CtrlMsgPrune)
-	})
-	t.Run("gossipsub spam mitigation invalid ihaves", func(t *testing.T) {
-		testGossipSubSpamMitigationIntegration(t, p2pmsg.CtrlMsgIHave)
-	})
+// TestGossipSubSpamMitigationIntegration_Grafts tests that the spam mitigation feature of GossipSub is working as expected for Graft control messages.
+func TestGossipSubSpamMitigationIntegration_Grafts(t *testing.T) {
+	testGossipSubSpamMitigationIntegration(t, p2pmsg.CtrlMsgGraft)
+}
+
+// TestGossipSubSpamMitigationIntegration_Prunes tests that the spam mitigation feature of GossipSub is working as expected for Prune control messages.
+func TestGossipSubSpamMitigationIntegration_Prunes(t *testing.T) {
+	testGossipSubSpamMitigationIntegration(t, p2pmsg.CtrlMsgPrune)
+}
+
+// TestGossipSubSpamMitigationIntegration_IHaves tests that the spam mitigation feature of GossipSub is working as expected for IHaves control messages.
+func TestGossipSubSpamMitigationIntegration_IHaves(t *testing.T) {
+	testGossipSubSpamMitigationIntegration(t, p2pmsg.CtrlMsgIHave)
 }
 
 // testGossipSubSpamMitigationIntegration tests that the spam mitigation feature of GossipSub is working as expected.
@@ -994,6 +990,8 @@ func testGossipSubSpamMitigationIntegration(t *testing.T, msgType p2pmsg.Control
 	// set the scoring parameters to be more aggressive to speed up the test
 	cfg.NetworkConfig.GossipSub.RpcTracer.ScoreTracerInterval = 100 * time.Millisecond
 	cfg.NetworkConfig.GossipSub.ScoringParameters.ScoringRegistryParameters.AppSpecificScore.ScoreTTL = 100 * time.Millisecond
+	cfg.NetworkConfig.GossipSub.ScoringParameters.ScoringRegistryParameters.SpamRecordCache.Decay.MaximumSpamPenaltyDecayFactor = .99
+
 	victimNode, victimId := p2ptest.NodeFixture(t,
 		sporkID,
 		t.Name(),
@@ -1023,7 +1021,7 @@ func testGossipSubSpamMitigationIntegration(t *testing.T, msgType p2pmsg.Control
 		}
 	})
 
-	spamRpcCount := 50000           // total number of individual rpc messages to send
+	spamRpcCount := 1000            // total number of individual rpc messages to send
 	spamCtrlMsgCount := int64(1000) // total number of control messages to send on each RPC
 
 	// unknownTopic is an unknown topic to the victim node but shaped like a valid topic (i.e., it has the correct prefix and spork ID).
@@ -1067,7 +1065,7 @@ func testGossipSubSpamMitigationIntegration(t *testing.T, msgType p2pmsg.Control
 	case p2pmsg.CtrlMsgPrune:
 		unknownTopicSpam = spammer.GenerateCtlMessages(int(spamCtrlMsgCount), p2ptest.WithPrune(spamRpcCount, unknownTopic.String()))
 		malformedTopicSpam = spammer.GenerateCtlMessages(int(spamCtrlMsgCount), p2ptest.WithPrune(spamRpcCount, malformedTopic.String()))
-		invalidSporkIDTopicSpam = spammer.GenerateCtlMessages(int(spamCtrlMsgCount), p2ptest.WithGraft(spamRpcCount, invalidSporkIDTopic.String()))
+		invalidSporkIDTopicSpam = spammer.GenerateCtlMessages(int(spamCtrlMsgCount), p2ptest.WithPrune(spamRpcCount, invalidSporkIDTopic.String()))
 		duplicateTopicSpam = spammer.GenerateCtlMessages(int(spamCtrlMsgCount), // sets duplicate to +2 above the threshold to ensure that the victim node will penalize the spammer node
 			p2ptest.WithPrune(cfg.NetworkConfig.GossipSub.RpcInspector.Validation.GraftPrune.DuplicateTopicIdThreshold+2, duplicateTopic.String()))
 	case p2pmsg.CtrlMsgIHave:

insecure/integration/functional/test/gossipsub/scoring/scoring_test.go

@@ -27,13 +27,11 @@ import (
 // a spammer peer, the victim will eventually penalize the spammer and stop receiving messages from them.
 // Note: the term integration is used here because it requires integrating all components of the libp2p stack.
 func TestGossipSubInvalidMessageDelivery_Integration(t *testing.T) {
-	unittest.SkipUnless(t, unittest.TEST_FLAKY, "https://github.com/dapperlabs/flow-go/issues/6949")
 	tt := []struct {
 		name           string
 		spamMsgFactory func(spammerId peer.ID, victimId peer.ID, topic channels.Topic) *pubsub_pb.Message
 	}{
 		{
-
 			name: "unknown peer, invalid signature",
 			spamMsgFactory: func(spammerId peer.ID, _ peer.ID, topic channels.Topic) *pubsub_pb.Message {
 				return p2ptest.PubsubMessageFixture(t, p2ptest.WithTopic(topic.String()))
@@ -96,7 +94,6 @@ func TestGossipSubInvalidMessageDelivery_Integration(t *testing.T) {
 // - t: the test instance.
 // - spamMsgFactory: a function that creates unique invalid messages to spam the victim with.
 func testGossipSubInvalidMessageDeliveryScoring(t *testing.T, spamMsgFactory func(peer.ID, peer.ID, channels.Topic) *pubsub_pb.Message) {
-
 	role := flow.RoleConsensus
 	sporkId := unittest.IdentifierFixture()
 	blockTopic := channels.TopicFromChannel(channels.PushBlocks, sporkId)
@@ -143,7 +140,7 @@ func testGossipSubInvalidMessageDeliveryScoring(t *testing.T, spamMsgFactory fun
 		msgs = append(msgs, spamMsgFactory(spammer.SpammerNode.ID(), victimNode.ID(), blockTopic))
 	}
 
-	// sends all 2000 spam messages to the victim node over 1 RPC.
+	// sends all 3000 spam messages to the victim node over 1 RPC.
 	spammer.SpamControlMessage(t, victimNode,
 		spammer.GenerateCtlMessages(1), msgs...)
 

integration/Makefile

@@ -1,6 +1,8 @@
 # Name of the cover profile
 COVER_PROFILE := cover.out
 
+GO_TEST_PACKAGES := `go list ./... | grep -v -e integration/tests`
+
 # allows CI to specify whether to have race detection on / off
 ifeq ($(RACE_DETECTOR),1)
 	RACE_FLAG := -race
@@ -19,7 +21,7 @@ integration-test: access-tests ghost-tests mvp-tests execution-tests verificatio
 # Run unit tests for test utilities in this module
 .PHONY: test
 test:
-	$(CGO_FLAG) go test $(if $(VERBOSE),-v,) -coverprofile=$(COVER_PROFILE) $(RACE_FLAG) $(if $(JSON_OUTPUT),-json,) $(if $(NUM_RUNS),-count $(NUM_RUNS),) `go list ./... | grep -v -e integration/tests`
+	$(CGO_FLAG) go test $(if $(VERBOSE),-v,) -coverprofile=$(COVER_PROFILE) $(RACE_FLAG) $(if $(JSON_OUTPUT),-json,) $(if $(NUM_RUNS),-count $(NUM_RUNS),) $(GO_TEST_PACKAGES)
 
 .PHONY: access-tests
 access-tests: access-cohort1-tests access-cohort2-tests access-cohort3-tests

tools/test_matrix_generator/default-test-matrix-config.json

@@ -0,0 +1,33 @@
+{
+  "includeOthers": true,
+  "packages": [
+    {"name": "admin"},
+    {"name":  "cmd"},
+    {"name":  "consensus"},
+    {"name":  "fvm"},
+    {"name":  "ledger"},
+    {"name":  "state"},
+    {"name":  "storage"},
+    {"name":  "utils"},
+    {"name":  "engine", "runner":  "buildjet-4vcpu-ubuntu-2004" ,"subpackages": [
+      {"name": "engine/access"},
+      {"name": "engine/collection"},
+      {"name": "engine/common"},
+      {"name": "engine/consensus"},
+      {"name": "engine/execution/computation"},
+      {"name": "engine/execution"},
+      {"name": "engine/verification"},
+      {"name": "engine/execution/ingestion", "runner":  "buildjet-8vcpu-ubuntu-2004"}
+    ]},
+    {"name":  "module", "runner":  "buildjet-4vcpu-ubuntu-2004" ,"subpackages": [{"name": "module/dkg"}]},
+    {"name":  "network", "subpackages": [
+      {"name": "network/alsp"},
+      {"name": "network/p2p/connection"},
+      {"name": "network/p2p/scoring"},
+      {"name": "network/p2p", "runner":  "buildjet-16vcpu-ubuntu-2004"},
+      {"name": "network/test/cohort1", "runner":  "buildjet-16vcpu-ubuntu-2004"},
+      {"name": "network/test/cohort2", "runner":  "buildjet-4vcpu-ubuntu-2004"},
+      {"name": "network/p2p/node", "runner":  "buildjet-4vcpu-ubuntu-2004"}
+    ]}
+  ]
+}

tools/test_matrix_generator/insecure-module-test-matrix-config.json

@@ -0,0 +1,10 @@
+{
+  "packagesPath": "./insecure",
+  "includeOthers": false,
+  "packages": [
+      {"name":  "insecure", "runner":  "buildjet-4vcpu-ubuntu-2004" ,"subpackages": [
+      {"name": "insecure/integration/functional/test/gossipsub/rpc_inspector", "runner":  "buildjet-8vcpu-ubuntu-2004"},
+      {"name": "insecure/integration/functional/test/gossipsub/scoring", "runner":  "buildjet-8vcpu-ubuntu-2004"}
+    ]}
+  ]
+}

tools/test_matrix_generator/integration-module-test-matrix-config.json

@@ -0,0 +1,9 @@
+{
+  "packagesPath": "./integration",
+  "includeOthers": false,
+  "packages": [{
+      "name": "integration",
+      "runner": "buildjet-4vcpu-ubuntu-2004",
+      "exclude": ["integration/tests"]
+    }]
+}