test/gracefulstop: use stubserver instead of testservice implementation #7907

pvsravani · 2024-12-06T07:49:26Z

Partially Addresses: #7291

RELEASE NOTES: None

linux-foundation-easycla · 2024-12-06T07:49:30Z

The committers listed above are authorized under a signed CLA.

✅ login: pvsravani (3d059c2, 6da62e6, a73e23c, 2190f99, 0bfd0f6, 1df0441, a623c8a, 61374bc, f976afd)

codecov · 2024-12-06T07:54:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.02%. Comparing base (66ba4b2) to head (2190f99).
Report is 23 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7907      +/-   ##
==========================================
- Coverage   82.04%   82.02%   -0.03%     
==========================================
  Files         377      381       +4     
  Lines       38180    38539     +359     
==========================================
+ Hits        31326    31610     +284     
- Misses       5551     5606      +55     
- Partials     1303     1323      +20

see 49 files with indirect coverage changes

purnesh42H · 2024-12-09T04:47:59Z

test/gracefulstop_test.go

@@ -122,7 +122,8 @@ func (s) TestGracefulStop(t *testing.T) {
 		},
 	}
 	s := grpc.NewServer()
-	testgrpc.RegisterTestServiceServer(s, ss)
+	ss.S = s


i think you have to assign the listener as well when creating the stub server?

purnesh42H · 2024-12-09T04:49:12Z

test/gracefulstop_test.go

@@ -122,7 +122,8 @@ func (s) TestGracefulStop(t *testing.T) {
 		},
 	}
 	s := grpc.NewServer()
-	testgrpc.RegisterTestServiceServer(s, ss)
+	ss.S = s


nit: this can be just ss.S = grpc.NewServer() and all the usages can be updated as ss.S. like ss.S.stop() etc.

purnesh42H · 2024-12-16T04:27:14Z

test/gracefulstop_test.go

