Add support for setting volume size and type#76
Conversation
alexmorrisnz
force-pushed
the
volumespec
branch
from
a764414 to
f9cc169
Compare
vooon
left a comment
vooon
left a comment
There was a problem hiding this comment.
Overall looks good, but seems you haven't done go-fmt.
FYI i'm no longer working here, unsure if anyone would look...
I've ran it now, though I did keep the spacing out of the VolumeType and VolumeSize under the InstanceGroup struct to match the ones above as go fmt removed it No worries if it's not merged, just keeping it in public so others can reuse if needed. |
| if g.VolumeSize != 0 && g.VolumeType != "" { | ||
| volumeOpts := volumes.CreateOpts{ | ||
| Name: spec.Name, | ||
| Size: g.VolumeSize, | ||
| VolumeType: g.VolumeType, | ||
| ImageID: spec.ImageRef, | ||
| } | ||
| volume, err := g.client.CreateVolume(ctx, volumeOpts) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| err = g.client.WaitForVolumeStatus(ctx, volume.ID, "available") | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
There was a problem hiding this comment.
I would remove this part as well as reverting all changes in the internal/openstackclient/openstackclient.go in favour of the block_device_mapping_v2.source_type: "image". Thus changing the following part to:
servers.BlockDevice{
BootIndex: 0,
DeleteOnTermination: true,
DestinationType: servers.DestinationVolume,
SourceType: servers.SourceImage,
UUID: spec.ImageRef,
VolumeSize: g.VolumeSize,
VolumeType: g.VolumeType,
}| spec.BlockDevice = []servers.BlockDevice{ | ||
| { | ||
| BootIndex: 0, | ||
| DeleteOnTermination: true, | ||
| DestinationType: servers.DestinationVolume, | ||
| SourceType: servers.SourceVolume, | ||
| UUID: volume.ID, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
I do not think blindly overwriting the spec.BlockDevices is a good idea. I would append the block device and check within the InstanceGroup.Init if the g.ServerSpec.BlockDevices already contain a block device with a boot index set to 0 (in case that VolumeType and VolumeSize are not empty) and return an error.
|
@vooon is anyone else eligible to review this PR and, if appropriate, merge it? |
|
@ProbstDJakob i don't know. |
| Name: spec.Name, | ||
| Size: g.VolumeSize, | ||
| VolumeType: g.VolumeType, | ||
| ImageID: spec.ImageRef, |
There was a problem hiding this comment.
spec.ImageRef is possibly not known at this point and will only be available after this block ending with line 286.
There was a problem hiding this comment.
When this can be implemented, I also need this feature for our openstack project.
There was a problem hiding this comment.
FYI, you don't need to wait for this feature to use volume-backed instances, you can include block_device_mapping_v2 in the server_spec already:
https://gitlab.com/alasca.cloud/infra/configuration/-/blob/86f078fb4dfa5db5da8111fa202abd56c814e5e0/gitlab-runner-managers/grm-yaook-ephemeral.tpl.toml#L54
There was a problem hiding this comment.
Thanks — we're already using block_device_mapping_v2 in server_spec, similar to your example. The blocker for us isn't enabling volume-backed boot; it's image rotation without redeploying the runner managers.
Our setup:
Packer builds a new worker image every two weeks under a stable name (ubuntu-2404-glrunner), archiving the previous one so only one active image keeps that name.
Terraform resolves the image UUID at controller deploy time and bakes it into /etc/gitlab-runner/config.toml in block_device_mapping_v2.uuid.
We also set image_name in server_spec, which the fleeting plugin resolves to imageRef on each worker create.
The problem is that with destination_type = "volume", Nova boots from the BDM uuid, not from imageRef. The plugin's image_name resolution only updates imageRef — it does not update block_device_mapping_v2[].uuid. So after a packer run, Glance has a new image UUID under the same name, but controllers still have the old UUID hardcoded in BDM, and new workers keep booting from the old image.
The only workaround that works today without a plugin change would be redeploying controllers after each packer build — but that's not something we'd want to adopt as a process. It would only work because Terraform re-runs the image lookup at deploy time and re-bakes the new UUID into config.toml. It's operational overhead on a two-week cadence, not a real solution to the problem.
What we actually need is for the plugin to resolve image_name into BDM UUIDs at worker create time (same as it already does for imageRef), so managers don't need to be redeployed just to pick up a new image.
5 participants
No description provided.