From 06a45192fb331794dd52d22465dbe5d27c4e88f0 Mon Sep 17 00:00:00 2001 From: Chris Doherty Date: Thu, 7 Apr 2022 13:36:12 -0400 Subject: [PATCH] Generalize the hardware client interface The hardware client is used to interact with a Cacher service and the Tink service. It embodies gRPC specific types and an ambiguous All() call that are difficult to model for a Kubernetes data provider. This change alters the interface to model more generic behavior and removes gRPC specific types paving the path for implementing a Kubernetes data provider client. Signed-off-by: Chris Doherty --- hardware/client.go | 54 +++++++++++++++++--------------------- hardware/mock/mock.go | 9 +++---- http-server/http_server.go | 8 +++--- 3 files changed, 32 insertions(+), 39 deletions(-) diff --git a/hardware/client.go b/hardware/client.go index 3fc2685e..d6c1cc0a 100644 --- a/hardware/client.go +++ b/hardware/client.go @@ -13,7 +13,6 @@ import ( tinkClient "github.com/tinkerbell/tink/client" tpkg "github.com/tinkerbell/tink/pkg" tink "github.com/tinkerbell/tink/protos/hardware" - "google.golang.org/grpc" ) var ( @@ -21,14 +20,18 @@ var ( facility = flag.String("facility", env.Get("HEGEL_FACILITY", "onprem"), "The facility we are running in (mostly to connect to cacher)") ) -// Client acts as the messenger between Hegel and Cacher/Tink. +// Client defines the behaviors for interacting with hardware data providers. type Client interface { - All(ctx context.Context, opts ...grpc.CallOption) (AllClient, error) - ByIP(ctx context.Context, id string, opts ...grpc.CallOption) (Hardware, error) - Watch(ctx context.Context, id string, opts ...grpc.CallOption) (Watcher, error) -} + // IsHealthy reports whether the client is connected and can retrieve hardware data from the data provider. + IsHealthy(ctx context.Context) bool + + // ByIP retrieves hardware data by its IP address. + ByIP(ctx context.Context, ip string) (Hardware, error) -type AllClient interface{} + // Watch creates a subscription to a hardware identified by id such that updates to the hardware data are + // pushed to the stream. + Watch(ctx context.Context, id string) (Watcher, error) +} // Hardware is the interface for Cacher/Tink hardware types. type Hardware interface { @@ -102,22 +105,17 @@ func NewClient() (Client, error) { return hg, nil } -// All retrieves all the pieces of hardware stored in Cacher. -func (hg clientCacher) All(ctx context.Context, opts ...grpc.CallOption) (AllClient, error) { - in := &cacher.Empty{} - all, err := hg.client.All(ctx, in, opts...) - if err != nil { - return nil, err - } - return all, nil +func (hg clientCacher) IsHealthy(ctx context.Context) bool { + _, err := hg.client.All(ctx, &cacher.Empty{}) + return err == nil } // ByIP retrieves from Cacher the piece of hardware with the specified IP. -func (hg clientCacher) ByIP(ctx context.Context, ip string, opts ...grpc.CallOption) (Hardware, error) { +func (hg clientCacher) ByIP(ctx context.Context, ip string) (Hardware, error) { in := &cacher.GetRequest{ IP: ip, } - hw, err := hg.client.ByIP(ctx, in, opts...) + hw, err := hg.client.ByIP(ctx, in) if err != nil { return nil, err } @@ -125,11 +123,11 @@ func (hg clientCacher) ByIP(ctx context.Context, ip string, opts ...grpc.CallOpt } // Watch returns a Cacher watch client on the hardware with the specified ID. -func (hg clientCacher) Watch(ctx context.Context, id string, opts ...grpc.CallOption) (Watcher, error) { +func (hg clientCacher) Watch(ctx context.Context, id string) (Watcher, error) { in := &cacher.GetRequest{ ID: id, } - w, err := hg.client.Watch(ctx, in, opts...) + w, err := hg.client.Watch(ctx, in) if err != nil { return nil, err } @@ -137,21 +135,17 @@ func (hg clientCacher) Watch(ctx context.Context, id string, opts ...grpc.CallOp } // All retrieves all the pieces of hardware stored in Cacher. -func (hg clientTinkerbell) All(ctx context.Context, opts ...grpc.CallOption) (AllClient, error) { - in := &tink.Empty{} - all, err := hg.client.All(ctx, in, opts...) - if err != nil { - return nil, err - } - return all, nil +func (hg clientTinkerbell) IsHealthy(ctx context.Context) bool { + _, err := hg.client.All(ctx, &tink.Empty{}) + return err == nil } // ByIP retrieves from Tink the piece of hardware with the specified IP. -func (hg clientTinkerbell) ByIP(ctx context.Context, ip string, opts ...grpc.CallOption) (Hardware, error) { +func (hg clientTinkerbell) ByIP(ctx context.Context, ip string) (Hardware, error) { in := &tink.GetRequest{ Ip: ip, } - hw, err := hg.client.ByIP(ctx, in, opts...) + hw, err := hg.client.ByIP(ctx, in) if err != nil { return nil, err } @@ -159,11 +153,11 @@ func (hg clientTinkerbell) ByIP(ctx context.Context, ip string, opts ...grpc.Cal } // Watch returns a Tink watch client on the hardware with the specified ID. -func (hg clientTinkerbell) Watch(ctx context.Context, id string, opts ...grpc.CallOption) (Watcher, error) { +func (hg clientTinkerbell) Watch(ctx context.Context, id string) (Watcher, error) { in := &tink.GetRequest{ Id: id, } - w, err := hg.client.DeprecatedWatch(ctx, in, opts...) + w, err := hg.client.DeprecatedWatch(ctx, in) if err != nil { return nil, err } diff --git a/hardware/mock/mock.go b/hardware/mock/mock.go index 3cc9ec83..d96ae7e4 100644 --- a/hardware/mock/mock.go +++ b/hardware/mock/mock.go @@ -8,7 +8,6 @@ import ( "github.com/packethost/cacher/protos/cacher" "github.com/tinkerbell/hegel/hardware" - "google.golang.org/grpc" ) // HardwareClient is a mock implementation of the hardwareGetter interface @@ -17,8 +16,8 @@ type HardwareClient struct { Data string } -func (hg HardwareClient) All(_ context.Context, _ ...grpc.CallOption) (hardware.AllClient, error) { - return nil, nil +func (hg HardwareClient) IsHealthy(context.Context) bool { + return true } // ByIP mocks the retrieval a piece of hardware from tink/cacher by ip @@ -26,7 +25,7 @@ func (hg HardwareClient) All(_ context.Context, _ ...grpc.CallOption) (hardware. // having to parse the (mock) hardware data `HardwareClient.Data`, the process has been simplified to only match with the constant `UserIP`. // Given any other IP inside the get request, ByIP will return an empty piece of hardware regardless of whether or not the IP // actually matches the IP inside `Data`. -func (hg HardwareClient) ByIP(_ context.Context, ip string, _ ...grpc.CallOption) (hardware.Hardware, error) { +func (hg HardwareClient) ByIP(_ context.Context, ip string) (hardware.Hardware, error) { var hw hardware.Hardware dataModelVersion := os.Getenv("DATA_MODEL_VERSION") switch dataModelVersion { @@ -52,7 +51,7 @@ func (hg HardwareClient) ByIP(_ context.Context, ip string, _ ...grpc.CallOption return hw, nil } -func (hg HardwareClient) Watch(_ context.Context, _ string, _ ...grpc.CallOption) (hardware.Watcher, error) { +func (hg HardwareClient) Watch(context.Context, string) (hardware.Watcher, error) { return nil, nil } diff --git a/http-server/http_server.go b/http-server/http_server.go index 08ce128a..6e0c9891 100644 --- a/http-server/http_server.go +++ b/http-server/http_server.go @@ -99,9 +99,9 @@ func checkHardwareClientHealth() { // this will have to do. // Note that we don't do anything with the stream (we don't read from it) var isHardwareClientAvailableTemp bool - ctx, cancel := context.WithCancel(context.Background()) - _, err := hegelServer.HardwareClient().All(ctx) // checks for tink health as well - if err == nil { + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + if hegelServer.HardwareClient().IsHealthy(ctx) { isHardwareClientAvailableTemp = true } cancel() @@ -118,6 +118,6 @@ func checkHardwareClientHealth() { metrics.CacherConnected.Set(0) metrics.CacherHealthcheck.WithLabelValues("false").Inc() metrics.Errors.WithLabelValues("cacher", "healthcheck").Inc() - logger.With("status", isHardwareClientAvailableTemp).Error(err) + logger.With("status", isHardwareClientAvailableTemp).Error(errors.New("client reported unhealthy")) } }