Skip to content

Commit 0507507

Browse files
wanyufewanyufe
authored
Add diskOffering to machine template config (#67)
* Add diskOffering to machine template config * Add logic to remove vol while destroy virtual machine * add description of mountPath in machine template * add device, filesystem and label to machine template disk offering * missing headers * mark diskOffering in machine template optional * Add diskOffering to machine template config * Add logic to remove vol while destroy virtual machine * add description of mountPath in machine template * add device, filesystem and label to machine template disk offering * missing headers * mark diskOffering in machine template optional * update unit testing for disk offering while destroy VM * Some un-functional changes per PR comments Co-authored-by: Yufei Wang <[email protected]>
1 parent 73703a7 commit 0507507

13 files changed

+289
-1
lines changed

‎.gitignore

+1
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ nginx.conf
1111
nginx.conf.bak
1212

1313
pkg/mocks/*
14+
pkg/cloud/gomock_reflect_*
1415
!pkg/mocks/.keep
1516

1617
# Various build/test artifacts.

‎api/v1beta1/cloudstackmachine_types.go

+16
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,10 @@ type CloudStackMachineSpec struct {
4848
// CloudStack template to use.
4949
Template CloudStackResourceIdentifier `json:"template"`
5050

51+
// CloudStack disk offering to use.
52+
// +optional
53+
DiskOffering CloudStackResourceDiskOffering `json:"diskOffering,omitempty"`
54+
5155
// CloudStack ssh key to use.
5256
// +optional
5357
SSHKey string `json:"sshKey"`
@@ -95,6 +99,18 @@ type CloudStackResourceIdentifier struct {
9599
Name string `json:"name,omitempty"`
96100
}
97101

102+
type CloudStackResourceDiskOffering struct {
103+
CloudStackResourceIdentifier `json:",inline"`
104+
// mount point the data disk uses to mount. The actual partition, mkfs and mount are done by cloud-init generated by kubeadmConfig.
105+
MountPath string `json:"mountPath"`
106+
// device name of data disk, for example /dev/vdb
107+
Device string `json:"device"`
108+
// filesystem used by data disk, for example, ext4, xfs
109+
Filesystem string `json:"filesystem"`
110+
// label of data disk, used by mkfs as label parameter
111+
Label string `json:"label"`
112+
}
113+
98114
// TODO: Review the use of this field/type.
99115
type InstanceState string
100116

‎api/v1beta1/cloudstackmachine_webhook.go

+5
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,11 @@ func (r *CloudStackMachine) ValidateUpdate(old runtime.Object) error {
8282
oldSpec := oldMachine.Spec
8383

8484
errorList = webhookutil.EnsureBothFieldsAreEqual(r.Spec.Offering.ID, r.Spec.Offering.Name, oldSpec.Offering.ID, oldSpec.Offering.Name, "offering", errorList)
85+
errorList = webhookutil.EnsureBothFieldsAreEqual(r.Spec.DiskOffering.ID, r.Spec.DiskOffering.Name, oldSpec.DiskOffering.ID, oldSpec.DiskOffering.Name, "diskOffering", errorList)
86+
errorList = webhookutil.EnsureStringFieldsAreEqual(r.Spec.DiskOffering.MountPath, oldSpec.DiskOffering.MountPath, "mountPath", errorList)
87+
errorList = webhookutil.EnsureStringFieldsAreEqual(r.Spec.DiskOffering.Device, oldSpec.DiskOffering.Device, "device", errorList)
88+
errorList = webhookutil.EnsureStringFieldsAreEqual(r.Spec.DiskOffering.Filesystem, oldSpec.DiskOffering.Filesystem, "filesystem", errorList)
89+
errorList = webhookutil.EnsureStringFieldsAreEqual(r.Spec.DiskOffering.Label, oldSpec.DiskOffering.Label, "label", errorList)
8590
errorList = webhookutil.EnsureStringFieldsAreEqual(r.Spec.SSHKey, oldSpec.SSHKey, "sshkey", errorList)
8691
errorList = webhookutil.EnsureBothFieldsAreEqual(r.Spec.Template.ID, r.Spec.Template.Name, oldSpec.Template.ID, oldSpec.Template.Name, "template", errorList)
8792
errorList = webhookutil.EnsureStringStringMapFieldsAreEqual(&r.Spec.Details, &oldSpec.Details, "details", errorList)

‎api/v1beta1/cloudstackmachine_webhook_test.go

+11
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,11 @@ var _ = Describe("CloudStackMachine webhook", func() {
4141
Expect(k8sClient.Create(ctx, dummies.CSMachine1)).Should(Succeed())
4242
})
4343

44+
It("should accept a CloudStackMachine with disk Offering attribute", func() {
45+
dummies.CSMachine1.Spec.DiskOffering = dummies.DiskOffering2
46+
Expect(k8sClient.Create(ctx, dummies.CSMachine1)).Should(Succeed())
47+
})
48+
4449
It("should reject a CloudStackMachine with missing Offering attribute", func() {
4550
dummies.CSMachine1.Spec.Offering = v1beta1.CloudStackResourceIdentifier{ID: "", Name: ""}
4651
Expect(k8sClient.Create(ctx, dummies.CSMachine1)).
@@ -77,6 +82,12 @@ var _ = Describe("CloudStackMachine webhook", func() {
7782
Should(MatchError(MatchRegexp(forbiddenRegex, "template")))
7883
})
7984

85+
It("should reject VM disk offering updates to the CloudStackMachine", func() {
86+
dummies.CSMachine1.Spec.DiskOffering = dummies.DiskOffering2
87+
Ω(k8sClient.Update(ctx, dummies.CSMachine1)).
88+
Should(MatchError(MatchRegexp(forbiddenRegex, "diskOffering")))
89+
})
90+
8091
It("should reject updates to VM details of the CloudStackMachine", func() {
8192
dummies.CSMachine1.Spec.Details = map[string]string{"memoryOvercommitRatio": "1.5"}
8293
Ω(k8sClient.Update(ctx, dummies.CSMachine1)).

‎api/v1beta1/cloudstackmachinetemplate_webhook.go

+1
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ func (r *CloudStackMachineTemplate) ValidateUpdate(old runtime.Object) error {
9797

9898
errorList := field.ErrorList(nil)
9999
errorList = webhookutil.EnsureBothFieldsAreEqual(spec.Offering.ID, spec.Offering.Name, oldSpec.Offering.ID, oldSpec.Offering.Name, "offering", errorList)
100+
errorList = webhookutil.EnsureBothFieldsAreEqual(spec.DiskOffering.ID, spec.DiskOffering.Name, oldSpec.DiskOffering.ID, oldSpec.DiskOffering.Name, "diskOffering", errorList)
100101
errorList = webhookutil.EnsureStringFieldsAreEqual(spec.SSHKey, oldSpec.SSHKey, "sshkey", errorList)
101102
errorList = webhookutil.EnsureBothFieldsAreEqual(spec.Template.ID, spec.Template.Name, oldSpec.Template.ID, oldSpec.Template.Name, "template", errorList)
102103
errorList = webhookutil.EnsureStringStringMapFieldsAreEqual(&spec.Details, &oldSpec.Details, "details", errorList)

‎api/v1beta1/cloudstackmachinetemplate_webhook_test.go

+14
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,13 @@ var _ = Describe("CloudStackMachineTemplate webhook", func() {
4141
Expect(k8sClient.Create(ctx, dummies.CSMachineTemplate1)).Should(Succeed())
4242
})
4343

44+
It("Should accept a CloudStackMachineTemplate when missing the VM Disk Offering attribute", func() {
45+
dummies.CSMachineTemplate1.Spec.Spec.Spec.DiskOffering = v1beta1.CloudStackResourceDiskOffering{
46+
CloudStackResourceIdentifier: v1beta1.CloudStackResourceIdentifier{Name: "", ID: ""},
47+
}
48+
Expect(k8sClient.Create(ctx, dummies.CSMachineTemplate1)).Should(Succeed())
49+
})
50+
4451
It("Should reject a CloudStackMachineTemplate when missing the VM Offering attribute", func() {
4552
dummies.CSMachineTemplate1.Spec.Spec.Spec.Offering = v1beta1.CloudStackResourceIdentifier{Name: "", ID: ""}
4653
Expect(k8sClient.Create(ctx, dummies.CSMachineTemplate1)).
@@ -71,6 +78,13 @@ var _ = Describe("CloudStackMachineTemplate webhook", func() {
7178
Should(MatchError(MatchRegexp(forbiddenRegex, "template")))
7279
})
7380

81+
It("should reject VM disk offering updates to the CloudStackMachineTemplate", func() {
82+
dummies.CSMachineTemplate1.Spec.Spec.Spec.DiskOffering = v1beta1.CloudStackResourceDiskOffering{
83+
CloudStackResourceIdentifier: v1beta1.CloudStackResourceIdentifier{Name: "DiskOffering2"}}
84+
Ω(k8sClient.Update(ctx, dummies.CSMachineTemplate1)).
85+
Should(MatchError(MatchRegexp(forbiddenRegex, "diskOffering")))
86+
})
87+
7488
It("should reject VM offering updates to the CloudStackMachineTemplate", func() {
7589
dummies.CSMachineTemplate1.Spec.Spec.Spec.Offering = v1beta1.CloudStackResourceIdentifier{Name: "Offering2"}
7690
Ω(k8sClient.Update(ctx, dummies.CSMachineTemplate1)).

‎api/v1beta1/zz_generated.deepcopy.go

+17
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎config/crd/bases/infrastructure.cluster.x-k8s.io_cloudstackmachines.yaml

+30
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,36 @@ spec:
113113
type: string
114114
description: Optional details map for deployVirtualMachine
115115
type: object
116+
diskOffering:
117+
description: CloudStack disk offering to use.
118+
properties:
119+
device:
120+
description: device name of data disk, for example /dev/vdb
121+
type: string
122+
filesystem:
123+
description: filesystem used by data disk, for example, ext4,
124+
xfs
125+
type: string
126+
id:
127+
description: Cloudstack resource ID.
128+
type: string
129+
label:
130+
description: label of data disk, used by mkfs as label parameter
131+
type: string
132+
mountPath:
133+
description: mount point the data disk uses to mount. The actual
134+
partition, mkfs and mount are done by cloud-init generated by
135+
kubeadmConfig.
136+
type: string
137+
name:
138+
description: Cloudstack resource Name
139+
type: string
140+
required:
141+
- device
142+
- filesystem
143+
- label
144+
- mountPath
145+
type: object
116146
id:
117147
description: ID.
118148
type: string

‎config/crd/bases/infrastructure.cluster.x-k8s.io_cloudstackmachinetemplates.yaml

+31
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,37 @@ spec:
100100
type: string
101101
description: Optional details map for deployVirtualMachine
102102
type: object
103+
diskOffering:
104+
description: CloudStack disk offering to use.
105+
properties:
106+
device:
107+
description: device name of data disk, for example /dev/vdb
108+
type: string
109+
filesystem:
110+
description: filesystem used by data disk, for example,
111+
ext4, xfs
112+
type: string
113+
id:
114+
description: Cloudstack resource ID.
115+
type: string
116+
label:
117+
description: label of data disk, used by mkfs as label
118+
parameter
119+
type: string
120+
mountPath:
121+
description: mount point the data disk uses to mount.
122+
The actual partition, mkfs and mount are done by cloud-init
123+
generated by kubeadmConfig.
124+
type: string
125+
name:
126+
description: Cloudstack resource Name
127+
type: string
128+
required:
129+
- device
130+
- filesystem
131+
- label
132+
- mountPath
133+
type: object
103134
id:
104135
description: ID.
105136
type: string

‎pkg/cloud/helpers.go

+7
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,20 @@ import (
2323
)
2424

2525
type set func(string)
26+
type setArray func([]string)
2627

2728
func setIfNotEmpty(str string, setFn set) {
2829
if str != "" {
2930
setFn(str)
3031
}
3132
}
3233

34+
func setArrayIfNotEmpty(strArray []string, setFn setArray) {
35+
if len(strArray) > 0 {
36+
setFn(strArray)
37+
}
38+
}
39+
3340
func CompressAndEncodeString(str string) (string, error) {
3441
buf := &bytes.Buffer{}
3542
gzipWriter := gzip.NewWriter(buf)

‎pkg/cloud/instance.go

+63
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,41 @@ func (c *client) ResolveTemplate(
135135
return templateID, nil
136136
}
137137

138+
// ResolveDiskOffering Retrieves diskOffering by using disk offering ID if ID is provided and confirm returned
139+
// disk offering name matches name provided in spec.
140+
// If disk offering ID is not provided, the disk offering name is used to retrieve disk offering ID.
141+
func (c *client) ResolveDiskOffering(csMachine *infrav1.CloudStackMachine) (diskOfferingID string, retErr error) {
142+
if len(csMachine.Spec.DiskOffering.ID) > 0 {
143+
csDiskOffering, count, err := c.cs.DiskOffering.GetDiskOfferingByID(csMachine.Spec.DiskOffering.ID)
144+
if err != nil {
145+
return "", multierror.Append(retErr, errors.Wrapf(
146+
err, "could not get DiskOffering by ID %s", csMachine.Spec.DiskOffering.ID))
147+
} else if count != 1 {
148+
return "", multierror.Append(retErr, errors.Errorf(
149+
"expected 1 DiskOffering with UUID %s, but got %d", csMachine.Spec.DiskOffering.ID, count))
150+
}
151+
152+
if len(csMachine.Spec.DiskOffering.Name) > 0 && csMachine.Spec.DiskOffering.Name != csDiskOffering.Name {
153+
return "", multierror.Append(retErr, errors.Errorf(
154+
"diskOffering name %s does not match name %s returned using UUID %s",
155+
csMachine.Spec.DiskOffering.Name, csDiskOffering.Name, csMachine.Spec.DiskOffering.ID))
156+
}
157+
return csMachine.Spec.DiskOffering.ID, nil
158+
}
159+
if len(csMachine.Spec.DiskOffering.Name) == 0 {
160+
return "", nil
161+
}
162+
diskID, count, err := c.cs.DiskOffering.GetDiskOfferingID(csMachine.Spec.DiskOffering.Name)
163+
if err != nil {
164+
return "", multierror.Append(retErr, errors.Wrapf(
165+
err, "could not get DiskOffering ID from %s", csMachine.Spec.DiskOffering.Name))
166+
} else if count != 1 {
167+
return "", multierror.Append(retErr, errors.Errorf(
168+
"expected 1 DiskOffering with name %s, but got %d", csMachine.Spec.DiskOffering.Name, count))
169+
}
170+
return diskID, nil
171+
}
172+
138173
// GetOrCreateVMInstance CreateVMInstance will fetch or create a VM instance, and
139174
// sets the infrastructure machine spec and status accordingly.
140175
func (c *client) GetOrCreateVMInstance(
@@ -159,6 +194,10 @@ func (c *client) GetOrCreateVMInstance(
159194
if err != nil {
160195
return err
161196
}
197+
diskOfferingID, err := c.ResolveDiskOffering(csMachine)
198+
if err != nil {
199+
return err
200+
}
162201

163202
// Create VM instance.
164203
p := c.cs.VirtualMachine.NewDeployVirtualMachineParams(offeringID, templateID, csMachine.Status.ZoneID)
@@ -167,6 +206,7 @@ func (c *client) GetOrCreateVMInstance(
167206
p.SetNetworkids([]string{zone.Spec.Network.ID})
168207
setIfNotEmpty(csMachine.Name, p.SetName)
169208
setIfNotEmpty(csMachine.Name, p.SetDisplayname)
209+
setIfNotEmpty(diskOfferingID, p.SetDiskofferingid)
170210

171211
setIfNotEmpty(csMachine.Spec.SSHKey, p.SetKeypair)
172212

@@ -218,7 +258,12 @@ func (c *client) DestroyVMInstance(csMachine *infrav1.CloudStackMachine) error {
218258

219259
// Attempt deletion regardless of machine state.
220260
p := c.cs.VirtualMachine.NewDestroyVirtualMachineParams(*csMachine.Spec.InstanceID)
261+
volIDs, err := c.listVMInstanceVolumeIDs(*csMachine.Spec.InstanceID)
262+
if err != nil {
263+
return err
264+
}
221265
p.SetExpunge(true)
266+
setArrayIfNotEmpty(volIDs, p.SetVolumeids)
222267
if _, err := c.csAsync.VirtualMachine.DestroyVirtualMachine(p); err != nil &&
223268
strings.Contains(strings.ToLower(err.Error()), "unable to find uuid for id") {
224269
// VM doesn't exist. Success...
@@ -241,3 +286,21 @@ func (c *client) DestroyVMInstance(csMachine *infrav1.CloudStackMachine) error {
241286

242287
return errors.New("VM deletion in progress")
243288
}
289+
290+
func (c *client) listVMInstanceVolumeIDs(instanceID string) ([]string, error) {
291+
p := c.cs.Volume.NewListVolumesParams()
292+
p.SetVirtualmachineid(instanceID)
293+
294+
listVOLResp, err := c.csAsync.Volume.ListVolumes(p)
295+
if err != nil {
296+
return nil, err
297+
}
298+
299+
var ret []string
300+
for _, vol := range listVOLResp.Volumes {
301+
ret = append(ret, vol.Id)
302+
}
303+
304+
return ret, nil
305+
306+
}

0 commit comments

Comments
 (0)