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 kubevirt#1133 Signed-off-by: John Griffith [email protected]
portalgun-io · Jun 21, 2018 · 7f6723e · 7f6723e
1 parent 7ff4979
commit 7f6723e
Show file tree

Hide file tree

Showing 12 changed files with 170 additions and 0 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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() {