cleanup: refactor and use bicopy everywhere #2944

tamird · 2024-11-25T19:43:13Z

See commit message.

nirs

This changes the behavior but you did not provide any reason for the change. If the purpose is to remove eliminate a package, just inline the package code in the single user. There is no need to change the code.

If you want to reimplement it and/or change the behavior, please explain why.

Also we have good tests for port_darwin? Before rewriting we should have good tests ensuring that we don't change the behavior by mistake. It should be simple test coping data over unix sockets.

pkg/hostagent/port_darwin.go

pkg/bicopy/bicopy.go

pkg/vz/network_darwin.go

pkg/bicopy/bicopy.go

pkg/vz/network_darwin.go

pkg/portfwd/client.go

pkg/bicopy/bicopy.go

nirs · 2024-12-03T01:49:25Z

I think the common behavior in all 3 cases is that we start an endless io.Copy() in both directions. The copy is not expected to complete in normal conditions. This is the case for vz/network_darwin.go, I' not sure about the other 2, maybe port forwarding may end earlier in normal conditions.

In the vz network case, the copy should stop only when the vm is shutting down, or if socket_vmnet crashed or terminated. In this case we want to log an error or warning message about the peer closing the socket. Any other error should be logged as an error. In base case we want to log the issue immediately without a delay, so we don't lose errors.

When one of the io.Copy() goroutines fail, we want to other to fail immediately. I'm not sure this is ensured by current code. This should fixed and tested before we unify the implementation.

When both io.Copy() finished, we already logged the error, so there is nothing to return to the caller, even if we have a caller waiting for the result.

Usually saving the first error (synchronized with a lock or atomics) and returning it will be more helpful than returning 8 possible errors joined. After something fail, the next errors are usually just noise. If we can ensure that the second copy fail immediately when the first fail, we can return the first error to the caller and the caller can log it. This will be more useful for a library since different callers may want to use different loggers. May not be relevant to lima use case.

If we improve bicopy we want to share them with upstream. Did you open upstream issues for the problems you are trying to fix in bicopy?

tamird · 2024-12-03T16:30:23Z

If we improve bicopy we want to share them with upstream. Did you open upstream issues for the problems you are trying to fix in bicopy?

I'm just removing features we aren't using. If they need early cancellation that's just fine.

nirs

I like the simplified Bicopy!

pkg/vz/network_darwin.go

pkg/bicopy/bicopy.go

nirs · 2024-12-03T17:33:22Z

If we improve bicopy we want to share them with upstream. Did you open upstream issues for the problems you are trying to fix in bicopy?

I'm just removing features we aren't using. If they need early cancellation that's just fine.

Good reason why this is not relevant to upstream.

pkg/vz/network_darwin.go

pkg/portfwd/client.go

pkg/hostagent/port_darwin.go

pkg/bicopy/bicopy.go

pkg/portfwd/client.go

nirs · 2024-12-03T21:12:39Z

@tamird see #2969 for the io.EOF checks.

tamird · 2024-12-04T00:13:11Z

Looks like I caught a test flake - I don't see bicopy in the stack trace.

TestGetPorts/loadBalancer_service
2024-12-03T23:19:21.6922190Z panic: test timed out after 10m0s

jandubois · 2024-12-04T00:18:49Z

Looks like I caught a test flake - I don't see bicopy in the stack trace.

I've triggered a re-run, but I'm waiting for approval from another reviewer before merging; not going to work my way through 72 comments... 😄

balajiv113 · 2024-12-04T04:29:55Z

pkg/bicopy/bicopy.go

-	<-finish
-	// TODO: return copied bytes
+	}()
+	wg.Wait()


Do we really need bicopy to handle the error and not throw it to the caller ??

I would prefer it to be handled by the caller itself. It gives us better control. Handling error ourself looks generic but all we are doing is just logging. But in some cases we may need to act on error in caller side

There has been a lot of back and forth on this already. I'm inclined to leave it as is. Another option is to pass a handler callback, but nobody needs this today.

but nobody needs this today - If errors are returned back, callers will have more control to handle logging parts. Some cases might need more detailed logging rather than generic message context: io.Copy(y, x) which is not self explanatory.

If you have a valid reason for not returning error please let me know.

Also i think func Bicopy(context string, x, y NamedReadWriter) error {} should be good as well. We don't need to complicate with handlers / callbacks

@nirs expressed concern that if io.Copy in one direction returns, the io.Copy in the other direction might not - and returning an error cannot happen until both functions complete. That's why the latest versions logs directly instead of returning an error.

It will eventually fail because one end of stream is closed already.

Since we have wg.Done at last. Returning error after both io.Copy are closed looks correct to me.

With errgroup as soon as one is returned non-nil its over. It will stop the other go func as well.

If we are using reader/writer then we will not even have close so it's just stopping the loop

