Skip to content

Commit

Permalink
Adjust update vmi webhooks to new memory dump volume source
Browse files Browse the repository at this point in the history 
This volume source doesnt have a matching disk, need to take
that into account in the checks verifications

Signed-off-by: Shelly Kagan <[email protected]>
  • Loading branch information
@ShellyKa13
ShellyKa13 committed May 24, 2022
1 parent 32d201b commit c50137b
Show file tree
Hide file tree
Showing 5 changed files with 156 additions and 36 deletions.
13 changes: 13 additions & 0 deletions pkg/testutils/mock_config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"k8s.io/apimachinery/pkg/util/rand"
"k8s.io/client-go/tools/cache"

k8score "k8s.io/api/core/v1"

KVv1 "kubevirt.io/api/core/v1"
virtconfig "kubevirt.io/kubevirt/pkg/virt-config"
)
Expand Down Expand Up @@ -57,6 +59,17 @@ func NewFakeContainerDiskSource() *KVv1.ContainerDiskSource {
}
}

func NewFakeMemoryDumpSource() *KVv1.MemoryDumpVolumeSource {
return &KVv1.MemoryDumpVolumeSource{
PersistentVolumeClaimVolumeSource: KVv1.PersistentVolumeClaimVolumeSource{
PersistentVolumeClaimVolumeSource: k8score.PersistentVolumeClaimVolumeSource{
ClaimName: "fake-memory-dump",
},
Hotpluggable: true,
},
}
}

