From 7f6723e7b16aa456cab3a3ceffcd9612cbfb971d Mon Sep 17 00:00:00 2001 From: j-griffith Date: Thu, 21 Jun 2018 11:14:12 -0600 Subject: [PATCH] Add optional serial number param to disk creation Adds an optional parameter to the Disk object (Serial) to allow setting/maintaining a constant Serial number when creating the Disk XML for Libvirt. This includes unit tests as well as the addition of a functional test (thanks to the suggestion from rmohr) that uses the cirros image. Fixes #1133 Signed-off-by: John Griffith jgriffith@redhat.com --- api/openapi-spec/swagger.json | 4 ++ manifests/generated/vm-resource.yaml | 2 + manifests/generated/vmi-resource.yaml | 2 + manifests/generated/vmipreset-resource.yaml | 2 + manifests/generated/vmirs-resource.yaml | 2 + pkg/api/v1/openapi_generated.go | 7 +++ pkg/api/v1/schema.go | 3 + pkg/api/v1/schema_swagger_generated.go | 1 + pkg/api/v1/schema_test.go | 19 ++++++ .../validating-webhook/validating-webhook.go | 21 +++++++ .../validating-webhook_test.go | 60 +++++++++++++++++++ tests/storage_test.go | 47 +++++++++++++++ 12 files changed, 170 insertions(+) diff --git a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json index 833ea2f4b50a..aa972b0ab47d 100644 --- a/api/openapi-spec/swagger.json +++ b/api/openapi-spec/swagger.json @@ -3487,6 +3487,10 @@ "description": "Name is the device name", "type": "string" }, + "serial": { + "description": "Serial provides the ability to specify a serial number for the disk device.\n+optional", + "type": "string" + }, "volumeName": { "description": "Name of the volume which is referenced.\nMust match the Name of a Volume.", "type": "string" diff --git a/manifests/generated/vm-resource.yaml b/manifests/generated/vm-resource.yaml index 35e3bf8bead0..bf880103e7ce 100644 --- a/manifests/generated/vm-resource.yaml +++ b/manifests/generated/vm-resource.yaml @@ -90,6 +90,8 @@ spec: type: boolean name: type: string + serial: + type: string volumeName: type: string required: diff --git a/manifests/generated/vmi-resource.yaml b/manifests/generated/vmi-resource.yaml index c8b02bf36cc2..19696633b85f 100644 --- a/manifests/generated/vmi-resource.yaml +++ b/manifests/generated/vmi-resource.yaml @@ -83,6 +83,8 @@ spec: type: boolean name: type: string + serial: + type: string volumeName: type: string required: diff --git a/manifests/generated/vmipreset-resource.yaml b/manifests/generated/vmipreset-resource.yaml index d93f0dd94f5f..2ea3b3f37a35 100644 --- a/manifests/generated/vmipreset-resource.yaml +++ b/manifests/generated/vmipreset-resource.yaml @@ -78,6 +78,8 @@ spec: type: boolean name: type: string + serial: + type: string volumeName: type: string required: diff --git a/manifests/generated/vmirs-resource.yaml b/manifests/generated/vmirs-resource.yaml index eadcd49f2bf5..6ca48e20a7ee 100644 --- a/manifests/generated/vmirs-resource.yaml +++ b/manifests/generated/vmirs-resource.yaml @@ -94,6 +94,8 @@ spec: type: boolean name: type: string + serial: + type: string volumeName: type: string required: diff --git a/pkg/api/v1/openapi_generated.go b/pkg/api/v1/openapi_generated.go index 7f9561603156..d1ee738a7b4c 100644 --- a/pkg/api/v1/openapi_generated.go +++ b/pkg/api/v1/openapi_generated.go @@ -290,6 +290,13 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA Format: "int32", }, }, + "serial": { + SchemaProps: spec.SchemaProps{ + Description: "Serial provides the ability to specify a serial number for the disk device.", + Type: []string{"string"}, + Format: "", + }, + }, }, Required: []string{"name", "volumeName"}, }, diff --git a/pkg/api/v1/schema.go b/pkg/api/v1/schema.go index 897e2120500f..a39e6c1a1f42 100644 --- a/pkg/api/v1/schema.go +++ b/pkg/api/v1/schema.go @@ -184,6 +184,9 @@ type Disk struct { // Disks without a boot order are not tried if a disk with a boot order exists. // +optional BootOrder *uint `json:"bootOrder,omitempty"` + // Serial provides the ability to specify a serial number for the disk device. + // +optional + Serial string `json:"serial,omitempty"` } // Represents the target of a volume to mount. diff --git a/pkg/api/v1/schema_swagger_generated.go b/pkg/api/v1/schema_swagger_generated.go index a19f06128412..e077a4ca4389 100644 --- a/pkg/api/v1/schema_swagger_generated.go +++ b/pkg/api/v1/schema_swagger_generated.go @@ -90,6 +90,7 @@ func (Disk) SwaggerDoc() map[string]string { "name": "Name is the device name", "volumeName": "Name of the volume which is referenced.\nMust match the Name of a Volume.", "bootOrder": "BootOrder is an integer value > 0, used to determine ordering of boot devices.\nLower values take precedence.\nDisks without a boot order are not tried if a disk with a boot order exists.\n+optional", + "serial": "Serial provides the ability to specify a serial number for the disk device.\n+optional", } } diff --git a/pkg/api/v1/schema_test.go b/pkg/api/v1/schema_test.go index 9ddba4eec771..ec5f1916f6f5 100644 --- a/pkg/api/v1/schema_test.go +++ b/pkg/api/v1/schema_test.go @@ -150,6 +150,14 @@ var exampleJSON = `{ "bus": "virtio", "readonly": true } + }, + { + "name": "disk1", + "volumeName": "volume4", + "disk": { + "bus": "virtio" + }, + "serial": "sn-11223344" } ], "interfaces": [ @@ -240,6 +248,17 @@ var _ = Describe("Schema", func() { }, }, }, + { + Name: "disk1", + VolumeName: "volume4", + Serial: "sn-11223344", + DiskDevice: DiskDevice{ + Disk: &DiskTarget{ + Bus: "virtio", + ReadOnly: false, + }, + }, + }, } exampleVMI.Spec.Volumes = []Volume{ diff --git a/pkg/virt-api/validating-webhook/validating-webhook.go b/pkg/virt-api/validating-webhook/validating-webhook.go index 84d8ed37645b..c6ae89fa64db 100644 --- a/pkg/virt-api/validating-webhook/validating-webhook.go +++ b/pkg/virt-api/validating-webhook/validating-webhook.go @@ -25,6 +25,7 @@ import ( "fmt" "io/ioutil" "net/http" + "regexp" "k8s.io/apimachinery/pkg/api/resource" @@ -42,6 +43,7 @@ import ( const ( cloudInitMaxLen = 2048 arrayLenMax = 256 + maxStrLen = 256 ) var validInterfaceModels = []string{"e1000", "e1000e", "ne2k_pci", "pcnet", "rtl8139", "virtio"} @@ -187,6 +189,25 @@ func validateDisks(field *k8sfield.Path, disks []v1.Disk) []metav1.StatusCause { Field: field.Index(idx).Child("bootorder").String(), }) } + + // Verify serial number is made up of valid characters for libvirt, if provided + isValid := regexp.MustCompile(`^[A-Za-z0-9_.+-]+$`).MatchString + if disk.Serial != "" && !isValid(disk.Serial) { + causes = append(causes, metav1.StatusCause{ + Type: metav1.CauseTypeFieldValueInvalid, + Message: fmt.Sprintf("%s must be made up of the following characters [A-Za-z0-9_.+-], if specified", field.Index(idx).String()), + Field: field.Index(idx).Child("serial").String(), + }) + } + + // Verify serial number is within valid length, if provided + if disk.Serial != "" && len([]rune(disk.Serial)) > maxStrLen { + causes = append(causes, metav1.StatusCause{ + Type: metav1.CauseTypeFieldValueInvalid, + Message: fmt.Sprintf("%s must be less than or equal to %d in length, if specified", field.Index(idx).String(), maxStrLen), + Field: field.Index(idx).Child("serial").String(), + }) + } } return causes diff --git a/pkg/virt-api/validating-webhook/validating-webhook_test.go b/pkg/virt-api/validating-webhook/validating-webhook_test.go index 8693da4d4ba8..d96d81086d40 100644 --- a/pkg/virt-api/validating-webhook/validating-webhook_test.go +++ b/pkg/virt-api/validating-webhook/validating-webhook_test.go @@ -23,6 +23,7 @@ import ( "encoding/base64" "encoding/json" "fmt" + "strings" . "github.com/onsi/ginkgo" "github.com/onsi/ginkgo/extensions/table" @@ -1044,6 +1045,65 @@ var _ = Describe("Validating Webhook", func() { Expect(len(causes)).To(Equal(1)) Expect(causes[0].Field).To(Equal("fake[0].bootorder")) }) + + It("should reject invalid SN characters", func() { + vmi := v1.NewMinimalVMI("testvmi") + order := uint(1) + sn := "$$$$" + + vmi.Spec.Domain.Devices.Disks = append(vmi.Spec.Domain.Devices.Disks, v1.Disk{ + Name: "testdisk2", + VolumeName: "testvolume2", + BootOrder: &order, + Serial: sn, + DiskDevice: v1.DiskDevice{ + Disk: &v1.DiskTarget{}, + }, + }) + + causes := validateDisks(k8sfield.NewPath("fake"), vmi.Spec.Domain.Devices.Disks) + Expect(len(causes)).To(Equal(1)) + Expect(causes[0].Field).To(Equal("fake[0].serial")) + }) + + It("should reject SN > maxStrLen characters", func() { + vmi := v1.NewMinimalVMI("testvmi") + order := uint(1) + sn := strings.Repeat("1", maxStrLen+1) + + vmi.Spec.Domain.Devices.Disks = append(vmi.Spec.Domain.Devices.Disks, v1.Disk{ + Name: "testdisk2", + VolumeName: "testvolume2", + BootOrder: &order, + Serial: sn, + DiskDevice: v1.DiskDevice{ + Disk: &v1.DiskTarget{}, + }, + }) + + causes := validateDisks(k8sfield.NewPath("fake"), vmi.Spec.Domain.Devices.Disks) + Expect(len(causes)).To(Equal(1)) + Expect(causes[0].Field).To(Equal("fake[0].serial")) + }) + + It("should accept valid SN", func() { + vmi := v1.NewMinimalVMI("testvmi") + order := uint(1) + + vmi.Spec.Domain.Devices.Disks = append(vmi.Spec.Domain.Devices.Disks, v1.Disk{ + Name: "testdisk2", + VolumeName: "testvolume2", + BootOrder: &order, + Serial: "SN-1_a", + DiskDevice: v1.DiskDevice{ + Disk: &v1.DiskTarget{}, + }, + }) + + causes := validateDisks(k8sfield.NewPath("fake"), vmi.Spec.Domain.Devices.Disks) + Expect(len(causes)).To(Equal(0)) + }) + }) }) diff --git a/tests/storage_test.go b/tests/storage_test.go index 0c457ea212cf..df732e46f166 100644 --- a/tests/storage_test.go +++ b/tests/storage_test.go @@ -21,6 +21,7 @@ package tests_test import ( "flag" + "fmt" "time" "github.com/google/goexpect" @@ -36,6 +37,10 @@ import ( "kubevirt.io/kubevirt/tests" ) +const ( + diskSerial = "FB-fb_18030C10002032" +) + type VMICreationFunc func(string) *v1.VirtualMachineInstance var _ = Describe("Storage", func() { @@ -155,6 +160,48 @@ var _ = Describe("Storage", func() { }) + Context("With an emptyDisk defined and a specified serial number", func() { + // The following case is mostly similar to the alpine PVC test above, except using different VirtualMachineInstance. + It("should create a writeable emptyDisk with the specified serial number", func() { + + // Start the VirtualMachineInstance with the empty disk attached + vmi := tests.NewRandomVMIWithEphemeralDiskAndUserdata(tests.RegistryDiskFor(tests.RegistryDiskCirros), "echo hi!") + vmi.Spec.Domain.Devices.Disks = append(vmi.Spec.Domain.Devices.Disks, v1.Disk{ + Name: "emptydisk1", + VolumeName: "emptydiskvolume1", + Serial: diskSerial, + DiskDevice: v1.DiskDevice{ + Disk: &v1.DiskTarget{ + Bus: "virtio", + }, + }, + }) + vmi.Spec.Volumes = append(vmi.Spec.Volumes, v1.Volume{ + Name: "emptydiskvolume1", + VolumeSource: v1.VolumeSource{ + EmptyDisk: &v1.EmptyDiskSource{ + Capacity: resource.MustParse("1Gi"), + }, + }, + }) + RunVMIAndExpectLaunch(vmi, false, 90) + + expecter, err := tests.LoggedInCirrosExpecter(vmi) + Expect(err).To(BeNil()) + defer expecter.Close() + + snQuery := fmt.Sprintf("sudo find /sys -type f -regex \".*/block/vdb/serial\" | xargs cat && echo %s\n", diskSerial) + By("Checking for the specified serial number") + res, err := expecter.ExpectBatch([]expect.Batcher{ + &expect.BSnd{S: snQuery}, + &expect.BExp{R: diskSerial}, + }, 10*time.Second) + log.DefaultLogger().Object(vmi).Infof("%v", res) + Expect(err).To(BeNil()) + }) + + }) + Context("With ephemeral alpine PVC", func() { // The following case is mostly similar to the alpine PVC test above, except using different VirtualMachineInstance. It("should be successfully started", func() {