The goroutine is canceled magically? how does it stop?

I don't see anything in the docs about cancelling goroutine blocked on a syscall.
https://pkg.go.dev/golang.org/x/sync/errgroup#WithContext

Yes you are right, My mistake.

Only context is closed. So we need to check for context close and close the connection.

For pure reader / writer until that syscall is returned we have no other option. But most of our reader / writer have close method so io.Closable should be fine

I think this should also be added to tcpproxy upstream - ensure termination and return an error. With both we can use it instead of Bicopy.

Improve error logging in bicopy and extend its use to `pkg/vz/network_darwin.go`. Signed-off-by: Tamir Duberstein <[email protected]>

nirs · 2025-01-13T19:13:53Z

pkg/bicopy/bicopy.go

-			logrus.WithError(err).Debug("failed to call xCloser.Close")
+		defer wg.Done()
+		if _, err := io.Copy(x.ReadWriter, y.ReadWriter); err != nil {
+			logrus.WithError(err).Errorf("%s: io.Copy(%s, %s)", context, x.Name, y.Name)


This logs an error like:

"forwarding packets: io.Copy(VMNENT, VZ)" ... {"error": "actual error from io.Copy"}

For code already calling bicopy, this is an improvement, since before we logged a DEBUG message without any information on which direction failed, or what is the purpose of the copy:

"failed to call io.Copy" ... ... {"error": "actual error from io.Copy"}

But for the call from network_darwin this is worse. Before we had this clear error:

"Failed to forward packets from VZ to VMNET: actual error from io.Copy"

I would like to keep the previous error message or something very close. I don't see the value of logging the error separately using logrous.WithError().

It can be done using:

logrus.Errorf("Failed to %s from %s to %s: %s", context, y.Name, x.Name, err)

nirs · 2025-01-13T19:15:23Z

pkg/hostagent/port_darwin.go

 	defer ac.Close()
 	unixConn, err := net.DialUnix("unix", nil, plf.unixAddr)
 	if err != nil {
-		return err
+		logrus.WithError(err).Errorf("pseudoloopback forwarder: failed to dial %q", plf.unixAddr)


This error adds context (good) but logs the error separately (bad). Before we logged only the error from net.DialUnix (line 139).

nirs · 2025-01-13T19:17:41Z

pkg/hostagent/port_darwin.go

+	}, bicopy.NamedReadWriter{
+		ReadWriter: unixConn,
+		Name:       "unix",
+	})


The context does not match other context ("pseudoloopback fowarder" vs "forward packets") and the names of the readwriters are little bit too generic, I'm not sure what is the context on this code, may be others can suggest better values.

nirs · 2025-01-13T19:21:29Z

pkg/portfwd/client.go

@@ -29,7 +29,14 @@ func HandleTCPConnection(ctx context.Context, client *guestagentclient.GuestAgen
 	}

 	rw := &GrpcClientRW{stream: stream, id: id, addr: guestAddr, protocol: "tcp"}
