diff --git a/api/core/v1alpha2/vmbdacondition/condition.go b/api/core/v1alpha2/vmbdacondition/condition.go index 7ed5de02a..fd0955b6e 100644 --- a/api/core/v1alpha2/vmbdacondition/condition.go +++ b/api/core/v1alpha2/vmbdacondition/condition.go @@ -54,7 +54,9 @@ const ( NotAttached AttachedReason = "NotAttached" // AttachmentRequestSent signifies that the attachment request has been sent and the hot-plug process has started. AttachmentRequestSent AttachedReason = "AttachmentRequestSent" - // Conflict indicates that there is another `VirtualMachineBlockDeviceAttachment` with the same virtual machine and virtual disk to be hot-plugged. + // Conflict indicates that virtual disk is already attached to the virtual machine: + // Either there is another `VirtualMachineBlockDeviceAttachment` with the same virtual machine and virtual disk to be hot-plugged. + // or the virtual disk is already attached to the virtual machine spec. // Only the one that was created or started sooner can be processed. Conflict AttachedReason = "Conflict" ) diff --git a/images/virtualization-artifact/pkg/controller/service/attachment_service.go b/images/virtualization-artifact/pkg/controller/service/attachment_service.go index 50fcfc46b..eb9e13c26 100644 --- a/images/virtualization-artifact/pkg/controller/service/attachment_service.go +++ b/images/virtualization-artifact/pkg/controller/service/attachment_service.go @@ -46,7 +46,12 @@ func NewAttachmentService(client Client, controllerNamespace string) *Attachment } } -var ErrVolumeStatusNotReady = errors.New("hotplug is not ready") +var ( + ErrVolumeStatusNotReady = errors.New("hotplug is not ready") + ErrDiskIsSpecAttached = errors.New("virtual disk is already attached to the virtual machine spec") + ErrHotPlugRequestAlreadySent = errors.New("attachment request is already sent") + ErrVirtualMachineWaitsForRestartApproval = errors.New("virtual machine waits for restart approval") +) func (s AttachmentService) IsHotPlugged(vd *virtv2.VirtualDisk, vm *virtv2.VirtualMachine, kvvmi *virtv1.VirtualMachineInstance) (bool, error) { if vd == nil { @@ -74,15 +79,27 @@ func (s AttachmentService) IsHotPlugged(vd *virtv2.VirtualDisk, vm *virtv2.Virtu return false, nil } -func (s AttachmentService) IsHotPlugRequestSent(vd *virtv2.VirtualDisk, kvvm *virtv1.VirtualMachine) (bool, error) { - name := kvbuilder.GenerateVMDDiskName(vd.Name) +func (s AttachmentService) CanHotPlug(vd *virtv2.VirtualDisk, vm *virtv2.VirtualMachine, kvvm *virtv1.VirtualMachine) (bool, error) { + if vd == nil { + return false, errors.New("cannot hot plug a nil VirtualDisk") + } - for _, vr := range kvvm.Status.VolumeRequests { - if vr.AddVolumeOptions.Name == name { - return true, nil + if vm == nil { + return false, errors.New("cannot hot plug a disk into a nil VirtualMachine") + } + + if kvvm == nil { + return false, errors.New("cannot hot plug a disk into a nil KVVM") + } + + for _, bdr := range vm.Spec.BlockDeviceRefs { + if bdr.Kind == virtv2.DiskDevice && bdr.Name == vd.Name { + return false, fmt.Errorf("%w: virtual machine has a virtual disk reference, but it is not a hot-plugged volume", ErrDiskIsSpecAttached) } } + name := kvbuilder.GenerateVMDDiskName(vd.Name) + if kvvm.Spec.Template != nil { for _, vs := range kvvm.Spec.Template.Spec.Volumes { if vs.Name == name { @@ -91,21 +108,26 @@ func (s AttachmentService) IsHotPlugRequestSent(vd *virtv2.VirtualDisk, kvvm *vi } if !vs.PersistentVolumeClaim.Hotpluggable { - return false, fmt.Errorf("kvvm %s/%s spec volume %s has a pvc reference, but it is not a hot-plugged volume", kvvm.Namespace, kvvm.Name, vs.Name) + return false, fmt.Errorf("%w: virtual machine has a virtual disk reference, but it is not a hot-plugged volume", ErrDiskIsSpecAttached) } - return true, nil + return false, ErrHotPlugRequestAlreadySent } } } - return false, nil -} + for _, vr := range kvvm.Status.VolumeRequests { + if vr.AddVolumeOptions.Name == name { + return false, ErrHotPlugRequestAlreadySent + } + } -var ( - ErrVirtualDiskIsAlreadyAttached = errors.New("virtual disk is already attached to virtual machine") - ErrVirtualMachineWaitsForRestartApproval = errors.New("virtual machine waits for restart approval") -) + if len(vm.Status.RestartAwaitingChanges) > 0 { + return false, ErrVirtualMachineWaitsForRestartApproval + } + + return true, nil +} func (s AttachmentService) HotPlugDisk(ctx context.Context, vd *virtv2.VirtualDisk, vm *virtv2.VirtualMachine, kvvm *virtv1.VirtualMachine) error { if vd == nil { @@ -116,14 +138,8 @@ func (s AttachmentService) HotPlugDisk(ctx context.Context, vd *virtv2.VirtualDi return errors.New("cannot hot plug a disk into a nil VirtualMachine") } - for _, bdr := range vm.Spec.BlockDeviceRefs { - if bdr.Kind == virtv2.DiskDevice && bdr.Name == vd.Name { - return ErrVirtualDiskIsAlreadyAttached - } - } - - if len(vm.Status.RestartAwaitingChanges) > 0 { - return ErrVirtualMachineWaitsForRestartApproval + if kvvm == nil { + return errors.New("cannot hot plug a disk into a nil KVVM") } name := kvbuilder.GenerateVMDDiskName(vd.Name) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/block_device.go b/images/virtualization-artifact/pkg/controller/vm/internal/block_device.go index 03d0649af..a8f0b49ea 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/block_device.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/block_device.go @@ -79,17 +79,6 @@ func (h *BlockDeviceHandler) Handle(ctx context.Context, s state.VirtualMachineS cb := conditions.NewConditionBuilder2(vmcondition.TypeBlockDevicesReady). Generation(current.GetGeneration()) - disksMessage := h.checkBlockDevicesSanity(current) - if !isDeletion(current) && disksMessage != "" { - log.Error(fmt.Sprintf("invalid disks: %s", disksMessage)) - mgr.Update(cb. - Status(metav1.ConditionFalse). - Reason2(vmcondition.ReasonBlockDevicesNotReady). - Message(disksMessage).Condition()) - changed.Status.Conditions = mgr.Generate() - return reconcile.Result{Requeue: true}, nil - } - bdState := NewBlockDeviceState(s) err := bdState.Reload(ctx) if err != nil { @@ -99,65 +88,30 @@ func (h *BlockDeviceHandler) Handle(ctx context.Context, s state.VirtualMachineS if isDeletion(current) { return reconcile.Result{}, h.removeFinalizersOnBlockDevices(ctx, changed, bdState) } + if err = h.setFinalizersOnBlockDevices(ctx, changed, bdState); err != nil { return reconcile.Result{}, fmt.Errorf("unable to add block devices finalizers: %w", err) } - // Fill BlockDeviceRefs every time without knowledge of previously kept BlockDeviceRefs. - changed.Status.BlockDeviceRefs = nil - - // Set BlockDeviceRef from spec to the status. - for _, bdSpecRef := range changed.Spec.BlockDeviceRefs { - bdStatusRef := h.getDiskStatusRef(bdSpecRef.Kind, bdSpecRef.Name) - bdStatusRef.Size = h.getBlockDeviceSize(&bdStatusRef, bdState) - - changed.Status.BlockDeviceRefs = append( - changed.Status.BlockDeviceRefs, - bdStatusRef, - ) - } - - vmbdas, err := s.VMBDAList(ctx) + // Get hot plugged BlockDeviceRefs from vmbdas. + vmbdaStatusRefs, err := h.getBlockDeviceStatusRefsFromVMBDA(ctx, s) if err != nil { - return reconcile.Result{}, err + return reconcile.Result{}, fmt.Errorf("failed to get hotplugged block devices: %w", err) } - // Set hot plugged BlockDeviceRef to the status. - for _, vmbda := range vmbdas { - switch vmbda.Status.Phase { - case virtv2.BlockDeviceAttachmentPhaseInProgress, - virtv2.BlockDeviceAttachmentPhaseAttached: - default: - continue - } - - var vd *virtv2.VirtualDisk - vd, err = s.VirtualDisk(ctx, vmbda.Spec.BlockDeviceRef.Name) - if err != nil { - return reconcile.Result{}, err - } - - if vd == nil { - continue - } - - bdStatusRef := h.getDiskStatusRef(virtv2.DiskDevice, vmbda.Spec.BlockDeviceRef.Name) - bdStatusRef.Size = vd.Status.Capacity - bdStatusRef.Hotplugged = true - bdStatusRef.VirtualMachineBlockDeviceAttachmentName = vmbda.Name + // Get BlockDeviceRefs from spec. + bdStatusRefs, conflictWarning := h.getBlockDeviceStatusRefsFromSpec(current.Spec.BlockDeviceRefs, bdState, vmbdaStatusRefs) - changed.Status.BlockDeviceRefs = append( - changed.Status.BlockDeviceRefs, - bdStatusRef, - ) - } + // Fill BlockDeviceRefs every time without knowledge of previously kept BlockDeviceRefs. + changed.Status.BlockDeviceRefs = bdStatusRefs + changed.Status.BlockDeviceRefs = append(changed.Status.BlockDeviceRefs, vmbdaStatusRefs...) kvvmi, err := s.KVVMI(ctx) if err != nil { return reconcile.Result{}, err } - // Sync BlockDeviceRef in the status with KVVMI volumes. + // Sync BlockDeviceRefs in the status with KVVMI volumes. if kvvmi != nil { for i, bdStatusRef := range changed.Status.BlockDeviceRefs { vs := h.findVolumeStatus(GenerateDiskName(bdStatusRef.Kind, bdStatusRef.Name), kvvmi) @@ -170,6 +124,17 @@ func (h *BlockDeviceHandler) Handle(ctx context.Context, s state.VirtualMachineS } } + // Update the BlockDevicesReady condition if there are conflicted virtual disks. + if conflictWarning != "" { + log.Info(fmt.Sprintf("Conflicted virtual disks: %s", conflictWarning)) + + mgr.Update(cb.Status(metav1.ConditionFalse). + Reason2(vmcondition.ReasonBlockDevicesNotReady). + Message(conflictWarning).Condition()) + changed.Status.Conditions = mgr.Generate() + return reconcile.Result{Requeue: true}, nil + } + // Update the BlockDevicesReady condition. if readyCount, canStartKVVM, warnings := h.countReadyBlockDevices(current, bdState, log); len(current.Spec.BlockDeviceRefs) != readyCount { var reason vmcondition.Reason @@ -189,8 +154,7 @@ func (h *BlockDeviceHandler) Handle(ctx context.Context, s state.VirtualMachineS log.Info(msg, "actualReady", readyCount, "expectedReady", len(current.Spec.BlockDeviceRefs)) h.recorder.Event(changed, corev1.EventTypeNormal, reason.String(), msg) - mgr.Update(cb. - Status(metav1.ConditionFalse). + mgr.Update(cb.Status(metav1.ConditionFalse). Reason(reason.String()). Message(msg). Condition()) @@ -209,35 +173,74 @@ func (h *BlockDeviceHandler) Name() string { return nameBlockDeviceHandler } -// checkBlockDevicesSanity compares spec.blockDevices and status.blockDevicesAttached. -// It returns false if the same disk contains in both arrays. -// It is a precaution to not apply changes in spec.blockDevices if disk is already -// hotplugged using the VMBDA resource. The reverse check is done by the vmbda-controller. -func (h *BlockDeviceHandler) checkBlockDevicesSanity(vm *virtv2.VirtualMachine) string { - if vm == nil { - return "" +func (h *BlockDeviceHandler) getBlockDeviceStatusRefsFromSpec(bdSpecRefs []virtv2.BlockDeviceSpecRef, bdState BlockDevicesState, hotplugs []virtv2.BlockDeviceStatusRef) ([]virtv2.BlockDeviceStatusRef, string) { + hotplugsByName := make(map[string]struct{}, len(hotplugs)) + for _, hotplug := range hotplugs { + hotplugsByName[hotplug.Name] = struct{}{} } - disks := make([]string, 0) - hotplugged := make(map[string]struct{}) - for _, bda := range vm.Status.BlockDeviceRefs { - if bda.Hotplugged { - hotplugged[bda.Name] = struct{}{} - } - } + var conflictedRefs []string + var refs []virtv2.BlockDeviceStatusRef - for _, bd := range vm.Spec.BlockDeviceRefs { - if bd.Kind == virtv2.DiskDevice { - if _, ok := hotplugged[bd.Name]; ok { - disks = append(disks, bd.Name) + for _, bdSpecRef := range bdSpecRefs { + // It is a precaution to not apply changes in spec.blockDeviceRefs if disk is already + // hotplugged using the VMBDA resource. The reverse check is done by the vmbda-controller. + if bdSpecRef.Kind == virtv2.DiskDevice { + if _, conflict := hotplugsByName[bdSpecRef.Name]; conflict { + conflictedRefs = append(conflictedRefs, bdSpecRef.Name) + continue } } + + bdStatusRef := h.getDiskStatusRef(bdSpecRef.Kind, bdSpecRef.Name) + bdStatusRef.Size = h.getBlockDeviceSize(&bdStatusRef, bdState) + + refs = append(refs, bdStatusRef) } - if len(disks) == 0 { - return "" + var warning string + if len(conflictedRefs) > 0 { + warning = fmt.Sprintf("spec.blockDeviceRefs field contains hotplugged disks (%s): unplug or remove them from spec to continue.", strings.Join(conflictedRefs, ", ")) } - return fmt.Sprintf("spec.blockDeviceRefs contain hotplugged disks: %s. Unplug or remove them from spec to continue.", strings.Join(disks, ", ")) + + return refs, warning +} + +func (h *BlockDeviceHandler) getBlockDeviceStatusRefsFromVMBDA(ctx context.Context, s state.VirtualMachineState) ([]virtv2.BlockDeviceStatusRef, error) { + vmbdas, err := s.VMBDAList(ctx) + if err != nil { + return nil, err + } + + var refs []virtv2.BlockDeviceStatusRef + + for _, vmbda := range vmbdas { + switch vmbda.Status.Phase { + case virtv2.BlockDeviceAttachmentPhaseInProgress, + virtv2.BlockDeviceAttachmentPhaseAttached: + default: + continue + } + + var vd *virtv2.VirtualDisk + vd, err = s.VirtualDisk(ctx, vmbda.Spec.BlockDeviceRef.Name) + if err != nil { + return nil, err + } + + if vd == nil { + continue + } + + bdStatusRef := h.getDiskStatusRef(virtv2.DiskDevice, vmbda.Spec.BlockDeviceRef.Name) + bdStatusRef.Size = vd.Status.Capacity + bdStatusRef.Hotplugged = true + bdStatusRef.VirtualMachineBlockDeviceAttachmentName = vmbda.Name + + refs = append(refs, bdStatusRef) + } + + return refs, nil } // countReadyBlockDevices check if all attached images and disks are ready to use by the VM. diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/block_device_refs_validator.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/block_device_refs_validator.go new file mode 100644 index 000000000..e12265d9d --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/block_device_refs_validator.go @@ -0,0 +1,54 @@ +/* +Copyright 2024 Flant JSC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validators + +import ( + "context" + "fmt" + + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + "github.com/deckhouse/virtualization/api/core/v1alpha2" +) + +type BlockDeviceSpecRefsValidator struct{} + +func NewBlockDeviceSpecRefsValidator() *BlockDeviceSpecRefsValidator { + return &BlockDeviceSpecRefsValidator{} +} + +func (v *BlockDeviceSpecRefsValidator) ValidateCreate(_ context.Context, vm *v1alpha2.VirtualMachine) (admission.Warnings, error) { + return nil, v.noDoubles(vm) +} + +func (v *BlockDeviceSpecRefsValidator) ValidateUpdate(_ context.Context, _, newVM *v1alpha2.VirtualMachine) (admission.Warnings, error) { + return nil, v.noDoubles(newVM) +} + +func (v *BlockDeviceSpecRefsValidator) noDoubles(vm *v1alpha2.VirtualMachine) error { + blockDevicesByRef := make(map[v1alpha2.BlockDeviceSpecRef]struct{}, len(vm.Spec.BlockDeviceRefs)) + + for _, bdRef := range vm.Spec.BlockDeviceRefs { + if _, ok := blockDevicesByRef[bdRef]; ok { + return fmt.Errorf("cannot specify the same block device reference more than once: %s with name %s has a duplicate reference", bdRef.Kind, bdRef.Name) + } + + blockDevicesByRef[bdRef] = struct{}{} + } + + return nil +} diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/ipam_validator.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/ipam_validator.go new file mode 100644 index 000000000..1994dc8cb --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/ipam_validator.go @@ -0,0 +1,86 @@ +/* +Copyright 2024 Flant JSC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validators + +import ( + "context" + "fmt" + + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + "github.com/deckhouse/virtualization-controller/pkg/controller/vm/internal" + "github.com/deckhouse/virtualization-controller/pkg/sdk/framework/helper" + "github.com/deckhouse/virtualization/api/core/v1alpha2" +) + +type IPAMValidator struct { + ipam internal.IPAM + client client.Client +} + +func NewIPAMValidator(ipam internal.IPAM, client client.Client) *IPAMValidator { + return &IPAMValidator{ipam: ipam, client: client} +} + +func (v *IPAMValidator) ValidateCreate(ctx context.Context, vm *v1alpha2.VirtualMachine) (admission.Warnings, error) { + vmipName := vm.Spec.VirtualMachineIPAddress + if vmipName == "" { + vmipName = vm.Name + } + + vmipKey := types.NamespacedName{Name: vmipName, Namespace: vm.Namespace} + vmip, err := helper.FetchObject(ctx, vmipKey, v.client, &v1alpha2.VirtualMachineIPAddress{}) + if err != nil { + return nil, fmt.Errorf("unable to get referenced VirtualMachineIPAddress %s: %w", vmipKey, err) + } + + if vmip == nil { + return nil, nil + } + + // VM is created without ip address, but ip address resource is already exists. + if vm.Spec.VirtualMachineIPAddress == "" { + return nil, fmt.Errorf("VirtualMachineIPAddress with the name of the virtual machine"+ + " already exists: set spec.virtualMachineIPAddress field to %s to use IP %s", vmip.Name, vmip.Status.Address) + } + + return nil, v.ipam.CheckIpAddressAvailableForBinding(vm.Name, vmip) +} + +func (v *IPAMValidator) ValidateUpdate(ctx context.Context, oldVM, newVM *v1alpha2.VirtualMachine) (admission.Warnings, error) { + if oldVM.Spec.VirtualMachineIPAddress == newVM.Spec.VirtualMachineIPAddress { + return nil, nil + } + + if newVM.Spec.VirtualMachineIPAddress == "" { + return nil, fmt.Errorf("spec.virtualMachineIPAddress cannot be changed to an empty value once set") + } + + vmipKey := types.NamespacedName{Name: newVM.Spec.VirtualMachineIPAddress, Namespace: newVM.Namespace} + vmip, err := helper.FetchObject(ctx, vmipKey, v.client, &v1alpha2.VirtualMachineIPAddress{}) + if err != nil { + return nil, fmt.Errorf("unable to get VirtualMachineIPAddress %s: %w", vmipKey, err) + } + + if vmip == nil { + return nil, nil + } + + return nil, v.ipam.CheckIpAddressAvailableForBinding(newVM.Name, vmip) +} diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/meta_validator.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/meta_validator.go new file mode 100644 index 000000000..b6db3b0a3 --- /dev/null +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/meta_validator.go @@ -0,0 +1,69 @@ +/* +Copyright 2024 Flant JSC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validators + +import ( + "context" + "fmt" + "strings" + + "kubevirt.io/api/core" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + "github.com/deckhouse/virtualization/api/core/v1alpha2" +) + +type MetaValidator struct { + client client.Client +} + +func NewMetaValidator(client client.Client) *MetaValidator { + return &MetaValidator{client: client} +} + +func (v *MetaValidator) ValidateCreate(_ context.Context, vm *v1alpha2.VirtualMachine) (admission.Warnings, error) { + for key := range vm.Annotations { + if strings.Contains(key, core.GroupName) { + return nil, fmt.Errorf("using the %s group's name in the annotation is prohibited", core.GroupName) + } + } + + for key := range vm.Labels { + if strings.Contains(key, core.GroupName) { + return nil, fmt.Errorf("using the %s group's name in the label is prohibited", core.GroupName) + } + } + + return nil, nil +} + +func (v *MetaValidator) ValidateUpdate(_ context.Context, _, newVM *v1alpha2.VirtualMachine) (admission.Warnings, error) { + for key := range newVM.Annotations { + if strings.Contains(key, core.GroupName) { + return nil, fmt.Errorf("using the %s group's name in the annotation is prohibited", core.GroupName) + } + } + + for key := range newVM.Labels { + if strings.Contains(key, core.GroupName) { + return nil, fmt.Errorf("using the %s group's name in the label is prohibited", core.GroupName) + } + } + + return nil, nil +} diff --git a/images/virtualization-artifact/pkg/controller/vm/vm_webhook.go b/images/virtualization-artifact/pkg/controller/vm/vm_webhook.go index 2674e5070..7b8bf0d23 100644 --- a/images/virtualization-artifact/pkg/controller/vm/vm_webhook.go +++ b/images/virtualization-artifact/pkg/controller/vm/vm_webhook.go @@ -20,29 +20,32 @@ import ( "context" "fmt" "log/slog" - "strings" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" - "kubevirt.io/api/core" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "github.com/deckhouse/virtualization-controller/pkg/controller/vm/internal" - "github.com/deckhouse/virtualization-controller/pkg/sdk/framework/helper" + "github.com/deckhouse/virtualization-controller/pkg/controller/vm/internal/validators" "github.com/deckhouse/virtualization/api/core/v1alpha2" ) +type VirtualMachineValidator interface { + ValidateCreate(ctx context.Context, vm *v1alpha2.VirtualMachine) (admission.Warnings, error) + ValidateUpdate(ctx context.Context, oldVM, newVM *v1alpha2.VirtualMachine) (admission.Warnings, error) +} + type Validator struct { - validators []vmValidator + validators []VirtualMachineValidator log *slog.Logger } func NewValidator(ipam internal.IPAM, client client.Client, log *slog.Logger) *Validator { return &Validator{ - validators: []vmValidator{ - newMetaVMValidator(client), - newIPAMVMValidator(ipam, client), + validators: []VirtualMachineValidator{ + validators.NewMetaValidator(client), + validators.NewIPAMValidator(ipam, client), + validators.NewBlockDeviceSpecRefsValidator(), }, log: log.With("webhook", "validation"), } @@ -59,7 +62,7 @@ func (v *Validator) ValidateCreate(ctx context.Context, obj runtime.Object) (adm var warnings admission.Warnings for _, validator := range v.validators { - warn, err := validator.validateCreate(ctx, vm) + warn, err := validator.ValidateCreate(ctx, vm) if err != nil { return nil, err } @@ -88,7 +91,7 @@ func (v *Validator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.O var warnings admission.Warnings for _, validator := range v.validators { - warn, err := validator.validateUpdate(ctx, oldVM, newVM) + warn, err := validator.ValidateUpdate(ctx, oldVM, newVM) if err != nil { return nil, err } @@ -103,104 +106,3 @@ func (v *Validator) ValidateDelete(_ context.Context, _ runtime.Object) (admissi v.log.Error("Ensure the correctness of ValidatingWebhookConfiguration", "err", err.Error()) return nil, nil } - -type vmValidator interface { - validateCreate(ctx context.Context, vm *v1alpha2.VirtualMachine) (admission.Warnings, error) - validateUpdate(ctx context.Context, oldVM, newVM *v1alpha2.VirtualMachine) (admission.Warnings, error) -} - -type metaVMValidator struct { - client client.Client -} - -func newMetaVMValidator(client client.Client) *metaVMValidator { - return &metaVMValidator{client: client} -} - -func (v *metaVMValidator) validateCreate(_ context.Context, vm *v1alpha2.VirtualMachine) (admission.Warnings, error) { - for key := range vm.Annotations { - if strings.Contains(key, core.GroupName) { - return nil, fmt.Errorf("using the %s group's name in the annotation is prohibited", core.GroupName) - } - } - - for key := range vm.Labels { - if strings.Contains(key, core.GroupName) { - return nil, fmt.Errorf("using the %s group's name in the label is prohibited", core.GroupName) - } - } - - return nil, nil -} - -func (v *metaVMValidator) validateUpdate(_ context.Context, _, newVM *v1alpha2.VirtualMachine) (admission.Warnings, error) { - for key := range newVM.Annotations { - if strings.Contains(key, core.GroupName) { - return nil, fmt.Errorf("using the %s group's name in the annotation is prohibited", core.GroupName) - } - } - - for key := range newVM.Labels { - if strings.Contains(key, core.GroupName) { - return nil, fmt.Errorf("using the %s group's name in the label is prohibited", core.GroupName) - } - } - - return nil, nil -} - -type ipamVMValidator struct { - ipam internal.IPAM - client client.Client -} - -func newIPAMVMValidator(ipam internal.IPAM, client client.Client) *ipamVMValidator { - return &ipamVMValidator{ipam: ipam, client: client} -} - -func (v *ipamVMValidator) validateCreate(ctx context.Context, vm *v1alpha2.VirtualMachine) (admission.Warnings, error) { - vmipName := vm.Spec.VirtualMachineIPAddress - if vmipName == "" { - vmipName = vm.Name - } - - vmipKey := types.NamespacedName{Name: vmipName, Namespace: vm.Namespace} - vmip, err := helper.FetchObject(ctx, vmipKey, v.client, &v1alpha2.VirtualMachineIPAddress{}) - if err != nil { - return nil, fmt.Errorf("unable to get referenced VirtualMachineIPAddress %s: %w", vmipKey, err) - } - - if vmip == nil { - return nil, nil - } - - // VM is created without ip address, but ip address resource is already exists. - if vm.Spec.VirtualMachineIPAddress == "" { - return nil, fmt.Errorf("VirtualMachineIPAddress with the name of the virtual machine"+ - " already exists: set spec.virtualMachineIPAddress field to %s to use IP %s", vmip.Name, vmip.Status.Address) - } - - return nil, v.ipam.CheckIpAddressAvailableForBinding(vm.Name, vmip) -} - -func (v *ipamVMValidator) validateUpdate(ctx context.Context, oldVM, newVM *v1alpha2.VirtualMachine) (admission.Warnings, error) { - if oldVM.Spec.VirtualMachineIPAddress == newVM.Spec.VirtualMachineIPAddress { - return nil, nil - } - - if newVM.Spec.VirtualMachineIPAddress == "" { - return nil, fmt.Errorf("spec.virtualMachineIPAddress cannot be changed to an empty value once set") - } - - vmipKey := types.NamespacedName{Name: newVM.Spec.VirtualMachineIPAddress, Namespace: newVM.Namespace} - vmip, err := helper.FetchObject(ctx, vmipKey, v.client, &v1alpha2.VirtualMachineIPAddress{}) - if err != nil { - return nil, fmt.Errorf("unable to get VirtualMachineIPAddress %s: %w", vmipKey, err) - } - - if vmip == nil { - return nil, nil - } - - return nil, v.ipam.CheckIpAddressAvailableForBinding(newVM.Name, vmip) -} diff --git a/images/virtualization-artifact/pkg/controller/vmbda/internal/life_cycle.go b/images/virtualization-artifact/pkg/controller/vmbda/internal/life_cycle.go index c955abaa6..f559548f8 100644 --- a/images/virtualization-artifact/pkg/controller/vmbda/internal/life_cycle.go +++ b/images/virtualization-artifact/pkg/controller/vmbda/internal/life_cycle.go @@ -94,9 +94,9 @@ func (h LifeCycleHandler) Handle(ctx context.Context, vmbda *virtv2.VirtualMachi } // Final phase: avoid processing VMDBA that had a conflict with another VMDBA previously. - if vmbda.Status.Phase == virtv2.BlockDeviceAttachmentPhaseFailed { - return reconcile.Result{}, nil - } + // if vmbda.Status.Phase == virtv2.BlockDeviceAttachmentPhaseFailed { + // return reconcile.Result{}, nil + // } isConflicted, conflictWithName, err := h.attacher.IsConflictedAttachment(ctx, vmbda) if err != nil { @@ -205,39 +205,47 @@ func (h LifeCycleHandler) Handle(ctx context.Context, vmbda *virtv2.VirtualMachi return reconcile.Result{}, nil } - isHotPlugRequestSent, err := h.attacher.IsHotPlugRequestSent(vd, kvvm) - if err != nil { - return reconcile.Result{}, err - } + _, err = h.attacher.CanHotPlug(vd, vm, kvvm) + switch { + case err == nil: + log.Info("Send attachment request") - if isHotPlugRequestSent { - log.Info("Attachment request sent: attachment is in progress.") + err = h.attacher.HotPlugDisk(ctx, vd, vm, kvvm) + if err != nil { + return reconcile.Result{}, err + } vmbda.Status.Phase = virtv2.BlockDeviceAttachmentPhaseInProgress condition.Status = metav1.ConditionFalse condition.Reason = vmbdacondition.AttachmentRequestSent - condition.Message = "Attachment request sent: attachment is in progress." + condition.Message = "Attachment request has sent: attachment is in progress." return reconcile.Result{}, nil - } + case errors.Is(err, service.ErrDiskIsSpecAttached): + log.Info("VirtualDisk is already attached to the virtual machine spec.") - log.Info("Send attachment request") + vmbda.Status.Phase = virtv2.BlockDeviceAttachmentPhaseFailed + condition.Status = metav1.ConditionFalse + condition.Reason = vmbdacondition.Conflict + condition.Message = service.CapitalizeFirstLetter(err.Error()) + return reconcile.Result{}, nil + case errors.Is(err, service.ErrHotPlugRequestAlreadySent): + log.Info("Attachment request sent: attachment is in progress.") - err = h.attacher.HotPlugDisk(ctx, vd, vm, kvvm) - switch { - case err == nil: vmbda.Status.Phase = virtv2.BlockDeviceAttachmentPhaseInProgress condition.Status = metav1.ConditionFalse condition.Reason = vmbdacondition.AttachmentRequestSent - condition.Message = "Attachment request has sent: attachment is in progress." + condition.Message = "Attachment request sent: attachment is in progress." return reconcile.Result{}, nil - case errors.Is(err, service.ErrVirtualDiskIsAlreadyAttached), - errors.Is(err, service.ErrVirtualMachineWaitsForRestartApproval): + case errors.Is(err, service.ErrVirtualMachineWaitsForRestartApproval): + log.Info("Virtual machine waits for restart approval") + vmbda.Status.Phase = virtv2.BlockDeviceAttachmentPhasePending condition.Status = metav1.ConditionFalse condition.Reason = vmbdacondition.NotAttached condition.Message = service.CapitalizeFirstLetter(err.Error()) return reconcile.Result{}, nil default: + return reconcile.Result{}, err } }