add http source for kubevirt VM Disk Image #1470

sankalp-r · 2022-10-31T11:43:09Z

What this PR does / why we need it:
This PR introduces PrimarDiskImageSource for the KubeVirt provider. The source can be HTTP or PVC-Clone.
If the source is HTTP with preAllocatedDataVolume as false, the VM disk-image will be downloaded from the HTTP source.
If the source is HTTP with preAllocatedDataVolume as true, the VM disk-image will be downloaded just once to create PreAllocatedDataVolume and VM Disks will be created by cloning over PreAllocatedDataVolume.
If the source is PVC-Clone, VM Disks will be created by cloning over custom-local-disks.

Which issue(s) this PR fixes:

Fixes #kubermatic/kubermatic#11234

What type of PR is this?

/kind feature

Special notes for your reviewer:

Does this PR introduce a user-facing change? Then add your Release Note here:

Add HTTP source for  KubeVirt VM Disk images which enables caching to avoid downloading images.

Documentation:

TBD

Signed-off-by: Sankalp Rangare sankalprangare786@gmail.com

sankalp-r · 2022-10-31T12:50:02Z

/retest

sankalp-r · 2022-11-07T21:25:02Z

/retest

mfranczy

I did a first review round. I will repeat after new changes.

mfranczy · 2022-11-09T13:16:42Z

pkg/cloudprovider/provider/kubevirt/provider.go

@@ -93,7 +98,7 @@ type Config struct {
 	CPUs                      string
 	Memory                    string
 	Namespace                 string
-	OsImage                   OSImage
+	OsImage                   *OSImage


Should be OSImage on the left side as well.

Btw why it has to be a pointer now?

mfranczy · 2022-11-09T13:17:06Z

pkg/cloudprovider/provider/kubevirt/provider.go

@@ -149,8 +154,18 @@ type SecondaryDisks struct {
 }

 type OSImage struct {


please add comments to each structure with explanation of the structutre itself and fields

mfranczy · 2022-11-09T13:18:35Z

pkg/cloudprovider/provider/kubevirt/provider.go

+
+type HTTPSource struct {
+	URL                    string
+	PreAllocatedDataVolume bool


maybe let's rename to CacheImage

mfranczy · 2022-11-09T13:18:44Z

pkg/cloudprovider/provider/kubevirt/provider.go

 	DataVolumeName string
+	NameSpace      string


Should be named Namespace

mfranczy · 2022-11-09T13:28:26Z

pkg/cloudprovider/provider/kubevirt/provider.go

+	}
+
+	var parsedOSImage *OSImage
+	// If PrimaryDiskOSSource is specified


I would extend this comment and say that if source is specified then that means we deal with new API

mfranczy · 2022-11-09T13:29:41Z

pkg/cloudprovider/provider/kubevirt/provider.go

+			parsedOSImage = &OSImage{PVCClone: &PVCSource{DataVolumeName: osImage, NameSpace: nameSpace}}
+		}
+	}
+	// If PrimaryDiskOSSource is not specified


We have to extend that comment and say that we are dealing here with older API and it's a matter of backward compatibility

mfranczy · 2022-11-09T13:33:35Z

pkg/cloudprovider/provider/kubevirt/provider.go

+	if !exist {
+		return sigClient.Create(ctx, newDataVolume(c, dvName, machine.Name))
+	}
+	// annotate existing DataVolume with VM-name, if already exist.


this comment is not needed, the code is quite clear

the same with the below comment

mfranczy · 2022-11-09T13:34:38Z

pkg/cloudprovider/provider/kubevirt/provider.go

+		return sigClient.Create(ctx, newDataVolume(c, dvName, machine.Name))
+	}
+	// annotate existing DataVolume with VM-name, if already exist.
+	updatedDV := existingDV.DeepCopy()


any reason for doing a DeepCopy in here? Controller runtime get function will return already a deep copy.

mfranczy · 2022-11-09T13:35:46Z

pkg/cloudprovider/provider/kubevirt/provider.go

