refactor: Network test sync logic #2748

nasdf · 2024-06-21T19:06:57Z

Relevant issue(s)

Resolves #2747

Description

This PR improves the network sync test logic to allow for further improvements to the network implementation.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

make test

Specify the platform(s) on which this was tested:

MacOS

codecov · 2024-06-21T19:21:01Z

Codecov Report

Attention: Patch coverage is 95.55556% with 2 lines in your changes missing coverage. Please review.

Project coverage is 79.38%. Comparing base (a7fe539) to head (b4fbe86).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2748      +/-   ##
===========================================
+ Coverage    79.36%   79.38%   +0.02%     
===========================================
  Files          323      323              
  Lines        24661    24691      +30     
===========================================
+ Hits         19572    19600      +28     
+ Misses        3692     3686       -6     
- Partials      1397     1405       +8

Flag	Coverage Δ
all-tests	`79.38% <95.56%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
net/peer.go	`82.49% <100.00%> (-2.44%)`	⬇️
net/server.go	`74.76% <95.00%> (+1.25%)`	⬆️

... and 18 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7fe539...b4fbe86. Read the comment docs.

fredcarle · 2024-06-27T19:50:42Z

tests/integration/p2p.go

+	for i := 0; i < len(s.nodes); i++ {
+		if nodeID.HasValue() && nodeID.Value() != i {
+			continue // node is not selected
+		}


suggestion: The documentation says waits for all selected nodes but the function only receives one. You can probably do away with the loop and just get the node directly.

If nodeID has no value it will wait for all nodes.

fredcarle · 2024-06-27T19:52:26Z

tests/integration/p2p.go

+		// update the expected document heads of connected nodes
+		for id := range s.nodeConnections[i] {
+			if _, ok := s.actualDocHeads[id][evt.DocID]; ok {
+				s.expectedDocHeads[id][evt.DocID] = evt.Cid
+			}
+		}
+		// update the expected document heads of replicator sources
+		for id := range s.nodeReplicatorTargets[i] {
+			if _, ok := s.actualDocHeads[id][evt.DocID]; ok {
+				s.expectedDocHeads[id][evt.DocID] = evt.Cid
+			}
+		}


thougth: It's odd that we would be updating the expected heads only if already present in the actual heads. Shouldn't it be updated regardless?

I believe the code is correct based on what we discussed regarding replicator target nodes sending updates of a document back to the replicator source node.

AndrewSisley · 2024-07-03T20:34:52Z

tests/integration/state.go

-	// eventSubs is a list of all event subscriptions
-	eventSubs []*event.Subscription
+	// nodeUpdateSubs is a list of all update event subscriptions
+	nodeUpdateSubs []*event.Subscription


suggestion: Can we wrap these up in a dedicated type? They are consuming a disproportionately large amount of attention on the state struct compared to their visibility needs.

something like:

type p2pState struct { nodeConnections []map[int]struct{} ... } type state struct { ... p2pState p2pState }

AndrewSisley · 2024-07-03T20:38:38Z

tests/integration/utils2.go

@@ -1205,6 +1205,10 @@ func createDoc(
 		s.documents = append(s.documents, make([][]*client.Document, action.CollectionID-len(s.documents)+1)...)
 	}
 	s.documents[action.CollectionID] = append(s.documents[action.CollectionID], doc)
+
+	if action.ExpectedError == "" {
+		waitForUpdateEvents(s, action.NodeID, action.CollectionID)


thought: This is a significant change in the tests - and one that looks like it removes our ability to test concurrent P2P stuff (something that we very much want-to/need-to/did test)?

The function name is a bit misleading, but Update events are published immediately after a document is saved and shouldn't really change how we test things concurrently.

We could also merge waitForUpdateEvents into waitForSync since the events are buffered if you would prefer that.

Ah I see what you mean - it is not waiting for the sync, just the event publishing to the current node's event queue?

Why are you calling it then?

Ah I see what you mean - it is not waiting for the sync, just the event publishing to the current node's event queue?

Yes

Why are you calling it then?

Its a convenient way to track which doc heads are expected to be merged on remote nodes.

Its a convenient way to track which doc heads are expected to be merged on remote nodes.

Can you please document that really well? Our integration tests are already super magic and far more complex than most, and that complexity is not really tested. The P2P space is particularly complicated and prone to annoying, late-to-discover, flaky bugs and it is well worth over-explaining things to save future hassle.

I added more documentation about how the document head are tracked. It should also be much easier to follow since its all moved to an events.go file. Let me know what you think.

Changes look really good, thanks Keenan. Has got me thinking about test Cid substitution too, in the future we might want to tweak state.nodeP2P.DocHeads to allow this.

AndrewSisley · 2024-07-03T20:41:29Z

tests/integration/utils2.go

@@ -1355,6 +1359,10 @@ func deleteDoc(
 	}

 	assertExpectedErrorRaised(s.t, s.testCase.Description, action.ExpectedError, expectedErrorRaised)
+
+	if action.ExpectedError == "" {
+		waitForUpdateEvents(s, action.NodeID, action.CollectionID)


question: Why is this only done using create-save, and not create-gql and the other create? And why not on delete?

thought: This looks quite easy to mess up and forget, and it missing it might not create an obvious and immediate failure (just flaky tests).

+1 similar thought, for here and other instances of this change.

If I'm understanding this correctly the createDoc function calls the create-save and create-gql functions so they both should be covered.

😁 My bad, the github presentation of the diff helped me miss-read where this line was... Ignore this thread :)

AndrewSisley

I've not finished reviewing, but want to block the merge until my concerns around the apparent loss of concurrency have been resolved. The code looks simpler, but I think we may have lost the ability to test some important things.

AndrewSisley · 2024-07-03T20:46:10Z

todo: Please make sure you run the P2P tests a lot (IIRC Fred/I had to run them 100 or so times to reproduce the last solved issues) before merging this, we had flakiness problems with this before and they cost a lot of time.

Maybe involve a few colleagues with different machines, as in the past more constrained machines had different flaky bugs compared to faster ones.

islamaliev · 2024-07-04T05:37:02Z

tests/integration/state.go

+	// nodeReplicatorSources contains all active replicators.
+	//
+	// The index of the slice is the source node id. The map key is the target node id.
+	nodeReplicatorSources []map[int]struct{}
+
+	// nodeReplicatorTargets contains all active replicators.


suggestion: documentation suggests that nodeReplicatorSources and nodeReplicatorTargets are the same, but they are not. Please make documentation reflect it.

AndrewSisley · 2024-07-04T16:32:32Z

tests/integration/utils2.go

@@ -781,6 +766,10 @@ func refreshCollections(
 ) {
 	s.collections = make([][]client.Collection, len(s.nodes))

+	for i := len(s.nodePeerCollections); i < len(s.collectionNames); i++ {
+		s.nodePeerCollections = append(s.nodePeerCollections, make(map[int]struct{}))


todo: This looks like it will break when collections are removed/deactivated in a node (using patchCollection)? Users currently can do this, although we likely have no P2P tests for it atm.

There may be other test things that currently break when doing that, but we should avoid adding new ones.

This should be easy to handle. Just need to find which collections to deactivate from the p2p state.

handle patchCollection

AndrewSisley · 2024-07-04T16:33:40Z

tests/integration/utils2.go

-		switch action := tc.(type) {
-		case ConnectPeers:
-			// Give the nodes a chance to connect to each other and learn about each other's subscribed topics.
-			time.Sleep(100 * time.Millisecond)


praise: I forgot we were currently relying on a sleep - thank you very much for removing this!

AndrewSisley · 2024-07-04T16:37:40Z

tests/integration/p2p.go

@@ -479,7 +337,61 @@ func getAllP2PCollections(
 	assert.Equal(s.t, expectedCollections, cols)
 }

-// waitForSync waits for all given wait channels to receive an item signaling completion.
+// waitForUpdateEvents waits for all selected nodes to publish an update event.


suggestion: Building on the other related documentation request, it might be worth highlighting that you mean publish to the local event queue, not to the pub/sub net or some other P2P related network thing.

AndrewSisley · 2024-07-17T14:41:09Z

tests/integration/events.go

+	reps, err := s.nodes[nodeID].GetAllReplicators(s.ctx)
+	require.NoError(s.t, err)
+
+	p2pTopicEvents := 0


question: It looks like p2pTopicEvents is only ever 0 or 1, why is it not a boolean? Is this a bug/wip?

Changed to bool

AndrewSisley · 2024-07-17T14:49:11Z

tests/integration/utils2.go

@@ -1205,6 +1205,10 @@ func createDoc(
 		s.documents = append(s.documents, make([][]*client.Document, action.CollectionID-len(s.documents)+1)...)
 	}
 	s.documents[action.CollectionID] = append(s.documents[action.CollectionID], doc)
+
+	if action.ExpectedError == "" {
+		waitForUpdateEvents(s, action.NodeID, action.CollectionID)


Changes look really good, thanks Keenan. Has got me thinking about test Cid substitution too, in the future we might want to tweak state.nodeP2P.DocHeads to allow this.

AndrewSisley

Looks a really nice improvement, thanks Keenan :)

AndrewSisley · 2024-07-17T14:52:20Z

tests/integration/utils2.go

@@ -1355,6 +1359,10 @@ func deleteDoc(
 	}

 	assertExpectedErrorRaised(s.t, s.testCase.Description, action.ExpectedError, expectedErrorRaised)
+
+	if action.ExpectedError == "" {
+		waitForUpdateEvents(s, action.NodeID, action.CollectionID)


😁 My bad, the github presentation of the diff helped me miss-read where this line was... Ignore this thread :)

AndrewSisley · 2024-07-17T14:56:29Z

tests/integration/state.go

@@ -24,6 +25,86 @@ import (
 	"github.com/sourcenetwork/defradb/tests/clients"
 )

+// p2pState contains all p2p related testing test.


nitpick: related testing state.?

islamaliev

Looks good. Core looks now much more intuitive. I just a have a question to clarify.

islamaliev · 2024-07-17T20:06:13Z

tests/integration/utils2.go

@@ -342,7 +341,7 @@ func performAction(
 		assertClientIntrospectionResults(s, action)

 	case WaitForSync:
-		waitForSync(s, action)
+		waitForMergeEvents(s, action)


praise: it's great that now waiting is done on the same thread. Earlier it used to call a goroutine for every ConnectPeers action and had subtle shortcomings.

islamaliev · 2024-07-17T20:09:42Z

tests/integration/utils2.go

@@ -1457,6 +1424,10 @@ func updateDoc(
 	}

 	assertExpectedErrorRaised(s.t, s.testCase.Description, action.ExpectedError, expectedErrorRaised)
+
+	if action.ExpectedError == "" {
+		waitForUpdateEvents(s, action.NodeID, action.CollectionID)


questions: this looks like unsolicited sync. Why is it unconditional?

There should always be an update event published to the local event bus if a document is updated or created.

islamaliev · 2024-07-17T20:33:37Z

tests/integration/test_case.go

-	DontSync bool
+	// Skip waiting for an update event. This should only be used for
+	// tests that do not correctly publish an update event.
+	SkipUpdateEvent bool


suggestion: why is it synced by default now? That resembles sequentially consistent memory order instead of relaxed one that we used before.
I think, if we want to sync, we should request it explicitly. Unsynced actions are closer to realistic behaviour.

todo: I think this is a misleading name and comment, IIRC waitForUpdateEvents does not wait for the P2P sync, but only the update event to be published to the event bus. If that is correct, @nasdf can you please rework the property name and documentation to make that clearer.

what is this then if not waiting: <-s.nodeEvents[i].update.Message():?

That is just the event bus, it is written to synchronously by the database when mutating documents - the P2P code also subscribes to this bus - so 'waiting' to the bus should never really block, and the P2P stuff hasn't even been sent over the network at that point.

If I understand it correctly. I made the same comment as you did earlier for waitForUpdateEvents - I think Keenan improved the documentation and code location since I commented, but given that it is still tripping up reviewers, that too should probably be improved further.

shahzadlone

Good Stuff 👍

refactor network test actions

e25b68a

nasdf added the area/testing Related to any test or testing suite label Jun 21, 2024

nasdf added this to the DefraDB v0.12 milestone Jun 21, 2024

nasdf self-assigned this Jun 21, 2024

nasdf added 2 commits June 21, 2024 12:21

fix node restart subscription

d832d32

fix peer collection expected docs

b299568

fredcarle mentioned this pull request Jun 25, 2024

refactor: Decouple client.DB from net #2768

Merged

4 tasks

fredcarle reviewed Jun 27, 2024

View reviewed changes

nasdf added 2 commits July 2, 2024 09:58

Merge branch 'develop' into nasdf/refactor/network-test-actions

489dc47

Merge branch 'develop' into nasdf/refactor/network-test-actions

ba973ee

nasdf requested a review from a team July 3, 2024 18:16

AndrewSisley reviewed Jul 3, 2024

View reviewed changes

AndrewSisley requested changes Jul 3, 2024

View reviewed changes

islamaliev reviewed Jul 4, 2024

View reviewed changes

AndrewSisley reviewed Jul 4, 2024

View reviewed changes

nasdf added 6 commits July 16, 2024 09:08

Merge branch 'develop' into nasdf/refactor/network-test-actions

997148a

add test event state

8fb16cf

move all event test logic to events file

6e6e3f5

move p2p state to struct

f3540b0

simplify p2p test state

4b88f6b

make peer connections deterministic. remove p2p sleeps

6e136a1

nasdf requested a review from AndrewSisley July 16, 2024 20:59

handle closed subscriptions

25ee3a4

AndrewSisley reviewed Jul 17, 2024

View reviewed changes

AndrewSisley approved these changes Jul 17, 2024

View reviewed changes

add required sleeps to p2p test actions

301445c

islamaliev approved these changes Jul 17, 2024

View reviewed changes

fix acp tests

666dbd2

islamaliev reviewed Jul 17, 2024

View reviewed changes

nasdf and others added 3 commits July 17, 2024 13:56

notify exchange of new block before push log

6e8fc67

fix linter error

d036cb6

Merge branch 'develop' into nasdf/refactor/network-test-actions

85410d3

shahzadlone approved these changes Jul 18, 2024

View reviewed changes

nasdf and others added 2 commits July 18, 2024 09:22

rename SkipUpdateEvent to SkipLocalUpdateEvent

5596026

Merge branch 'develop' into nasdf/refactor/network-test-actions

b4fbe86

nasdf merged commit 517333c into sourcenetwork:develop Jul 18, 2024
40 of 42 checks passed

refactor: Network test sync logic #2748

refactor: Network test sync logic #2748

Conversation

nasdf commented Jun 21, 2024

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Jun 21, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

AndrewSisley commented Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islamaliev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islamaliev Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

AndrewSisley Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 21, 2024 •

edited

Loading

AndrewSisley Jul 3, 2024 •

edited

Loading

AndrewSisley Jul 3, 2024 •

edited

Loading

AndrewSisley commented Jul 3, 2024 •

edited

Loading

AndrewSisley Jul 4, 2024 •

edited

Loading

islamaliev Jul 17, 2024 •

edited

Loading

AndrewSisley Jul 17, 2024 •

edited

Loading