From 76c796ffa417529c6afa974d71b75b6329948c99 Mon Sep 17 00:00:00 2001 From: Bowei Du Date: Sat, 20 Jan 2018 00:22:09 -0800 Subject: [PATCH] GCE: invalid location was used in regional and zonal operations Location was set to the full URL instead of the short name. Adds logging to the operations to make future issues easier to debug. --- pkg/cloudprovider/providers/gce/cloud/op.go | 69 ++++++++++++++----- .../providers/gce/cloud/service.go | 20 ++++-- 2 files changed, 64 insertions(+), 25 deletions(-) diff --git a/pkg/cloudprovider/providers/gce/cloud/op.go b/pkg/cloudprovider/providers/gce/cloud/op.go index 92ee6f3f6f3..1f12d54305a 100644 --- a/pkg/cloudprovider/providers/gce/cloud/op.go +++ b/pkg/cloudprovider/providers/gce/cloud/op.go @@ -18,6 +18,9 @@ package cloud import ( "context" + "fmt" + + "github.com/golang/glog" alpha "google.golang.org/api/compute/v0.alpha" beta "google.golang.org/api/compute/v0.beta" @@ -40,6 +43,11 @@ type gaOperation struct { s *Service op *ga.Operation projectID string + key *meta.Key +} + +func (o *gaOperation) String() string { + return fmt.Sprintf("gaOperation{%q, %v}", o.projectID, o.key) } func (o *gaOperation) isDone(ctx context.Context) (bool, error) { @@ -48,13 +56,18 @@ func (o *gaOperation) isDone(ctx context.Context) (bool, error) { err error ) - switch { - case o.op.Region != "": - op, err = o.s.GA.RegionOperations.Get(o.projectID, o.op.Region, o.op.Name).Context(ctx).Do() - case o.op.Zone != "": - op, err = o.s.GA.ZoneOperations.Get(o.projectID, o.op.Zone, o.op.Name).Context(ctx).Do() - default: + switch o.key.Type() { + case meta.Regional: + op, err = o.s.GA.RegionOperations.Get(o.projectID, o.key.Region, o.op.Name).Context(ctx).Do() + glog.V(5).Infof("GA.RegionOperations.Get(%v, %v, %v) = %+v, %v; ctx = %v", o.projectID, o.key.Region, o.op.Name, op, err, ctx) + case meta.Zonal: + op, err = o.s.GA.ZoneOperations.Get(o.projectID, o.key.Zone, o.op.Name).Context(ctx).Do() + glog.V(5).Infof("GA.ZoneOperations.Get(%v, %v, %v) = %+v, %v; ctx = %v", o.projectID, o.key.Zone, o.op.Name, op, err, ctx) + case meta.Global: op, err = o.s.GA.GlobalOperations.Get(o.projectID, o.op.Name).Context(ctx).Do() + glog.V(5).Infof("GA.GlobalOperations.Get(%v, %v) = %+v, %v; ctx = %v", o.projectID, o.op.Name, op, err, ctx) + default: + return false, fmt.Errorf("invalid key type: %#v", o.key) } if err != nil { return false, err @@ -75,6 +88,11 @@ type alphaOperation struct { s *Service op *alpha.Operation projectID string + key *meta.Key +} + +func (o *alphaOperation) String() string { + return fmt.Sprintf("alphaOperation{%q, %v}", o.projectID, o.key) } func (o *alphaOperation) isDone(ctx context.Context) (bool, error) { @@ -83,13 +101,18 @@ func (o *alphaOperation) isDone(ctx context.Context) (bool, error) { err error ) - switch { - case o.op.Region != "": - op, err = o.s.Alpha.RegionOperations.Get(o.projectID, o.op.Region, o.op.Name).Context(ctx).Do() - case o.op.Zone != "": - op, err = o.s.Alpha.ZoneOperations.Get(o.projectID, o.op.Zone, o.op.Name).Context(ctx).Do() - default: + switch o.key.Type() { + case meta.Regional: + op, err = o.s.Alpha.RegionOperations.Get(o.projectID, o.key.Region, o.op.Name).Context(ctx).Do() + glog.V(5).Infof("Alpha.RegionOperations.Get(%v, %v, %v) = %+v, %v; ctx = %v", o.projectID, o.key.Region, o.op.Name, op, err, ctx) + case meta.Zonal: + op, err = o.s.Alpha.ZoneOperations.Get(o.projectID, o.key.Zone, o.op.Name).Context(ctx).Do() + glog.V(5).Infof("Alpha.ZoneOperations.Get(%v, %v, %v) = %+v, %v; ctx = %v", o.projectID, o.key.Zone, o.op.Name, op, err, ctx) + case meta.Global: op, err = o.s.Alpha.GlobalOperations.Get(o.projectID, o.op.Name).Context(ctx).Do() + glog.V(5).Infof("Alpha.GlobalOperations.Get(%v, %v) = %+v, %v; ctx = %v", o.projectID, o.op.Name, op, err, ctx) + default: + return false, fmt.Errorf("invalid key type: %#v", o.key) } if err != nil { return false, err @@ -110,6 +133,11 @@ type betaOperation struct { s *Service op *beta.Operation projectID string + key *meta.Key +} + +func (o *betaOperation) String() string { + return fmt.Sprintf("betaOperation{%q, %v}", o.projectID, o.key) } func (o *betaOperation) isDone(ctx context.Context) (bool, error) { @@ -118,13 +146,18 @@ func (o *betaOperation) isDone(ctx context.Context) (bool, error) { err error ) - switch { - case o.op.Region != "": - op, err = o.s.Beta.RegionOperations.Get(o.projectID, o.op.Region, o.op.Name).Context(ctx).Do() - case o.op.Zone != "": - op, err = o.s.Beta.ZoneOperations.Get(o.projectID, o.op.Zone, o.op.Name).Context(ctx).Do() - default: + switch o.key.Type() { + case meta.Regional: + op, err = o.s.Beta.RegionOperations.Get(o.projectID, o.key.Region, o.op.Name).Context(ctx).Do() + glog.V(5).Infof("Beta.RegionOperations.Get(%v, %v, %v) = %+v, %v; ctx = %v", o.projectID, o.key.Region, o.op.Name, op, err, ctx) + case meta.Zonal: + op, err = o.s.Beta.ZoneOperations.Get(o.projectID, o.key.Zone, o.op.Name).Context(ctx).Do() + glog.V(5).Infof("Beta.ZoneOperations.Get(%v, %v, %v) = %+v, %v; ctx = %v", o.projectID, o.key.Zone, o.op.Name, op, err, ctx) + case meta.Global: op, err = o.s.Beta.GlobalOperations.Get(o.projectID, o.op.Name).Context(ctx).Do() + glog.V(5).Infof("Beta.GlobalOperations.Get(%v, %v) = %+v, %v; ctx = %v", o.projectID, o.op.Name, op, err, ctx) + default: + return false, fmt.Errorf("invalid key type: %#v", o.key) } if err != nil { return false, err diff --git a/pkg/cloudprovider/providers/gce/cloud/service.go b/pkg/cloudprovider/providers/gce/cloud/service.go index 8a6c0a6cf95..2eb14999507 100644 --- a/pkg/cloudprovider/providers/gce/cloud/service.go +++ b/pkg/cloudprovider/providers/gce/cloud/service.go @@ -20,6 +20,8 @@ import ( "context" "fmt" + "github.com/golang/glog" + alpha "google.golang.org/api/compute/v0.alpha" beta "google.golang.org/api/compute/v0.beta" ga "google.golang.org/api/compute/v1" @@ -36,26 +38,26 @@ type Service struct { } // wrapOperation wraps a GCE anyOP in a version generic operation type. -func (g *Service) wrapOperation(anyOp interface{}) (operation, error) { +func (s *Service) wrapOperation(anyOp interface{}) (operation, error) { switch o := anyOp.(type) { case *ga.Operation: r, err := ParseResourceURL(o.SelfLink) if err != nil { return nil, err } - return &gaOperation{g, o, r.ProjectID}, nil + return &gaOperation{s, o, r.ProjectID, r.Key}, nil case *alpha.Operation: r, err := ParseResourceURL(o.SelfLink) if err != nil { return nil, err } - return &alphaOperation{g, o, r.ProjectID}, nil + return &alphaOperation{s, o, r.ProjectID, r.Key}, nil case *beta.Operation: r, err := ParseResourceURL(o.SelfLink) if err != nil { return nil, err } - return &betaOperation{g, o, r.ProjectID}, nil + return &betaOperation{s, o, r.ProjectID, r.Key}, nil default: return nil, fmt.Errorf("invalid type %T", anyOp) } @@ -64,16 +66,20 @@ func (g *Service) wrapOperation(anyOp interface{}) (operation, error) { // WaitForCompletion of a long running operation. This will poll the state of // GCE for the completion status of the given operation. genericOp can be one // of alpha, beta, ga Operation types. -func (g *Service) WaitForCompletion(ctx context.Context, genericOp interface{}) error { - op, err := g.wrapOperation(genericOp) +func (s *Service) WaitForCompletion(ctx context.Context, genericOp interface{}) error { + op, err := s.wrapOperation(genericOp) if err != nil { + glog.Errorf("wrapOperation(%+v) error: %v", genericOp, err) return err } for done, err := op.isDone(ctx); !done; done, err = op.isDone(ctx) { if err != nil { + glog.V(4).Infof("op.isDone(%v) error; op = %v, err = %v", ctx, op, err) return err } - g.RateLimiter.Accept(ctx, op.rateLimitKey()) + glog.V(5).Infof("op.isDone(%v) waiting; op = %v", ctx, op) + s.RateLimiter.Accept(ctx, op.rateLimitKey()) } + glog.V(5).Infof("op.isDone(%v) complete; op = %v", ctx, op) return nil }