Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Connect function #11

Merged
merged 8 commits into from
Feb 7, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion Gopkg.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

71 changes: 71 additions & 0 deletions connection/connection.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
package connection
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need the usual copyright and license boiler plate code at the beginning of this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


import (
"context"
"net"
"strings"
"time"

"github.com/golang/glog"
"github.com/kubernetes-csi/csi-lib-utils/protosanitizer"
"google.golang.org/grpc"
)

const (
// Interval of logging connection errors
connectionLoggingInterval = 10 * time.Second
)

// Connect opens insecure gRPC connection to a CSI driver. Address must be either absolute path to a socket file
// or have format '<protocol>://', following gRPC name resolution mechanism at https://github.com/grpc/grpc/blob/master/doc/naming.md.
// The function tries to connect indefinitely every second until it connects. The function automatically adds
// interceptor for gRPC message logging.
func Connect(address string, dialOptions ...grpc.DialOption) (*grpc.ClientConn, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worthwhile to pass in a context? Then a caller can decide to abort after a certain time (context.WithTimeout) or run Connect in a goroutine and cancel the operation (context.WithCancel).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, the point of this function is to have the same behavior in all sidecar containers. Adding optional parameters works against this unification.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can have the same behavior in all sidecars by calling the function the same way, while still writing the underlying function in a more flexible way. But I get your point.

Let me add another reason for passing in a context: when discussing glog vs. klog, it was mentioned that it would be good to pass in a logger via the context instead of using the same global instance. If we had that now, a unit test could select a mock logger to test that the warning messages are printed.

But we don't have that now, so I'm fine with leaving out the context and adding it when really needed. We can deal with the API change in the sidecars on a case-by-case basis, should we do it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we move to klog, a test can add its own logging target to check that the right things are logged. IMO, the less options we offer to callers the better its usage is going to be. I removed dialOptions completely as separate commit.

dialOptions = append(dialOptions,
grpc.WithInsecure(), // Don't use TLS, it's usually local Unix domain socket in a container.
grpc.WithBackoffMaxDelay(time.Second), // Retry every second after failure.
grpc.WithBlock(), // Block until connection succeeds.
grpc.WithUnaryInterceptor(LogGRPC), // Log all messages.
)
if strings.HasPrefix(address, "/") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't handle an explicit unix:/// prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dial() handles unix:/// just fine without any special dialer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? grpc/grpc-go#1741 was asking for this and then the feature request moved to grpc/grpc-go#1911, which is still open.

I was under the impression that the custom dialer was used specifically to work around that shortcoming in gRPC. Perhaps I misunderstood. 🤷‍♂️

Looks like a good test case, don't you think? 😉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does indeed work. Then I don't understand what those open gRPC issues are about. It looks like the custom dialer is needed because an absolute path without the unix:/// prefix will not be treated as a Unix domain socket.

I'm still a bit wary about not always using that dialer, though. It creates two different code paths in our Connect function which (theoretically) could have different behavior. In practice I've not found any relevant difference, but the slow tests in PR jsafrane#1 now only cover the case with an absolute path. I only tried the unix:/// variant of those by editing the test source code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added code that removes custom dialer and uses unix:// for all sockets. This changes behavior of existing sidecar containers that used custom dialer in old releases, IMO it should be fine.

// It looks like filesystem path.
dialOptions = append(dialOptions, grpc.WithDialer(func(addr string, timeout time.Duration) (net.Conn, error) {
return net.DialTimeout("unix", addr, timeout)
}))
}
glog.Infof("Connecting to %s", address)

// Connect in background.
var conn *grpc.ClientConn
var err error
ready := make(chan bool)
go func() {
conn, err = grpc.Dial(address, dialOptions...)
close(ready)
}()

// Log error every connectionLoggingInterval
ticker := time.NewTicker(connectionLoggingInterval)
defer ticker.Stop()

// Wait until Dial() succeeds.
for {
select {
case <-ticker.C:
glog.Warningf("Still connecting to %s", address)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

glog has been replaced in Kubernetes. We should do the same in kubernetes-csi. Can you replace with k8s.io/klog here and in the rest of the new code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should move all repos to klog in one (short) burst. Is it the right time?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so:

  • klog became stable in Kubernetes 1.13
  • if a sidecar hasn't been updated to utility code from that release, it's time to update now
  • the master branch is open for more intrusive changes like this and we have time before the next release because there are no plans for one yet (in particular we don't need a major release for the upcoming Kubernetes 1.14)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but we should run it through CSI WG meeting. And add common logging initialization function to this library (as separate PR), so all the sidecar containers have the same cmdline arguments for logging.


case <-ready:
return conn, err
}
}
}

// LogGRPC is gPRC unary interceptor for logging of CSI messages at level 5. It removes any secrets from the message.
func LogGRPC(ctx context.Context, method string, req, reply interface{}, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error {
glog.V(5).Infof("GRPC call: %s", method)
glog.V(5).Infof("GRPC request: %s", protosanitizer.StripSecrets(req))
err := invoker(ctx, method, req, reply, cc, opts...)
glog.V(5).Infof("GRPC response: %s", protosanitizer.StripSecrets(reply))
glog.V(5).Infof("GRPC error: %v", err)
return err
}
191 changes: 191 additions & 0 deletions vendor/github.com/golang/glog/LICENSE

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading