From eea3ef3c35041711cf61ed12ee2da965322e7ae8 Mon Sep 17 00:00:00 2001 From: Cody Baker Date: Wed, 13 Mar 2019 12:38:17 -0400 Subject: [PATCH] address PR comments - rework tagVolumes for clarity and to fix accidental context reuse in after cancel - use flag rather than environmental var - rename doVolTag to doTag in anticipation of upcoming snapshot tagging - check errors on tagVolumes --- cmd/do-csi-plugin/main.go | 3 ++- driver/controller.go | 52 +++++++++++++++++++++------------------ driver/controller_test.go | 2 +- driver/driver.go | 10 +++----- driver/driver_test.go | 4 +-- 5 files changed, 36 insertions(+), 35 deletions(-) diff --git a/cmd/do-csi-plugin/main.go b/cmd/do-csi-plugin/main.go index 6667be88..323b001c 100644 --- a/cmd/do-csi-plugin/main.go +++ b/cmd/do-csi-plugin/main.go @@ -30,6 +30,7 @@ func main() { endpoint = flag.String("endpoint", "unix:///var/lib/kubelet/plugins/"+driver.DriverName+"/csi.sock", "CSI endpoint") token = flag.String("token", "", "DigitalOcean access token") url = flag.String("url", "https://api.digitalocean.com/", "DigitalOcean API URL") + doTag = flag.String("do-tag", "", "Tag DigitalOcean volumes on Create/Attach") version = flag.Bool("version", false, "Print the version and exit.") ) flag.Parse() @@ -39,7 +40,7 @@ func main() { os.Exit(0) } - drv, err := driver.NewDriver(*endpoint, *token, *url) + drv, err := driver.NewDriver(*endpoint, *token, *url, *doTag) if err != nil { log.Fatalln(err) } diff --git a/driver/controller.go b/driver/controller.go index 5c6e8b43..71e464ba 100644 --- a/driver/controller.go +++ b/driver/controller.go @@ -153,8 +153,8 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) SizeGigaBytes: size / GB, } - if d.doVolTag != "" { - volumeReq.Tags = append(volumeReq.Tags, d.doVolTag) + if d.doTag != "" { + volumeReq.Tags = append(volumeReq.Tags, d.doTag) } ll.Info("checking volume limit") @@ -261,7 +261,11 @@ func (d *Driver) ControllerPublishVolume(ctx context.Context, req *csi.Controlle return nil, err } - d.tagVolume(ctx, vol) + err = d.tagVolume(ctx, vol) + if err != nil { + ll.Errorf("error tagging volume: %s", err) + return nil, status.Errorf(codes.Internal, "failed to tag volume") + } // check if droplet exist before trying to attach the volume to the droplet _, resp, err = d.droplets.Get(ctx, dropletID) @@ -974,9 +978,9 @@ func validateCapabilities(caps []*csi.VolumeCapability) bool { return supported } -func (d *Driver) tagVolume(ctx context.Context, vol *godo.Volume) error { +func (d *Driver) tagVolume(parentCtx context.Context, vol *godo.Volume) error { for _, tag := range vol.Tags { - if tag == d.doVolTag { + if tag == d.doTag { return nil } } @@ -990,29 +994,29 @@ func (d *Driver) tagVolume(ctx context.Context, vol *godo.Volume) error { }, } - ctx, cancel := context.WithTimeout(ctx, doAPITimeout) - resp, err := d.tags.TagResources(ctx, d.doVolTag, tagReq) - cancel() + ctx, cancel := context.WithTimeout(parentCtx, doAPITimeout) + defer cancel() + resp, err := d.tags.TagResources(ctx, d.doTag, tagReq) - if resp != nil && resp.StatusCode == http.StatusNotFound { - // If the tag doesn't exist, create it and retry. - err = d.createTag(ctx) - if err != nil { - return err - } - ctx, cancel = context.WithTimeout(ctx, doAPITimeout) - resp, err = d.tags.TagResources(ctx, d.doVolTag, tagReq) - cancel() + if err == nil || (resp != nil && resp.StatusCode != http.StatusNotFound) { + // either success or irrecoverable failure + return err } - return err -} - -func (d *Driver) createTag(ctx context.Context) error { - ctx, cancel := context.WithTimeout(ctx, doAPITimeout) + // godo.TagsService returns 404 if the tag has not yet been + // created, if that happens we need to create the tag + // and then retry tagging the volume resource. + ctx, cancel = context.WithTimeout(parentCtx, doAPITimeout) defer cancel() - _, _, err := d.tags.Create(ctx, &godo.TagCreateRequest{ - Name: d.doVolTag, + _, _, err = d.tags.Create(parentCtx, &godo.TagCreateRequest{ + Name: d.doTag, }) + if err != nil { + return err + } + + ctx, cancel = context.WithTimeout(parentCtx, doAPITimeout) + defer cancel() + _, err = d.tags.TagResources(ctx, d.doTag, tagReq) return err } diff --git a/driver/controller_test.go b/driver/controller_test.go index 1fa39cbf..858a0814 100644 --- a/driver/controller_test.go +++ b/driver/controller_test.go @@ -76,7 +76,7 @@ func TestTagger(t *testing.T) { exists: tc.tagExists, } driver := &Driver{ - doVolTag: tag, + doTag: tag, tags: tagService, } diff --git a/driver/driver.go b/driver/driver.go index 52b05082..6a1a42f3 100644 --- a/driver/driver.go +++ b/driver/driver.go @@ -42,10 +42,6 @@ const ( ) var ( - // doVolTagEnv describes an environmental var. If present, the controller - // will apply the specified Digital Ocean tag to volumes during create/attach. - doVolTagEnv = "DIGITALOCEAN_TAG_VOLUMES" - gitTreeState = "not a git tree" commit string version string @@ -61,7 +57,7 @@ type Driver struct { endpoint string nodeId string region string - doVolTag string + doTag string srv *grpc.Server log *logrus.Entry @@ -83,7 +79,7 @@ type Driver struct { // NewDriver returns a CSI plugin that contains the necessary gRPC // interfaces to interact with Kubernetes over unix domain sockets for // managaing DigitalOcean Block Storage -func NewDriver(ep, token, url string) (*Driver, error) { +func NewDriver(ep, token, url, doTag string) (*Driver, error) { tokenSource := oauth2.StaticTokenSource(&oauth2.Token{ AccessToken: token, }) @@ -112,7 +108,7 @@ func NewDriver(ep, token, url string) (*Driver, error) { }) return &Driver{ - doVolTag: os.Getenv(doVolTagEnv), + doTag: doTag, endpoint: ep, nodeId: nodeId, region: region, diff --git a/driver/driver_test.go b/driver/driver_test.go index 9182f6d3..787f0bf5 100644 --- a/driver/driver_test.go +++ b/driver/driver_test.go @@ -59,7 +59,7 @@ func TestDriverSuite(t *testing.T) { } nodeID := 987654 - doVolTag := "k8s:cluster-id" + doTag := "k8s:cluster-id" volumes := make(map[string]*godo.Volume, 0) snapshots := make(map[string]*godo.Snapshot, 0) droplets := map[int]*godo.Droplet{ @@ -71,7 +71,7 @@ func TestDriverSuite(t *testing.T) { driver := &Driver{ endpoint: endpoint, nodeId: strconv.Itoa(nodeID), - doVolTag: doVolTag, + doTag: doTag, region: "nyc3", mounter: &fakeMounter{}, log: logrus.New().WithField("test_enabed", true),