-	bicopy.Bicopy(rw, conn, nil)
+
+	bicopy.Bicopy("tcp tunnel", bicopy.NamedReadWriter{


This is the tunnel for tcp port forwarding, so maybe a better context would be "forward tcp packets for port N". We probably can get the port from conn.RemoteAddr(), since we forward port N to port N on the host.

nirs · 2025-01-13T19:24:05Z

pkg/portfwd/client.go

+		Name:       guestAddr,
+	}, bicopy.NamedReadWriter{
+		ReadWriter: conn,
+		Name:       id,


id is a description of the entire port forwading action: "tcp-%s-%s" (see line 17), so it is not a good for the conn.

nirs · 2025-01-13T19:26:13Z

pkg/portfwdserver/server.go

+
+	bicopy.Bicopy("tcp tunnel", bicopy.NamedReadWriter{
+		ReadWriter: rw,
+		Name:       in.Id,


Not sure what Id is. Can you share the output we get in runtime?

nirs · 2025-01-13T19:33:28Z

pkg/portfwdserver/server.go

@@ -32,7 +32,14 @@ func (s *TunnelServer) Start(stream api.GuestService_TunnelServer) error {
 		return err
 	}
 	rw := &GRPCServerRW{stream: stream, id: in.Id}
-	bicopy.Bicopy(rw, conn, nil)
+
+	bicopy.Bicopy("tcp tunnel", bicopy.NamedReadWriter{


We can probably use the same context used in the client with the guest port.

tamird · 2025-01-13T19:41:03Z

Since I don't remember why I care about this and it's not fixing obvious bugs, I'm closing. If you want to reuse this work, you have my permission to do so without attribution.

tamird force-pushed the remove-bicopy branch 2 times, most recently from 4ec7fa0 to 63ae319 Compare November 27, 2024 17:49

nirs reviewed Nov 27, 2024

View reviewed changes

pkg/hostagent/port_darwin.go Outdated Show resolved Hide resolved

pkg/hostagent/port_darwin.go Outdated Show resolved Hide resolved

pkg/hostagent/port_darwin.go Outdated Show resolved Hide resolved

pkg/hostagent/port_darwin.go Show resolved Hide resolved

tamird force-pushed the remove-bicopy branch 2 times, most recently from 6a612d9 to c8e7582 Compare November 27, 2024 21:30

AkihiroSuda reviewed Nov 28, 2024

View reviewed changes

pkg/bicopy/bicopy.go Show resolved Hide resolved

nirs reviewed Nov 28, 2024

View reviewed changes

pkg/vz/network_darwin.go Show resolved Hide resolved

pkg/vz/network_darwin.go Show resolved Hide resolved

tamird force-pushed the remove-bicopy branch 3 times, most recently from a2a75a4 to b92ac24 Compare November 28, 2024 13:55

tamird changed the title ~~cleanup: remove package bicopy~~ cleanup: refactor and use bicopy everywhere Nov 28, 2024

tamird force-pushed the remove-bicopy branch from b92ac24 to 3d32079 Compare November 28, 2024 20:05

AkihiroSuda requested a review from nirs December 2, 2024 02:07

nirs reviewed Dec 3, 2024

View reviewed changes

nirs requested a review from balajiv113 December 3, 2024 01:14

tamird force-pushed the remove-bicopy branch from 3d32079 to 29d9e37 Compare December 3, 2024 11:30

nirs reviewed Dec 3, 2024

View reviewed changes

pkg/vz/network_darwin.go Outdated Show resolved Hide resolved

pkg/bicopy/bicopy.go Show resolved Hide resolved

tamird force-pushed the remove-bicopy branch from 29d9e37 to e68889d Compare December 3, 2024 18:36

nirs reviewed Dec 3, 2024

View reviewed changes

pkg/portfwd/client.go Outdated Show resolved Hide resolved

tamird force-pushed the remove-bicopy branch from e68889d to 553ebd9 Compare December 3, 2024 20:46

tamird mentioned this pull request Dec 3, 2024

portfwd: Optimize errors.Is() checks #2969

Merged

tamird force-pushed the remove-bicopy branch from 553ebd9 to ac2e30f Compare December 3, 2024 23:08

balajiv113 reviewed Dec 4, 2024

View reviewed changes

nirs mentioned this pull request Dec 6, 2024

Revamp GRPC Port forwarding tunnels to use existing proxy #2985

Merged

cleanup: refactor and use bicopy everywhere

cb7141f

Improve error logging in bicopy and extend its use to `pkg/vz/network_darwin.go`. Signed-off-by: Tamir Duberstein <[email protected]>

tamird force-pushed the remove-bicopy branch from ac2e30f to cb7141f Compare January 13, 2025 17:34

nirs reviewed Jan 13, 2025

View reviewed changes

tamird closed this Jan 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup: refactor and use bicopy everywhere #2944

cleanup: refactor and use bicopy everywhere #2944

tamird commented Nov 25, 2024 •

edited

Loading

nirs left a comment

nirs commented Dec 3, 2024

tamird commented Dec 3, 2024

nirs left a comment

nirs commented Dec 3, 2024

nirs commented Dec 3, 2024

tamird commented Dec 4, 2024

jandubois commented Dec 4, 2024

balajiv113 Dec 4, 2024

tamird Dec 4, 2024

balajiv113 Dec 9, 2024

tamird Dec 9, 2024

balajiv113 Dec 9, 2024

balajiv113 Dec 10, 2024 •

edited

Loading

nirs Dec 10, 2024

nirs Dec 10, 2024

balajiv113 Dec 10, 2024

nirs Dec 10, 2024

nirs Jan 13, 2025

nirs Jan 13, 2025

nirs Jan 13, 2025

nirs Jan 13, 2025

nirs Jan 13, 2025

nirs Jan 13, 2025

nirs Jan 13, 2025

tamird commented Jan 13, 2025

cleanup: refactor and use bicopy everywhere #2944

cleanup: refactor and use bicopy everywhere #2944

Conversation

tamird commented Nov 25, 2024 • edited Loading

nirs left a comment

Choose a reason for hiding this comment

nirs commented Dec 3, 2024

tamird commented Dec 3, 2024

nirs left a comment

Choose a reason for hiding this comment

nirs commented Dec 3, 2024

nirs commented Dec 3, 2024

tamird commented Dec 4, 2024

jandubois commented Dec 4, 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

balajiv113 Dec 10, 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

Choose a reason for hiding this comment

tamird commented Jan 13, 2025

tamird commented Nov 25, 2024 •

edited

Loading

balajiv113 Dec 10, 2024 •

edited

Loading