Skip to content

Commit 1ef41dd

Browse files
lubronzhanlubronzhan
committed
Move reconcile virtual controller code above
1 parent 04e606d commit 1ef41dd

File tree

4 files changed

+199
-16
lines changed

4 files changed

+199
-16
lines changed

‎pkg/providers/vsphere/session/session_vm_update.go‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -275,10 +275,6 @@ func (s *Session) reconcilePoweredOffOrPoweredOnVM(
275275
return err
276276
}
277277

278-
if err := s.reconcileVolumes(vmCtx); err != nil {
279-
return err
280-
}
281-
282278
if vmCtx.VM.Spec.GuestID == "" {
283279
// Assume the guest ID is valid until we know otherwise.
284280
conditions.Delete(vmCtx.VM, vmopv1.GuestIDReconfiguredCondition)
@@ -328,6 +324,14 @@ func (s *Session) reconcilePoweredOffOrPoweredOnVM(
328324
}
329325
}
330326

327+
// Moved down below doReconfigure because this function returns error
328+
// when volume is not ready. But sometimes this could due to corresponding
329+
// virtualcontrollers are not ready, while configuring virtualcontrollers
330+
// happens in doReconfigure.
331+
if err := s.reconcileVolumes(vmCtx); err != nil {
332+
return err
333+
}
334+
331335
return s.reconcileNetworkAndGuestCustomizationState(
332336
vmCtx,
333337
vcVM,
@@ -1169,6 +1173,8 @@ func reconcileVSpherePolicies(
11691173
return nil
11701174
}
11711175

1176+
// reconcileVirtualControllers check if VM need virtual controller changes,
1177+
// If yes, update configSpec.DeviceChange.
11721178
func reconcileVirtualControllers(
11731179
ctx context.Context,
11741180
k8sClient ctrlclient.Client,
@@ -1179,13 +1185,18 @@ func reconcileVirtualControllers(
11791185

11801186
pkglog.FromContextOrDefault(ctx).V(4).Info("Reconciling virtual controllers")
11811187

1182-
return vmconfvirtualcontroller.Reconcile(
1188+
if err := vmconfvirtualcontroller.Reconcile(
11831189
ctx,
11841190
k8sClient,
11851191
vcVM.Client(),
11861192
vm,
11871193
moVM,
1188-
configSpec)
1194+
configSpec); err != nil {
1195+
1196+
return err
1197+
}
1198+
1199+
return nil
11891200
}
11901201

11911202
func doReconfigure(

‎pkg/providers/vsphere/session/session_vm_update_test.go‎

Lines changed: 168 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -826,7 +826,9 @@ var _ = Describe("UpdateVirtualMachine", func() {
826826
})
827827

828828
assertUpdate := func() {
829-
ExpectWithOffset(1, ctxop.IsUpdate(vmCtx)).To(BeTrue())
829+
GinkgoHelper()
830+
831+
Expect(ctxop.IsUpdate(vmCtx)).To(BeTrue())
830832
}
831833

832834
When("VM is powered off", func() {
@@ -1423,6 +1425,171 @@ var _ = Describe("UpdateVirtualMachine", func() {
14231425
})
14241426
})
14251427
})
1428+
1429+
Context("Virtual Controllers", func() {
1430+
BeforeEach(func() {
1431+
vm.Spec.Hardware = &vmopv1.VirtualMachineHardwareSpec{}
1432+
})
1433+
1434+
When("Capability is enabled", func() {
1435+
1436+
JustBeforeEach(func() {
1437+
pkgcfg.SetContext(ctx, func(config *pkgcfg.Config) {
1438+
config.Features.VMSharedDisks = true
1439+
})
1440+
})
1441+
1442+
It("should not add virtual controllers to VM during update when VM is powered on", func() {
1443+
By("Setting VM power state to On", func() {
1444+
vmCtx.MoVM.Runtime.PowerState = vimtypes.VirtualMachinePowerStatePoweredOn
1445+
})
1446+
1447+
By("Specifying virtual controllers in the VM spec", func() {
1448+
vm.Spec.Hardware.SATAControllers = []vmopv1.SATAControllerSpec{
1449+
{
1450+
BusNumber: 1,
1451+
},
1452+
}
1453+
})
1454+
1455+
By("Updating the VM", func() {
1456+
// Update the VM
1457+
Expect(sess.UpdateVirtualMachine(vmCtx, vcVM, getUpdateArgs, getResizeArgs)).To(Succeed())
1458+
})
1459+
1460+
By("Verifying SATA controller was not added", func() {
1461+
Expect(vcVM.Properties(ctx, vcVM.Reference(), vmProps, &vmCtx.MoVM)).To(Succeed())
1462+
// Collect all the existing controller device keys.
1463+
sataControllerCount := 0
1464+
for _, d := range vmCtx.MoVM.Config.Hardware.Device {
1465+
if _, ok := d.(*vimtypes.VirtualAHCIController); ok {
1466+
sataControllerCount++
1467+
}
1468+
}
1469+
Expect(sataControllerCount).To(BeZero())
1470+
Expect(ctxop.IsUpdate(vmCtx)).ToNot(BeTrue())
1471+
})
1472+
})
1473+
1474+
It("should add virtual controllers to VM during update if VM is powered off", func() {
1475+
By("Setting up VM power state to Off", func() {
1476+
vmCtx.MoVM.Runtime.PowerState = vimtypes.VirtualMachinePowerStatePoweredOff
1477+
})
1478+
1479+
By("Specifying virtual controllers in the VM spec", func() {
1480+
vm.Spec.Hardware.IDEControllers = []vmopv1.IDEControllerSpec{
1481+
{
1482+
BusNumber: 1,
1483+
},
1484+
}
1485+
vm.Spec.Hardware.NVMEControllers = []vmopv1.NVMEControllerSpec{
1486+
{
1487+
BusNumber: 1,
1488+
SharingMode: vmopv1.VirtualControllerSharingModeNone,
1489+
},
1490+
}
1491+
vm.Spec.Hardware.SATAControllers = []vmopv1.SATAControllerSpec{
1492+
{
1493+
BusNumber: 1,
1494+
},
1495+
}
1496+
vm.Spec.Hardware.SCSIControllers = []vmopv1.SCSIControllerSpec{
1497+
{
1498+
BusNumber: 1,
1499+
SharingMode: vmopv1.VirtualControllerSharingModeNone,
1500+
Type: vmopv1.SCSIControllerTypeLsiLogic,
1501+
},
1502+
}
1503+
})
1504+
1505+
By("Updating the VM", func() {
1506+
// Update the VM
1507+
err := sess.UpdateVirtualMachine(vmCtx, vcVM, getUpdateArgs, getResizeArgs)
1508+
Expect(errors.Is(err, vsphere.ErrReconfigure)).To(BeTrue())
1509+
})
1510+
1511+
By("Verifying virtual controllers were added", func() {
1512+
// Refresh VM properties
1513+
Expect(vcVM.Properties(ctx, vcVM.Reference(), vmProps, &vmCtx.MoVM)).To(Succeed())
1514+
// Collect all the existing controller device keys.
1515+
ideControllers := []vimtypes.VirtualIDEController{}
1516+
nvmeControllers := []vimtypes.VirtualNVMEController{}
1517+
sataControllers := []vimtypes.VirtualAHCIController{}
1518+
scsiControllers := []vimtypes.BaseVirtualSCSIController{}
1519+
for _, d := range vmCtx.MoVM.Config.Hardware.Device {
1520+
switch d := d.(type) {
1521+
case *vimtypes.VirtualIDEController:
1522+
ideControllers = append(ideControllers, *d)
1523+
case *vimtypes.VirtualNVMEController:
1524+
nvmeControllers = append(nvmeControllers, *d)
1525+
case *vimtypes.VirtualAHCIController:
1526+
sataControllers = append(sataControllers, *d)
1527+
case vimtypes.BaseVirtualSCSIController:
1528+
scsiControllers = append(scsiControllers, d)
1529+
}
1530+
}
1531+
1532+
Expect(len(ideControllers)).To(Equal(1))
1533+
Expect(ideControllers[0].BusNumber).To(Equal(int32(1)))
1534+
Expect(len(nvmeControllers)).To(Equal(1))
1535+
Expect(nvmeControllers[0].BusNumber).To(Equal(int32(1)))
1536+
Expect(len(sataControllers)).To(Equal(1))
1537+
Expect(sataControllers[0].BusNumber).To(Equal(int32(1)))
1538+
Expect(len(scsiControllers)).To(Equal(1))
1539+
Expect(scsiControllers[0].GetVirtualSCSIController().BusNumber).To(Equal(int32(1)))
1540+
_, ok := scsiControllers[0].(*vimtypes.VirtualLsiLogicController)
1541+
Expect(ok).To(BeTrue())
1542+
})
1543+
1544+
assertUpdate()
1545+
})
1546+
})
1547+
1548+
When("Capability is disabled", func() {
1549+
1550+
JustBeforeEach(func() {
1551+
pkgcfg.SetContext(ctx, func(config *pkgcfg.Config) {
1552+
config.Features.VMSharedDisks = false
1553+
})
1554+
})
1555+
1556+
It("should not add virtual controllers to VM during update even if VM is powered off", func() {
1557+
By("Setting up VM power state to Off", func() {
1558+
vmCtx.MoVM.Runtime.PowerState = vimtypes.VirtualMachinePowerStatePoweredOff
1559+
})
1560+
1561+
By("Specifying virtual controllers in the VM spec", func() {
1562+
vm.Spec.Hardware.SATAControllers = []vmopv1.SATAControllerSpec{
1563+
{
1564+
BusNumber: 1,
1565+
},
1566+
}
1567+
})
1568+
1569+
By("Updating the VM", func() {
1570+
// Update the VM
1571+
Expect(sess.UpdateVirtualMachine(vmCtx, vcVM, getUpdateArgs, getResizeArgs)).To(MatchError(vmlifecycle.ErrBootstrapCustomize))
1572+
Expect(vcVM.Properties(ctx, vcVM.Reference(), vmProps, &vmCtx.MoVM)).To(Succeed())
1573+
// No-op
1574+
Expect(sess.UpdateVirtualMachine(vmCtx, vcVM, getUpdateArgs, getResizeArgs)).To(Succeed())
1575+
})
1576+
1577+
By("Verifying SATA controller was not added", func() {
1578+
Expect(vcVM.Properties(ctx, vcVM.Reference(), vmProps, &vmCtx.MoVM)).To(Succeed())
1579+
// Collect all the existing controller device keys.
1580+
sataControllerCount := 0
1581+
for _, d := range vmCtx.MoVM.Config.Hardware.Device {
1582+
if _, ok := d.(*vimtypes.VirtualAHCIController); ok {
1583+
sataControllerCount++
1584+
}
1585+
}
1586+
Expect(sataControllerCount).To(BeZero())
1587+
})
1588+
1589+
assertUpdate()
1590+
})
1591+
})
1592+
})
14261593
})
14271594