func RemoveDataVolumeAPI(crdInformer cache.SharedIndexInformer) {
crdInformer.GetStore().Replace(nil, "")
}
Expand Down
15 changes: 15 additions & 0 deletions pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,9 @@ func validateVirtualMachineInstanceSpecVolumeDisks(field *k8sfield.Path, spec *v

// Validate that volumes match disks and filesystems correctly
for idx, volume := range spec.Volumes {
if volume.MemoryDump != nil {
continue
}
if _, matchingDiskExists := diskAndFilesystemNames[volume.Name]; !matchingDiskExists {
causes = append(causes, metav1.StatusCause{
Type: metav1.CauseTypeFieldValueInvalid,
Expand Down Expand Up @@ -1848,6 +1851,7 @@ func validateVolumes(field *k8sfield.Path, volumes []v1.Volume, config *virtconf
// check that we have max 1 instance of below disks
serviceAccountVolumeCount := 0
downwardMetricVolumeCount := 0
memoryDumpVolumeCount := 0

for idx, volume := range volumes {
// verify name is unique
Expand Down Expand Up @@ -1923,6 +1927,10 @@ func validateVolumes(field *k8sfield.Path, volumes []v1.Volume, config *virtconf
downwardMetricVolumeCount++
volumeSourceSetCount++
}
if volume.MemoryDump != nil {
memoryDumpVolumeCount++
volumeSourceSetCount++
}

if volumeSourceSetCount != 1 {
causes = append(causes, metav1.StatusCause{
Expand Down Expand Up @@ -2129,6 +2137,13 @@ func validateVolumes(field *k8sfield.Path, volumes []v1.Volume, config *virtconf
Field: field.String(),
})
}
if memoryDumpVolumeCount > 1 {
causes = append(causes, metav1.StatusCause{
Type: metav1.CauseTypeFieldValueInvalid,
Message: fmt.Sprintf("%s must have max one memory dump volume set", field.String()),
Field: field.String(),
})
}

return causes
}
Expand Down
34 changes: 34 additions & 0 deletions pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3330,6 +3330,40 @@ var _ = Describe("Validating VMICreate Admitter", func() {
causes := validateVolumes(k8sfield.NewPath("fake"), vmi.Spec.Volumes, config)
Expect(causes).To(BeEmpty())
})
It("should accept a single memoryDump volume without a matching disk", func() {
vmi := api.NewMinimalVMI("testvmi")

vmi.Spec.Volumes = append(vmi.Spec.Volumes, v1.Volume{
Name: "testMemoryDump",
VolumeSource: v1.VolumeSource{
MemoryDump: testutils.NewFakeMemoryDumpSource(),
},
})

causes := validateVolumes(k8sfield.NewPath("fake"), vmi.Spec.Volumes, config)
Expect(causes).To(BeEmpty())
})
It("should reject memoryDump volumes if more than one exist", func() {
vmi := api.NewMinimalVMI("testvmi")

vmi.Spec.Volumes = append(vmi.Spec.Volumes,
v1.Volume{
Name: "testMemoryDump",
VolumeSource: v1.VolumeSource{
MemoryDump: testutils.NewFakeMemoryDumpSource(),
},
},
v1.Volume{
Name: "testMemoryDump2",
VolumeSource: v1.VolumeSource{
MemoryDump: testutils.NewFakeMemoryDumpSource(),
},
},
)
causes := validateVolumes(k8sfield.NewPath("fake"), vmi.Spec.Volumes, config)
Expect(causes).To(HaveLen(1))
Expect(causes[0].Message).To(ContainSubstring("fake must have max one memory dump volume set"))
})

})

Expand Down
87 changes: 51 additions & 36 deletions pkg/virt-api/webhooks/validating-webhook/admitters/vmi-update-admitter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,24 @@ func (admitter *VMIUpdateAdmitter) Admit(ar *admissionv1.AdmissionReview) *admis
return &reviewResponse
}

func getExpectedDisks(newVolumes []v1.Volume) int {
numMemoryDumpVolumes := 0
for _, volume := range newVolumes {
if volume.MemoryDump != nil {
numMemoryDumpVolumes = numMemoryDumpVolumes + 1
}
}
return len(newVolumes) - numMemoryDumpVolumes
}

// admitHotplug compares the old and new volumes and disks, and ensures that they match and are valid.
func admitHotplug(newVolumes, oldVolumes []v1.Volume, newDisks, oldDisks []v1.Disk, volumeStatuses []v1.VolumeStatus, newVMI *v1.VirtualMachineInstance, config *virtconfig.ClusterConfig) *admissionv1.AdmissionResponse {
if len(newVolumes) != len(newDisks) {
expectedDisks := getExpectedDisks(newVolumes)
if expectedDisks != len(newDisks) {
return webhookutils.ToAdmissionResponse([]metav1.StatusCause{
{
Type: metav1.CauseTypeFieldValueInvalid,
Message: fmt.Sprintf("number of disks (%d) does not equal the number of volumes (%d)", len(newDisks), len(newVolumes)),
Message: fmt.Sprintf("number of disks (%d) does not equal the number of volumes (%d)", len(newDisks), expectedDisks),
},
})
}
Expand Down Expand Up @@ -124,50 +135,54 @@ func verifyHotplugVolumes(newHotplugVolumeMap, oldHotplugVolumeMap map[string]v1
},
})
}
if _, ok := newDisks[k]; !ok {
return webhookutils.ToAdmissionResponse([]metav1.StatusCause{
{
Type: metav1.CauseTypeFieldValueInvalid,
Message: fmt.Sprintf("Volume %s doesn't have a matching disk", k),
},
})
}
if !equality.Semantic.DeepEqual(newDisks[k], oldDisks[k]) {
return webhookutils.ToAdmissionResponse([]metav1.StatusCause{
{
Type: metav1.CauseTypeFieldValueInvalid,
Message: fmt.Sprintf("hotplug disk %s, changed", k),
},
})
if v.MemoryDump == nil {
if _, ok := newDisks[k]; !ok {
return webhookutils.ToAdmissionResponse([]metav1.StatusCause{
{
Type: metav1.CauseTypeFieldValueInvalid,
Message: fmt.Sprintf("Volume %s doesn't have a matching disk", k),
},
})
}
if !equality.Semantic.DeepEqual(newDisks[k], oldDisks[k]) {
return webhookutils.ToAdmissionResponse([]metav1.StatusCause{
{
Type: metav1.CauseTypeFieldValueInvalid,
Message: fmt.Sprintf("hotplug disk %s, changed", k),
},
})
}
}
} else {
// This is a new volume, ensure that the volume is either DV or PVC
if v.DataVolume == nil && v.PersistentVolumeClaim == nil {
// This is a new volume, ensure that the volume is either DV, PVC or memoryDumpVolume
if v.DataVolume == nil && v.PersistentVolumeClaim == nil && v.MemoryDump == nil {
return webhookutils.ToAdmissionResponse([]metav1.StatusCause{
{
Type: metav1.CauseTypeFieldValueInvalid,
Message: fmt.Sprintf("volume %s is not a PVC or DataVolume", k),
},
})
}
// Also ensure the matching new disk exists and is of type scsi
if _, ok := newDisks[k]; !ok {
return webhookutils.ToAdmissionResponse([]metav1.StatusCause{
{
Type: metav1.CauseTypeFieldValueInvalid,
Message: fmt.Sprintf("Disk %s does not exist", k),
},
})
}
disk := newDisks[k]
if disk.Disk == nil || disk.Disk.Bus != "scsi" {
return webhookutils.ToAdmissionResponse([]metav1.StatusCause{
{
Type: metav1.CauseTypeFieldValueInvalid,
Message: fmt.Sprintf("hotplugged Disk %s does not use a scsi bus", k),
},
})
if v.MemoryDump == nil {
// Also ensure the matching new disk exists and is of type scsi
if _, ok := newDisks[k]; !ok {
return webhookutils.ToAdmissionResponse([]metav1.StatusCause{
{
Type: metav1.CauseTypeFieldValueInvalid,
Message: fmt.Sprintf("Disk %s does not exist", k),
},
})
}
disk := newDisks[k]
if disk.Disk == nil || disk.Disk.Bus != "scsi" {
return webhookutils.ToAdmissionResponse([]metav1.StatusCause{
{
Type: metav1.CauseTypeFieldValueInvalid,
Message: fmt.Sprintf("hotplugged Disk %s does not use a scsi bus", k),
},
})

}
}
}
}
Expand Down
43 changes: 43 additions & 0 deletions pkg/virt-api/webhooks/validating-webhook/admitters/vmi-update-admitter_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,35 @@ var _ = Describe("Validating VMIUpdate Admitter", func() {
return res
}

makeVolumesWithMemoryDumpVol := func(total int, indexes ...int) []v1.Volume {
res := make([]v1.Volume, 0)
for i := 0; i < total; i++ {
memoryDump := false
for _, index := range indexes {
if i == index {
memoryDump = true
res = append(res, v1.Volume{
Name: fmt.Sprintf("volume-name-%d", index),
VolumeSource: v1.VolumeSource{
MemoryDump: testutils.NewFakeMemoryDumpSource(),
},
})
}
}
if !memoryDump {
res = append(res, v1.Volume{
Name: fmt.Sprintf("volume-name-%d", i),
VolumeSource: v1.VolumeSource{
DataVolume: &v1.DataVolumeSource{
Name: fmt.Sprintf("dv-name-%d", i),
},
},
})
}
}
return res
}

makeInvalidVolumes := func(total int, indexes ...int) []v1.Volume {
res := make([]v1.Volume, 0)
for i := 0; i < total; i++ {
Expand Down Expand Up @@ -486,6 +515,20 @@ var _ = Describe("Validating VMIUpdate Admitter", func() {
makeDisks(0),
makeStatus(1, 0),
makeExpected("spec.domain.devices.disks[1] must have a boot order > 0, if supplied", "spec.domain.devices.disks[1].bootOrder")),
Entry("Should accept if memory dump volume exists without matching disk",
makeVolumesWithMemoryDumpVol(3, 2),
makeVolumes(0, 1),
makeDisks(0, 1),
makeDisks(0, 1),
makeStatus(3, 1),
nil),
Entry("Should reject if #volumes != #disks even when there is memory dump volume",
makeVolumesWithMemoryDumpVol(3, 2),
makeVolumesWithMemoryDumpVol(3, 2),
makeDisks(1),
makeDisks(1),
makeStatus(0, 0),
makeExpected("number of disks (1) does not equal the number of volumes (2)", "")),
)

DescribeTable("Admit or deny based on user", func(user string, expected types.GomegaMatcher) {
Expand Down

0 comments on commit c50137b

Please sign in to comment.