From 0c1749cecde761b9afbdbc4893bce17cca478f3f Mon Sep 17 00:00:00 2001 From: Diego Bonfigli Date: Sat, 8 Jul 2023 15:06:32 +0200 Subject: [PATCH] fix(cloudprovider/exteranlgrpc): properly handle unimplemented methods --- .../wrapper/wrapper.go | 14 ++++++ .../externalgrpc_cloud_provider.go | 10 +++++ .../externalgrpc_cloud_provider_test.go | 38 ++++++++++++++++ .../externalgrpc/externalgrpc_node_group.go | 10 +++++ .../externalgrpc_node_group_test.go | 43 +++++++++++++++++++ .../externalgrpc/protos/externalgrpc.pb.go | 4 +- .../externalgrpc/protos/externalgrpc.proto | 11 +++-- .../protos/externalgrpc_grpc.pb.go | 24 ++++++----- 8 files changed, 136 insertions(+), 18 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/externalgrpc/examples/external-grpc-cloud-provider-service/wrapper/wrapper.go b/cluster-autoscaler/cloudprovider/externalgrpc/examples/external-grpc-cloud-provider-service/wrapper/wrapper.go index 33a139785e67..1f83af75bae6 100644 --- a/cluster-autoscaler/cloudprovider/externalgrpc/examples/external-grpc-cloud-provider-service/wrapper/wrapper.go +++ b/cluster-autoscaler/cloudprovider/externalgrpc/examples/external-grpc-cloud-provider-service/wrapper/wrapper.go @@ -20,6 +20,8 @@ import ( "context" "fmt" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/anypb" apiv1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -113,6 +115,9 @@ func (w *Wrapper) PricingNodePrice(_ context.Context, req *protos.PricingNodePri model, err := w.provider.Pricing() if err != nil { + if err == cloudprovider.ErrNotImplemented { + return nil, status.Error(codes.Unimplemented, err.Error()) + } return nil, err } reqNode := req.GetNode() @@ -136,6 +141,9 @@ func (w *Wrapper) PricingPodPrice(_ context.Context, req *protos.PricingPodPrice model, err := w.provider.Pricing() if err != nil { + if err == cloudprovider.ErrNotImplemented { + return nil, status.Error(codes.Unimplemented, err.Error()) + } return nil, err } reqPod := req.GetPod() @@ -326,6 +334,9 @@ func (w *Wrapper) NodeGroupTemplateNodeInfo(_ context.Context, req *protos.NodeG } info, err := ng.TemplateNodeInfo() if err != nil { + if err == cloudprovider.ErrNotImplemented { + return nil, status.Error(codes.Unimplemented, err.Error()) + } return nil, err } return &protos.NodeGroupTemplateNodeInfoResponse{ @@ -354,6 +365,9 @@ func (w *Wrapper) NodeGroupGetOptions(_ context.Context, req *protos.NodeGroupAu } opts, err := ng.GetOptions(defaults) if err != nil { + if err == cloudprovider.ErrNotImplemented { + return nil, status.Error(codes.Unimplemented, err.Error()) + } return nil, err } if opts == nil { diff --git a/cluster-autoscaler/cloudprovider/externalgrpc/externalgrpc_cloud_provider.go b/cluster-autoscaler/cloudprovider/externalgrpc/externalgrpc_cloud_provider.go index fa5554483799..44cfce4e2cbe 100644 --- a/cluster-autoscaler/cloudprovider/externalgrpc/externalgrpc_cloud_provider.go +++ b/cluster-autoscaler/cloudprovider/externalgrpc/externalgrpc_cloud_provider.go @@ -27,7 +27,9 @@ import ( "time" "google.golang.org/grpc" + "google.golang.org/grpc/codes" "google.golang.org/grpc/credentials" + "google.golang.org/grpc/status" "gopkg.in/yaml.v2" apiv1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" @@ -158,6 +160,10 @@ func (m *pricingModel) NodePrice(node *apiv1.Node, startTime time.Time, endTime EndTime: &end, }) if err != nil { + st, ok := status.FromError(err) + if ok && st.Code() == codes.Unimplemented { + return 0, cloudprovider.ErrNotImplemented + } klog.V(1).Infof("Error on gRPC call PricingNodePrice: %v", err) return 0, err } @@ -178,6 +184,10 @@ func (m *pricingModel) PodPrice(pod *apiv1.Pod, startTime time.Time, endTime tim EndTime: &end, }) if err != nil { + st, ok := status.FromError(err) + if ok && st.Code() == codes.Unimplemented { + return 0, cloudprovider.ErrNotImplemented + } klog.V(1).Infof("Error on gRPC call PricingPodPrice: %v", err) return 0, err } diff --git a/cluster-autoscaler/cloudprovider/externalgrpc/externalgrpc_cloud_provider_test.go b/cluster-autoscaler/cloudprovider/externalgrpc/externalgrpc_cloud_provider_test.go index a123214cb6c1..0697372c61eb 100644 --- a/cluster-autoscaler/cloudprovider/externalgrpc/externalgrpc_cloud_provider_test.go +++ b/cluster-autoscaler/cloudprovider/externalgrpc/externalgrpc_cloud_provider_test.go @@ -23,8 +23,11 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/anypb" apiv1 "k8s.io/api/core/v1" + "k8s.io/autoscaler/cluster-autoscaler/cloudprovider" "k8s.io/autoscaler/cluster-autoscaler/cloudprovider/externalgrpc/protos" ) @@ -286,6 +289,23 @@ func TestCloudProvider_Pricing(t *testing.T) { _, err = model.NodePrice(apiv1Node3, time.Time{}, time.Time{}) assert.Error(t, err) + // test notImplemented for NodePrice + m.On( + "PricingNodePrice", mock.Anything, mock.MatchedBy(func(req *protos.PricingNodePriceRequest) bool { + return req.Node.Name == "node4" + }), + ).Return( + &protos.PricingNodePriceResponse{}, + status.Error(codes.Unimplemented, "mock error"), + ) + + apiv1Node4 := &apiv1.Node{} + apiv1Node4.Name = "node4" + + _, err = model.NodePrice(apiv1Node4, time.Time{}, time.Time{}) + assert.Error(t, err) + assert.Equal(t, cloudprovider.ErrNotImplemented, err) + // test correct PodPrice call m.On( "PricingPodPrice", mock.Anything, mock.MatchedBy(func(req *protos.PricingPodPriceRequest) bool { @@ -333,6 +353,24 @@ func TestCloudProvider_Pricing(t *testing.T) { _, err = model.PodPrice(apiv1Pod3, time.Time{}, time.Time{}) assert.Error(t, err) + + // test notImplemented for PodPrice + m.On( + "PricingPodPrice", mock.Anything, mock.MatchedBy(func(req *protos.PricingPodPriceRequest) bool { + return req.Pod.Name == "pod4" + }), + ).Return( + &protos.PricingPodPriceResponse{}, + status.Error(codes.Unimplemented, "mock error"), + ) + + apiv1Pod4 := &apiv1.Pod{} + apiv1Pod4.Name = "pod4" + + _, err = model.PodPrice(apiv1Pod4, time.Time{}, time.Time{}) + assert.Error(t, err) + assert.Equal(t, cloudprovider.ErrNotImplemented, err) + } func TestCloudProvider_GPULabel(t *testing.T) { diff --git a/cluster-autoscaler/cloudprovider/externalgrpc/externalgrpc_node_group.go b/cluster-autoscaler/cloudprovider/externalgrpc/externalgrpc_node_group.go index ae8752af1897..f45a87d7bcc8 100644 --- a/cluster-autoscaler/cloudprovider/externalgrpc/externalgrpc_node_group.go +++ b/cluster-autoscaler/cloudprovider/externalgrpc/externalgrpc_node_group.go @@ -20,6 +20,8 @@ import ( "context" "sync" + "google.golang.org/grpc/status" + "google.golang.org/grpc/codes" apiv1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/autoscaler/cluster-autoscaler/cloudprovider" @@ -206,6 +208,10 @@ func (n *NodeGroup) TemplateNodeInfo() (*schedulerframework.NodeInfo, error) { Id: n.id, }) if err != nil { + st, ok := status.FromError(err) + if ok && st.Code() == codes.Unimplemented { + return nil, cloudprovider.ErrNotImplemented + } klog.V(1).Infof("Error on gRPC call NodeGroupTemplateNodeInfo: %v", err) return nil, err } @@ -266,6 +272,10 @@ func (n *NodeGroup) GetOptions(defaults config.NodeGroupAutoscalingOptions) (*co }, }) if err != nil { + st, ok := status.FromError(err) + if ok && st.Code() == codes.Unimplemented { + return nil, cloudprovider.ErrNotImplemented + } klog.V(1).Infof("Error on gRPC call NodeGroupGetOptions: %v", err) return nil, err } diff --git a/cluster-autoscaler/cloudprovider/externalgrpc/externalgrpc_node_group_test.go b/cluster-autoscaler/cloudprovider/externalgrpc/externalgrpc_node_group_test.go index 51feebc9d803..19ffd6b382a9 100644 --- a/cluster-autoscaler/cloudprovider/externalgrpc/externalgrpc_node_group_test.go +++ b/cluster-autoscaler/cloudprovider/externalgrpc/externalgrpc_node_group_test.go @@ -23,6 +23,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" apiv1 "k8s.io/api/core/v1" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/autoscaler/cluster-autoscaler/cloudprovider" @@ -208,6 +210,27 @@ func TestCloudProvider_TemplateNodeInfo(t *testing.T) { _, err = ng4.TemplateNodeInfo() assert.Error(t, err) + // test notImplemented + m.On( + "NodeGroupTemplateNodeInfo", mock.Anything, mock.MatchedBy(func(req *protos.NodeGroupTemplateNodeInfoRequest) bool { + return req.Id == "nodeGroup5" + }), + ).Return( + &protos.NodeGroupTemplateNodeInfoResponse{ + NodeInfo: nil, + }, + status.Error(codes.Unimplemented, "mock error"), + ).Once() + + ng5 := NodeGroup{ + id: "nodeGroup5", + client: client, + } + + _, err = ng5.TemplateNodeInfo() + assert.Error(t, err) + assert.Equal(t, cloudprovider.ErrNotImplemented, err) + } func TestCloudProvider_GetOptions(t *testing.T) { @@ -285,6 +308,26 @@ func TestCloudProvider_GetOptions(t *testing.T) { opts, err = ng3.GetOptions(defaultsOpts) assert.NoError(t, err) assert.Nil(t, opts) + + // test notImplemented + m.On( + "NodeGroupGetOptions", mock.Anything, mock.MatchedBy(func(req *protos.NodeGroupAutoscalingOptionsRequest) bool { + return req.Id == "nodeGroup4" + }), + ).Return( + &protos.NodeGroupAutoscalingOptionsResponse{}, + status.Error(codes.Unimplemented, "mock error"), + ) + + ng4 := NodeGroup{ + id: "nodeGroup4", + client: client, + } + + _, err = ng4.GetOptions(defaultsOpts) + assert.Error(t, err) + assert.Equal(t, cloudprovider.ErrNotImplemented, err) + } func TestCloudProvider_TargetSize(t *testing.T) { diff --git a/cluster-autoscaler/cloudprovider/externalgrpc/protos/externalgrpc.pb.go b/cluster-autoscaler/cloudprovider/externalgrpc/protos/externalgrpc.pb.go index e815b88cc1ad..ac1cc70e9453 100644 --- a/cluster-autoscaler/cloudprovider/externalgrpc/protos/externalgrpc.pb.go +++ b/cluster-autoscaler/cloudprovider/externalgrpc/protos/externalgrpc.pb.go @@ -16,8 +16,8 @@ limitations under the License. // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.26.0 -// protoc v3.19.1 +// protoc-gen-go v1.28.1 +// protoc v3.15.8 // source: cloudprovider/externalgrpc/protos/externalgrpc.proto package protos diff --git a/cluster-autoscaler/cloudprovider/externalgrpc/protos/externalgrpc.proto b/cluster-autoscaler/cloudprovider/externalgrpc/protos/externalgrpc.proto index f83022f5b9a3..aa43ffd95669 100644 --- a/cluster-autoscaler/cloudprovider/externalgrpc/protos/externalgrpc.proto +++ b/cluster-autoscaler/cloudprovider/externalgrpc/protos/externalgrpc.proto @@ -41,13 +41,13 @@ service CloudProvider { // PricingNodePrice returns a theoretical minimum price of running a node for // a given period of time on a perfectly matching machine. - // Implementation optional. + // Implementation optional: if so should return error code 12 Unimplemented rpc PricingNodePrice(PricingNodePriceRequest) returns (PricingNodePriceResponse) {} // PricingPodPrice returns a theoretical minimum price of running a pod for a given // period of time on a perfectly matching machine. - // Implementation optional. + // Implementation optional: if so should return error code 12 Unimplemented rpc PricingPodPrice(PricingPodPriceRequest) returns (PricingPodPriceResponse) {} @@ -103,13 +103,13 @@ service CloudProvider { // NodeGroupTemplateNodeInfo returns a structure of an empty (as if just started) node, // with all of the labels, capacity and allocatable information. This will be used in // scale-up simulations to predict what would a new node look like if a node group was expanded. - // Implementation optional. + // Implementation optional: if so should return error code 12 Unimplemented rpc NodeGroupTemplateNodeInfo(NodeGroupTemplateNodeInfoRequest) returns (NodeGroupTemplateNodeInfoResponse) {} // GetOptions returns NodeGroupAutoscalingOptions that should be used for this particular - // NodeGroup. Returning a grpc error will result in using default options. - // Implementation optional. + // NodeGroup. + // Implementation optional: if so should return error code 12 Unimplemented rpc NodeGroupGetOptions(NodeGroupAutoscalingOptionsRequest) returns (NodeGroupAutoscalingOptionsResponse) {} } @@ -368,4 +368,3 @@ message NodeGroupAutoscalingOptionsResponse { // autoscaling options for the requested node. NodeGroupAutoscalingOptions nodeGroupAutoscalingOptions = 1; } - diff --git a/cluster-autoscaler/cloudprovider/externalgrpc/protos/externalgrpc_grpc.pb.go b/cluster-autoscaler/cloudprovider/externalgrpc/protos/externalgrpc_grpc.pb.go index 187820ef1d9c..12efa24f0261 100644 --- a/cluster-autoscaler/cloudprovider/externalgrpc/protos/externalgrpc_grpc.pb.go +++ b/cluster-autoscaler/cloudprovider/externalgrpc/protos/externalgrpc_grpc.pb.go @@ -15,6 +15,10 @@ limitations under the License. */ // Code generated by protoc-gen-go-grpc. DO NOT EDIT. +// versions: +// - protoc-gen-go-grpc v1.2.0 +// - protoc v3.15.8 +// source: cloudprovider/externalgrpc/protos/externalgrpc.proto package protos @@ -42,11 +46,11 @@ type CloudProviderClient interface { NodeGroupForNode(ctx context.Context, in *NodeGroupForNodeRequest, opts ...grpc.CallOption) (*NodeGroupForNodeResponse, error) // PricingNodePrice returns a theoretical minimum price of running a node for // a given period of time on a perfectly matching machine. - // Implementation optional. + // Implementation optional: if so should return error code 12 Unimplemented PricingNodePrice(ctx context.Context, in *PricingNodePriceRequest, opts ...grpc.CallOption) (*PricingNodePriceResponse, error) // PricingPodPrice returns a theoretical minimum price of running a pod for a given // period of time on a perfectly matching machine. - // Implementation optional. + // Implementation optional: if so should return error code 12 Unimplemented PricingPodPrice(ctx context.Context, in *PricingPodPriceRequest, opts ...grpc.CallOption) (*PricingPodPriceResponse, error) // GPULabel returns the label added to nodes with GPU resource. GPULabel(ctx context.Context, in *GPULabelRequest, opts ...grpc.CallOption) (*GPULabelResponse, error) @@ -80,11 +84,11 @@ type CloudProviderClient interface { // NodeGroupTemplateNodeInfo returns a structure of an empty (as if just started) node, // with all of the labels, capacity and allocatable information. This will be used in // scale-up simulations to predict what would a new node look like if a node group was expanded. - // Implementation optional. + // Implementation optional: if so should return error code 12 Unimplemented NodeGroupTemplateNodeInfo(ctx context.Context, in *NodeGroupTemplateNodeInfoRequest, opts ...grpc.CallOption) (*NodeGroupTemplateNodeInfoResponse, error) // GetOptions returns NodeGroupAutoscalingOptions that should be used for this particular - // NodeGroup. Returning a grpc error will result in using default options. - // Implementation optional. + // NodeGroup. + // Implementation optional: if so should return error code 12 Unimplemented NodeGroupGetOptions(ctx context.Context, in *NodeGroupAutoscalingOptionsRequest, opts ...grpc.CallOption) (*NodeGroupAutoscalingOptionsResponse, error) } @@ -243,11 +247,11 @@ type CloudProviderServer interface { NodeGroupForNode(context.Context, *NodeGroupForNodeRequest) (*NodeGroupForNodeResponse, error) // PricingNodePrice returns a theoretical minimum price of running a node for // a given period of time on a perfectly matching machine. - // Implementation optional. + // Implementation optional: if so should return error code 12 Unimplemented PricingNodePrice(context.Context, *PricingNodePriceRequest) (*PricingNodePriceResponse, error) // PricingPodPrice returns a theoretical minimum price of running a pod for a given // period of time on a perfectly matching machine. - // Implementation optional. + // Implementation optional: if so should return error code 12 Unimplemented PricingPodPrice(context.Context, *PricingPodPriceRequest) (*PricingPodPriceResponse, error) // GPULabel returns the label added to nodes with GPU resource. GPULabel(context.Context, *GPULabelRequest) (*GPULabelResponse, error) @@ -281,11 +285,11 @@ type CloudProviderServer interface { // NodeGroupTemplateNodeInfo returns a structure of an empty (as if just started) node, // with all of the labels, capacity and allocatable information. This will be used in // scale-up simulations to predict what would a new node look like if a node group was expanded. - // Implementation optional. + // Implementation optional: if so should return error code 12 Unimplemented NodeGroupTemplateNodeInfo(context.Context, *NodeGroupTemplateNodeInfoRequest) (*NodeGroupTemplateNodeInfoResponse, error) // GetOptions returns NodeGroupAutoscalingOptions that should be used for this particular - // NodeGroup. Returning a grpc error will result in using default options. - // Implementation optional. + // NodeGroup. + // Implementation optional: if so should return error code 12 Unimplemented NodeGroupGetOptions(context.Context, *NodeGroupAutoscalingOptionsRequest) (*NodeGroupAutoscalingOptionsResponse, error) mustEmbedUnimplementedCloudProviderServer() }