14281595
When("VM's resource police changes", func() {

‎pkg/vmconfig/virtualcontroller/virtualcontroller_reconciler.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ import (
1414
ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
1515

1616
vmopv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha5"
17-
pkgerr "github.com/vmware-tanzu/vm-operator/pkg/errors"
1817
pkglog "github.com/vmware-tanzu/vm-operator/pkg/log"
1918
"github.com/vmware-tanzu/vm-operator/pkg/vmconfig"
2019
)
@@ -83,10 +82,11 @@ func (r reconciler) Reconcile(
8382
return nil
8483
}
8584

86-
if vm.Status.PowerState == vmopv1.VirtualMachinePowerStateOn {
87-
return pkgerr.NoRequeueError{
88-
Message: "cannot add/edit/remove controllers when the VM is powered on",
89-
}
85+
if moVM.Runtime.PowerState != vimtypes.VirtualMachinePowerStatePoweredOff {
86+
// This shouldn't happen since validating webhook should've blocked this.
87+
pkglog.FromContextOrDefault(ctx).Info("updating virtual controllers when VM is not powered off is not allowed, skip updating")
88+
89+
return nil
9090
}
9191

9292
var (

‎pkg/vmconfig/virtualcontroller/virtualcontroller_reconciler_test.go‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,9 @@ var _ = Describe("Reconcile", func() {
7373

7474
moVM = mo.VirtualMachine{
7575
Config: &vimtypes.VirtualMachineConfigInfo{},
76+
Runtime: vimtypes.VirtualMachineRuntimeInfo{
77+
PowerState: vimtypes.VirtualMachinePowerStatePoweredOff,
78+
},
7679
}
7780

7881
configSpec = &vimtypes.VirtualMachineConfigSpec{}
@@ -100,6 +103,7 @@ var _ = Describe("Reconcile", func() {
100103
JustBeforeEach(func() {
101104
ctx = nil
102105
})
106+
103107
It("should panic", func() {
104108
fn := func() {
105109
_ = r.Reconcile(ctx, k8sClient, vimClient, vm, moVM, configSpec)
@@ -112,6 +116,7 @@ var _ = Describe("Reconcile", func() {
112116
JustBeforeEach(func() {
113117
k8sClient = nil
114118
})
119+
115120
It("should panic", func() {
116121
fn := func() {
117122
_ = r.Reconcile(ctx, k8sClient, vimClient, vm, moVM, configSpec)
@@ -204,12 +209,12 @@ var _ = Describe("Reconcile", func() {
204209

205210
When("VM is powered on", func() {
206211
BeforeEach(func() {
207-
vm.Status.PowerState = vmopv1.VirtualMachinePowerStateOn
212+
moVM.Runtime.PowerState = vimtypes.VirtualMachinePowerStatePoweredOn
208213
})
209214

210-
It("should return an error", func() {
211-
Expect(r.Reconcile(ctx, k8sClient, vimClient, vm, moVM, configSpec)).
212-
To(MatchError("cannot add/edit/remove controllers when the VM is powered on"))
215+
It("should succeeds without making any changes", func() {
216+
Expect(r.Reconcile(ctx, k8sClient, vimClient, vm, moVM, configSpec)).To(Succeed())
217+
Expect(configSpec.DeviceChange).To(HaveLen(0))
213218
})
214219
})
215220

0 commit comments

Comments
 (0)