@@ -42,7 +42,9 @@ type delayListener struct {
 	closeCalled  chan struct{}
 	acceptCalled chan struct{}
 	allowCloseCh chan struct{}
+	mu           sync.Mutex


it looks like we are introducing mutex only to update closed? Then it should be put right above it indicating that anything below is guarded by mutex.

purnesh42H · 2024-12-16T04:29:00Z

test/gracefulstop_test.go

 	dialed       bool
+	closed       bool


@pvsravani where and how are we using closed. Why is it needed when switching to stub server?

The closed field ensures the close() method is idempotent. It prevents multiple calls to close() from closing the listener again, which could cause errors.

purnesh42H · 2024-12-16T04:32:12Z

test/gracefulstop_test.go


 	// 1. Start Server
 	wg := sync.WaitGroup{}
 	wg.Add(1)
 	go func() {
-		s.Serve(dlis)
+		ss.S.Serve(dlis)


we should remove this as stubserver.StartTestService already starts the server

purnesh42H · 2024-12-18T06:00:36Z

@easwars for second review

easwars · 2024-12-18T18:45:08Z

test/gracefulstop_test.go

@@ -113,31 +113,27 @@ func (s) TestGracefulStop(t *testing.T) {
 	d := func(ctx context.Context, _ string) (net.Conn, error) { return dlis.Dial(ctx) }


While you are here, could you please rename this d as dialer and move it a line right before it is used. Currently it is only used on line 153, when it is passed to grpc.WithContextDialer in grpc.DialContext.

easwars · 2024-12-20T21:03:14Z

test/gracefulstop_test.go

 	}
-	s := grpc.NewServer()
-	testgrpc.RegisterTestServiceServer(s, ss)
+	stubserver.StartTestService(t, ss)


I don't think this is doing the correct thing. What we want as part of this test is for GracefulStop to be called when Accept is in the middle of accepting a connection.

If you folks have convinced yourself that it actually does what it is supposed to do, then we don't need a sync.WaitGroup. The same can be accomplished with a done channel.

Removed sync.WaitGroup

yeah i don't think we can use sync.waitgroup with stubserver.StartService(). stubserver.StartTestService creates a server and starts serving in the background. It handles server lifecycle internally. Having a done channel to track gracefulstop will work correctly.

purnesh42H · 2025-01-07T13:48:48Z

test/gracefulstop_test.go


 	// 1. Start Server
-	wg := sync.WaitGroup{}
-	wg.Add(1)
+	done := make(chan struct{})


@pvsravani let's rename this to gracefulStopDone

purnesh42H · 2025-01-07T13:49:30Z

test/gracefulstop_test.go

 	go func() {
-		s.Serve(dlis)
-		wg.Done()
+		<-dlis.acceptCalled


I don't think its needed to move it within go routine. Not that its wrong but this change is not needed i think.

purnesh42H · 2025-01-07T13:52:26Z

test/gracefulstop_test.go

 	}
-	s := grpc.NewServer()
-	testgrpc.RegisterTestServiceServer(s, ss)
+	stubserver.StartTestService(t, ss)


yeah i don't think we can use sync.waitgroup with stubserver.StartService(). stubserver.StartTestService creates a server and starts serving in the background. It handles server lifecycle internally. Having a done channel to track gracefulstop will work correctly.

purnesh42H · 2025-01-09T17:37:58Z

test/gracefulstop_test.go

@@ -110,36 +109,29 @@ func (s) TestGracefulStop(t *testing.T) {
 		closeCalled:  make(chan struct{}),
 		allowCloseCh: make(chan struct{}),
 	}
-	d := func(ctx context.Context, _ string) (net.Conn, error) { return dlis.Dial(ctx) }


nit: why not just keep this line 113 and remove line 145, 146 and keep line 153? That way we avoid unnecessary diff

Actually Easwar has asked to rename this d as dialer and move it a line right before it is used.

switching to stubserver

6da62e6

purnesh42H reviewed Dec 9, 2024

View reviewed changes

switching to stubserver

a73e23c

arjan-bal assigned pvsravani Dec 10, 2024

purnesh42H assigned purnesh42H and unassigned pvsravani Dec 14, 2024

purnesh42H changed the title ~~switching to stubserver~~ test/gracefulstop: use stubserver instead of testservice implementation Dec 16, 2024

purnesh42H reviewed Dec 16, 2024

View reviewed changes

purnesh42H assigned pvsravani and unassigned purnesh42H Dec 16, 2024

pvsravani added 3 commits December 16, 2024 06:56

Removing redundant server and updating mutex in close()

0bfd0f6

reverting mutex and closed fields

3d059c2

Moving the intialization of S field into the stubserver

a623c8a

purnesh42H approved these changes Dec 18, 2024

View reviewed changes

purnesh42H assigned easwars and unassigned pvsravani Dec 18, 2024

purnesh42H added the Type: Testing label Dec 18, 2024

purnesh42H added this to the 1.70 Release milestone Dec 18, 2024

easwars reviewed Dec 20, 2024

View reviewed changes

easwars assigned pvsravani and unassigned easwars Dec 20, 2024

renaming d as dialer and removing sync.waitgroup()

61374bc

easwars self-assigned this Jan 6, 2025

purnesh42H reviewed Jan 7, 2025

View reviewed changes

purnesh42H requested a review from easwars January 7, 2025 13:53

pvsravani added 2 commits January 8, 2025 06:34

Renaming done as gracefulStopDone

1df0441

Renaming done as gracefulStopDone

f976afd

Moving <-dlis.acceptCalled out of go routine

2190f99

purnesh42H reviewed Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test/gracefulstop: use stubserver instead of testservice implementation #7907

test/gracefulstop: use stubserver instead of testservice implementation #7907

pvsravani commented Dec 6, 2024 •

edited by purnesh42H

Loading

linux-foundation-easycla bot commented Dec 6, 2024 •

edited

Loading

codecov bot commented Dec 6, 2024 •

edited

Loading

purnesh42H Dec 9, 2024

pvsravani Dec 11, 2024

purnesh42H Dec 9, 2024

pvsravani Dec 11, 2024

purnesh42H Dec 16, 2024

pvsravani Dec 16, 2024

purnesh42H Dec 16, 2024

pvsravani Dec 16, 2024

purnesh42H Dec 16, 2024

pvsravani Jan 8, 2025

purnesh42H commented Dec 18, 2024

easwars Dec 18, 2024

pvsravani Dec 27, 2024

easwars Dec 20, 2024

pvsravani Dec 27, 2024

purnesh42H Jan 7, 2025

purnesh42H Jan 7, 2025

pvsravani Jan 8, 2025

purnesh42H Jan 7, 2025

pvsravani Jan 8, 2025

purnesh42H Jan 7, 2025

purnesh42H Jan 9, 2025

pvsravani Jan 10, 2025

		@@ -113,31 +113,27 @@ func (s) TestGracefulStop(t *testing.T) {
		d := func(ctx context.Context, _ string) (net.Conn, error) { return dlis.Dial(ctx) }

test/gracefulstop: use stubserver instead of testservice implementation #7907

Are you sure you want to change the base?

test/gracefulstop: use stubserver instead of testservice implementation #7907

Conversation

pvsravani commented Dec 6, 2024 • edited by purnesh42H Loading

linux-foundation-easycla bot commented Dec 6, 2024 • edited Loading

codecov bot commented Dec 6, 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

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

purnesh42H commented Dec 18, 2024

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

Choose a reason for hiding this comment

pvsravani commented Dec 6, 2024 •

edited by purnesh42H

Loading

linux-foundation-easycla bot commented Dec 6, 2024 •

edited

Loading

codecov bot commented Dec 6, 2024 •

edited

Loading