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.

Sign up for GitHub

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

gcp: support additionalDisks for GCP instance #3477

Merged
merged 1 commit into from
May 24, 2023
Merged

gcp: support additionalDisks for GCP instance #3477

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
65 changes: 60 additions & 5 deletions mantle/platform/api/gcloud/compute.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@ package gcloud
import (
"crypto/rand"
"fmt"
"strconv"
"strings"
"time"

"github.com/coreos/coreos-assembler/mantle/platform"
"github.com/coreos/coreos-assembler/mantle/util"
"golang.org/x/crypto/ssh/agent"
"google.golang.org/api/compute/v1"
Expand All @@ -33,8 +35,48 @@ func (a *API) vmname() string {
return fmt.Sprintf("%s-%x", a.options.BaseName, b)
}

// ["5G:interface=NVME"], by default the disk type is local-ssd
func ParseDiskSpec(spec string, zone string) (*compute.AttachedDisk, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine for now to have this here, but we should ideally dedupe this with the one in qemu.go. We can have a generic parser that returns the size in gigabytes as a uint and any options in a map[string]string.

But again, I'm OK to duplicate for now! Maybe let's add a comment in that case?

Copy link
Member Author

@HuijingHei HuijingHei May 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can I understand that it should return like map[string]string{"size": "5G", "interface": "NVME"} after parsing ? Might do this in the later patch.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking more something like:

package util

func ParseDiskSpec(spec string) (uint, map[string]string, error) {
    ...
}

?

Where the uint is the disk size in gigabytes. Since it's a required argument, we can enforce proper typing for it. Also, some cloud API bindings may take it directly as an integer (like GCP here) too which is nice. Others might require a string but suffixed differently (e.g. "GB" vs "G"), which would have to live in the cloud-specific code.

Copy link
Member

Choose a reason for hiding this comment

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

Optional, but I think it'd be pretty easy to have a unit test or two for this function.

split := strings.Split(spec, ":")
var diskinterface string
var disksize string
if len(split) == 1 {
disksize = split[0]
} else if len(split) == 2 {
disksize = split[0]
for _, opt := range strings.Split(split[1], ",") {
if strings.HasPrefix(opt, "interface=") {
diskinterface = strings.TrimPrefix(opt, "interface=")
if diskinterface == "" || (diskinterface != "NVME" && diskinterface != "SCSI") {
return nil, fmt.Errorf("invalid interface opt %s", opt)
}
} else {
return nil, fmt.Errorf("unknown disk option %s", opt)
}
}
} else {
return nil, fmt.Errorf("invalid disk spec %s", spec)
}
disksize = strings.TrimSuffix(disksize, "G")
size, err := strconv.ParseInt(disksize, 10, 32)
if err != nil {
return nil, fmt.Errorf("invalid size opt %s: %w", disksize, err)
}

return &compute.AttachedDisk{
AutoDelete: true,
Boot: false,
Type: "SCRATCH",
Interface: diskinterface,
InitializeParams: &compute.AttachedDiskInitializeParams{
DiskType: "/zones/" + zone + "/diskTypes/local-ssd",
DiskSizeGb: size,
},
}, nil
}

// Taken from: https://github.com/golang/build/blob/master/buildlet/gce.go
func (a *API) mkinstance(userdata, name string, keys []*agent.Key, useServiceAcct bool) *compute.Instance {
func (a *API) mkinstance(userdata, name string, keys []*agent.Key, opts platform.MachineOptions, useServiceAcct bool) (*compute.Instance, error) {
mantle := "mantle"
metadataItems := []*compute.MetadataItems{
{
Expand Down Expand Up @@ -120,14 +162,27 @@ func (a *API) mkinstance(userdata, name string, keys []*agent.Key, useServiceAcc
OnHostMaintenance: "TERMINATE",
}
}
return instance

// attach aditional disk
if len(opts.AdditionalDisks) > 0 {
for _, spec := range opts.AdditionalDisks {
Comment on lines +166 to +167
Copy link
Member

Choose a reason for hiding this comment

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

Completely minor nit, but is the len check here needed? Won't the loop be a no-op if the length is zero anyways?

Copy link
Member

Choose a reason for hiding this comment

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

(With stuff like this, feel free to merge as is. Minor fixups like this can be done as a followup alongside other cleanups)

plog.Debugf("Parsing disk spec %q\n", spec)
disk, err := ParseDiskSpec(spec, a.options.Zone)
if err != nil {
return nil, fmt.Errorf("failed to parse spec %s: %w", spec, err)
}
instance.Disks = append(instance.Disks, disk)
}
}
return instance, nil
}

// CreateInstance creates a Google Compute Engine instance.
func (a *API) CreateInstance(userdata string, keys []*agent.Key, useServiceAcct bool) (*compute.Instance, error) {
func (a *API) CreateInstance(userdata string, keys []*agent.Key, opts platform.MachineOptions, useServiceAcct bool) (*compute.Instance, error) {
name := a.vmname()
inst := a.mkinstance(userdata, name, keys, useServiceAcct)
inst, err := a.mkinstance(userdata, name, keys, opts, useServiceAcct)
if err != nil {
return nil, fmt.Errorf("failed to create instance %q: %w", name, err)
}

plog.Debugf("Creating instance %q", name)

Expand Down
5 changes: 1 addition & 4 deletions mantle/platform/machine/gcloud/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,6 @@ func (gc *cluster) NewMachine(userdata *conf.UserData) (platform.Machine, error)
}

func (gc *cluster) NewMachineWithOptions(userdata *conf.UserData, options platform.MachineOptions) (platform.Machine, error) {
if len(options.AdditionalDisks) > 0 {
return nil, errors.New("platform gcp does not yet support additional disks")
}
if options.MultiPathDisk {
return nil, errors.New("platform gcp does not support multipathed disks")
}
Expand Down Expand Up @@ -69,7 +66,7 @@ func (gc *cluster) NewMachineWithOptions(userdata *conf.UserData, options platfo
}
}

instance, err := gc.flight.api.CreateInstance(conf.String(), keys, !gc.RuntimeConf().NoInstanceCreds)
instance, err := gc.flight.api.CreateInstance(conf.String(), keys, options, !gc.RuntimeConf().NoInstanceCreds)
if err != nil {
return nil, err
}
Expand Down