-func (p *provider) Cleanup(ctx context.Context, machine *clusterv1alpha1.Machine, _ *cloudprovidertypes.ProviderData) (bool, error) {
+func (p *provider) createPreAllocatedDataVolume(ctx context.Context, c *Config, machine *clusterv1alpha1.Machine, sigClient client.Client, dvName string) error {
+	existingDV := &cdiv1beta1.DataVolume{}
+	exist := true


I think you can get rid of this flag, it's not required. You can check if the object is empty.

What I mean here is that you can return an error from Get and implement a check that if err is not found then create a new data volume.

mfranczy · 2022-11-09T13:38:23Z

pkg/cloudprovider/provider/kubevirt/types/types.go

+}
+
+// Source.
+type Source struct {


We need better comments with explanation about the fields.

mfranczy

Another review round.

mfranczy · 2022-11-09T14:46:30Z

pkg/cloudprovider/provider/kubevirt/provider.go

@@ -66,6 +69,8 @@ const (
 	// machineDeploymentLabelKey defines the label key used to contains as value the MachineDeployment name
 	// which machine comes from.
 	machineDeploymentLabelKey = "md"
+	// preAllocatedDVAnnotation defines the annotation for preAllocated DataVolume for cloning.
+	preAllocatedDVAnnotation = "datavolume.clone.kubevirt.io/machine"


let's change to disk-clone.machines.cluster.k8s.io/machine-%s

mfranczy · 2022-11-09T14:47:49Z

pkg/cloudprovider/provider/kubevirt/provider.go

@@ -679,9 +759,39 @@ func (p *provider) Cleanup(ctx context.Context, machine *clusterv1alpha1.Machine
 		return true, nil
 	}

+	// cleanup preAllocated DataVolume


not needed comment

mfranczy · 2022-11-09T14:50:29Z

pkg/cloudprovider/provider/kubevirt/provider.go

+		annotationKey := fmt.Sprintf("%s-%s", preAllocatedDVAnnotation, machine.Name)
+		if dv.Annotations[annotationKey] == "true" {
+			// If the Machine-Deployment is scaled to zero or deleted or preAllocateDataVolume is disabled, remove preAllocated DataVolume.
+			if len(dv.Annotations) == 1 {


let's keep it safe and change to <= 1

mfranczy · 2022-11-09T14:50:49Z

pkg/cloudprovider/provider/kubevirt/provider.go

+				return sigClient.Delete(ctx, dv)
+			}
+			// Remove annotation of corresponding VM from preAllocated DataVolume.
+			updatedDv := dv.DeepCopy()


you don't need deep copy

pkg/cloudprovider/provider/kubevirt/provider.go

mfranczy · 2022-11-09T14:53:50Z

pkg/cloudprovider/provider/kubevirt/provider.go

@@ -93,7 +98,7 @@ type Config struct {
 	CPUs                      string
 	Memory                    string
 	Namespace                 string
-	OsImage                   OSImage
+	OSImage                   *OSImage


Why it has to be a pointer now?

Notting specific, just parseOSImage() return pointer so I can directly assign it to config.OSImage
It does not affect the old API.

mfranczy · 2022-11-09T14:56:51Z

pkg/cloudprovider/provider/kubevirt/provider.go

+	var machineDeploymentName string
+	if mdName, err := machineDeploymentNameAndRevisionForMachineGetter(ctx, machine, data.Client)(); err == nil {
+		machineDeploymentName = mdName
+	}


I am not sure if I follow this logic.
Why it cannot be:

mdName, err := machineDeploymentNameAndRevisionForMachineGetter(ctx, machine, data.Client)() if err != nil { return nil, err }

Before this change, we used to create VMs even if getting the MD-name returned error.
So continue with earlier behavior.

so if MD-name returns an error then we have a problem. Based on that you create a data volume.

mfranczy · 2022-11-09T14:57:43Z

pkg/cloudprovider/provider/kubevirt/provider.go

+		if !kerrors.IsNotFound(err) {
+			return err
+		}
+		// create DV if it doesn't exist


I think we don't need comments like that, code is self explanatory

sankalp-r · 2022-11-10T11:06:26Z

/retest

mfranczy · 2022-11-10T11:52:24Z

pkg/cloudprovider/provider/kubevirt/types/types.go

+	PVCClone *PVCCloneSource `json:"pvcClone,omitempty"`
+}
+
+// HTTPSource.


We need better comments with explanation about the fields.

The same for all sources (HTTP and PVCClone)

mfranczy · 2022-11-10T12:34:57Z

I am sorry @sankalp-r I have to block this PR, we have to do another brainstorming session

/hold

mfranczy · 2022-11-14T09:08:27Z

pkg/cloudprovider/provider/kubevirt/provider.go

+
+// PVCSource represents pvc source for cloning VMImage.
+type PVCSource struct {
+	// DataVolumeName represent name of source pvc for cloning.
 	DataVolumeName string


let's change DataVolumeName to Name

sankalp-r · 2022-11-16T08:59:10Z

/retest

sankalp-r · 2022-11-16T13:46:17Z

/retest

sankalp-r · 2022-11-17T09:19:26Z

/retest

mfranczy · 2022-11-17T12:24:30Z

pkg/cloudprovider/provider/kubevirt/provider.go

@@ -66,6 +66,10 @@ const (
 	// machineDeploymentLabelKey defines the label key used to contains as value the MachineDeployment name
 	// which machine comes from.
 	machineDeploymentLabelKey = "md"
+	// httpSource defines the http source type for VM Disk Image.
+	httpSource = "http"


I would suggest to introduce imageSource type.

type imageSource string const ( ... httpSource imageSource = "http" pvcSource imageSource = "pvc" )

and later in the switch statement you would have

switch imageSource(osImageSource) ...

mfranczy · 2022-11-17T12:25:20Z

pkg/cloudprovider/provider/kubevirt/provider.go

+	case httpSource:
+		return &cdiv1beta1.DataVolumeSource{HTTP: &cdiv1beta1.DataVolumeSourceHTTP{URL: osImage}}, nil
+	case pvcSource:
+		if nameSpaceAndName := strings.Split(osImage, "/"); len(nameSpaceAndName) >= 2 {


please name it namespaceAndName (namespace is one word)

Signed-off-by: Sankalp Rangare <sankalprangare786@gmail.com>

mfranczy

/unhold
/approve
/lgtm

kubermatic-bot · 2022-11-17T13:53:45Z

LGTM label has been added.

Git tree hash: 908bfa286cec24942d7608b937967988b36079f9

sankalp-r · 2022-11-17T22:18:11Z

/retest

ahmedwaleedmalik

/approve ⚡

kubermatic-bot · 2022-11-21T08:13:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmedwaleedmalik, mfranczy, sankalp-r

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/cloudprovider/provider/OWNERS~~ [ahmedwaleedmalik]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

hdurand0710 · 2022-11-21T09:09:37Z

pkg/cloudprovider/provider/kubevirt/provider_test.go

@@ -167,7 +168,12 @@ func (k kubevirtProviderSpecConf) rawProviderSpec(t *testing.T) []byte {
 					"osImage": "http://x.y.z.t/ubuntu.img",
 					{{- end }}
 					"size": "10Gi",
+					{{- if .OsImageSource }}
+					"storageClassName": "longhorn",


As storageClassName is set to "longhorn" in the if and the else, it could go out of the if/else condition.

there is https://github.com/kubermatic/machine-controller/pull/1470/files#diff-2e539081e4b1bd387e1af416faf12022e219faba4f706b478b0f25523315a7a4R175

it could be just one if to add OsImageSource true, but the current state doesn't break the behaviour.

sankalp-r force-pushed the update-kubevirt-diskimagesource branch from d7a7115 to 4d5ebfb Compare October 31, 2022 11:47

sankalp-r changed the title ~~add http source for kubevirt diskImage~~ add http source for kubevirt VM Disk Image Oct 31, 2022

sankalp-r added the sig/virtualization Denotes a PR or issue as being assigned to SIG Virtualization. label Oct 31, 2022

sankalp-r force-pushed the update-kubevirt-diskimagesource branch from 4d5ebfb to 8db73b3 Compare October 31, 2022 13:20

sankalp-r requested a review from mfranczy October 31, 2022 13:43

sankalp-r changed the title ~~add http source for kubevirt VM Disk Image~~ [WIP] add http source for kubevirt VM Disk Image Nov 7, 2022

kubermatic-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 7, 2022

sankalp-r force-pushed the update-kubevirt-diskimagesource branch from 8db73b3 to c8f3062 Compare November 7, 2022 14:35

sankalp-r changed the title ~~[WIP] add http source for kubevirt VM Disk Image~~ add http source for kubevirt VM Disk Image Nov 7, 2022

kubermatic-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 7, 2022

sankalp-r force-pushed the update-kubevirt-diskimagesource branch 2 times, most recently from e40b7d3 to b61138f Compare November 7, 2022 16:07

mfranczy suggested changes Nov 9, 2022

View reviewed changes

kubermatic-bot assigned mfranczy Nov 9, 2022

sankalp-r force-pushed the update-kubevirt-diskimagesource branch from b61138f to ea080db Compare November 9, 2022 14:32

mfranczy suggested changes Nov 9, 2022

View reviewed changes

sankalp-r force-pushed the update-kubevirt-diskimagesource branch 2 times, most recently from ed2fb43 to 3721aa8 Compare November 9, 2022 19:15

mfranczy reviewed Nov 10, 2022

View reviewed changes

sankalp-r force-pushed the update-kubevirt-diskimagesource branch from 3721aa8 to 842b0cb Compare November 10, 2022 12:13

kubermatic-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 10, 2022

mfranczy reviewed Nov 14, 2022

View reviewed changes

sankalp-r force-pushed the update-kubevirt-diskimagesource branch 2 times, most recently from ea253d9 to 5293085 Compare November 15, 2022 17:11

sankalp-r requested review from mfranczy November 17, 2022 09:21

mfranczy reviewed Nov 17, 2022

View reviewed changes

sankalp-r force-pushed the update-kubevirt-diskimagesource branch 3 times, most recently from 419ff5d to 970c3b0 Compare November 17, 2022 13:37

add http source for kubevirt diskImage

9dc2b69

Signed-off-by: Sankalp Rangare <sankalprangare786@gmail.com>

sankalp-r force-pushed the update-kubevirt-diskimagesource branch from 970c3b0 to 9dc2b69 Compare November 17, 2022 13:47

mfranczy approved these changes Nov 17, 2022

View reviewed changes

kubermatic-bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 17, 2022

sankalp-r requested a review from ahmedwaleedmalik November 17, 2022 15:21

ahmedwaleedmalik approved these changes Nov 21, 2022

View reviewed changes

kubermatic-bot assigned ahmedwaleedmalik Nov 21, 2022

kubermatic-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 21, 2022

kubermatic-bot merged commit 099b8e3 into kubermatic:main Nov 21, 2022

hdurand0710 reviewed Nov 21, 2022

View reviewed changes

		@@ -149,8 +154,18 @@ type SecondaryDisks struct {
		}

		type OSImage struct {

add http source for kubevirt VM Disk Image #1470

add http source for kubevirt VM Disk Image #1470

Conversation

sankalp-r commented Oct 31, 2022 • edited Loading

sankalp-r commented Oct 31, 2022

sankalp-r commented Nov 7, 2022

mfranczy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfranczy Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfranczy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sankalp-r Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sankalp-r commented Nov 10, 2022

mfranczy Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

mfranczy commented Nov 10, 2022

Choose a reason for hiding this comment

sankalp-r commented Nov 16, 2022

sankalp-r commented Nov 16, 2022

sankalp-r commented Nov 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfranczy left a comment

Choose a reason for hiding this comment

kubermatic-bot commented Nov 17, 2022

sankalp-r commented Nov 17, 2022

ahmedwaleedmalik left a comment

Choose a reason for hiding this comment

kubermatic-bot commented Nov 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sankalp-r commented Oct 31, 2022 •

edited

Loading

mfranczy Nov 9, 2022 •

edited

Loading

sankalp-r Nov 9, 2022 •

edited

Loading

mfranczy Nov 10, 2022 •

edited

Loading