CNV-52722: Pass through extra VDDK configuration options to importer pod. #3572

mrnold · 2024-12-16T18:25:12Z

What this PR does / why we need it:
This pull request adds a new annotation "cdi.kubevirt.io/storage.pod.vddk.extraargs", referencing a ConfigMap that contains extra parameters to pass directly to the VDDK library. The use case is to allow tuning of asynchronous buffer counts for MTV as requested in CNV-52722. Testing has shown good results for cold migrations with:

VixDiskLib.nfcAio.Session.BufSizeIn64KB=16
VixDiskLib.nfcAio.Session.BufCount=4

These parameters are stored in a file whose path is passed to the VDDK via the nbdkit "config=" option. The file contents come from the referenced ConfigMap, and the ConfigMap is mounted to the importer pod as a volume.

Which issue(s) this PR fixes:
Fixes CNV-52722

Special notes for your reviewer:
As far as I can tell, a ConfigMap volume mount must be in the same namespace as the importer pod. So MTV will need to create or duplicate the ConfigMap to the same namespace as the DataVolume it creates.

Release note:

Allow extra VDDK configuration parameters to be passed to VDDK importer pods.

The VDDK library itself accepts infrequently-used arguments in a configuration file, and some of these arguments have been tested to show a significant transfer speedup in some environments. This adds an annotation that references a ConfigMap holding the contents of this VDDK configuration file, and mounts it to the importer pod. The first file in the mounted directory is passed to the VDDK. Signed-off-by: Matthew Arnold <[email protected]>

Signed-off-by: Matthew Arnold <[email protected]>

kubevirt-bot · 2024-12-16T18:25:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign awels for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

mrnold · 2024-12-16T18:32:06Z

@mnecas Any concerns about the need to copy the ConfigMap to the importer namespace? I wasn't sure if that would make things awkward to use from the forklift side.

coveralls · 2024-12-16T18:41:56Z

coverage: 59.406% (+0.03%) from 59.373%
when pulling aacc61a on mrnold:cnv-52722
into 791bbd6 on kubevirt:main.

mnecas

Generally LGTM, I have added some a note and NP but nothing on my side

mnecas · 2024-12-16T19:48:47Z

pkg/image/nbdkit.go

+	withHidden, err := os.ReadDir(common.VddkArgsDir)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return "", nil


NP: Please add a comment that the user did not specify the vddk additional config

mnecas · 2024-12-16T19:50:17Z

pkg/image/nbdkit.go

@@ -228,6 +236,30 @@ func getVddkPluginPath() NbdkitPlugin {
 	return NbdkitVddkPlugin
 }

+// Extra VDDK configuration options are stored in a ConfigMap mounted to the
+// importer pod. Just look for the first file in the mounted directory, and


Just look for the first file just note we need to be sure to document this so user does not chain the configs to separate configmaps.

kubevirt-bot · 2024-12-16T19:51:58Z

@mnecas: changing LGTM is restricted to collaborators

In response to this:

Generally LGTM, I have added some a note and NP but nothing on my side

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

mnecas · 2024-12-16T19:53:57Z

@mnecas Any concerns about the need to copy the ConfigMap to the importer namespace? I wasn't sure if that would make things awkward to use from the forklift side.

I think it should be okay, but also think we should document/verify.
Verify that the Forklift service account has enough permissions on the local cluster.
And check if the documentation specifies the permissions on a remote cluster.

Signed-off-by: Matthew Arnold <[email protected]>

mrnold · 2024-12-18T02:21:44Z

/retest

akalenyu

Looks good!

akalenyu · 2024-12-18T13:24:52Z

tests/datavolume_test.go

@@ -1254,6 +1276,30 @@ var _ = Describe("[vendor:[email protected]][level:component]DataVolume tests",
 					Message: "Import Complete",
 					Reason:  "Completed",
 				}}),
+			Entry("[test_id:5083]succeed importing VDDK data volume with extra arguments ConfigMap set", dataVolumeTestArguments{


Do you want to assert something in the final step of this test? like checking the config was applied?

For this test I wanted to make sure that the contents of the ConfigMap are present in the file, so the assertion happens in the vddk-test-plugin (the fgets/strcmp). Is there a better way to check the result from the importer? Like can this test read the pod logs or termination message?

oh okay I see

containerized-data-importer/tools/vddk-test/vddk-test-plugin.c

Line 50 in 625a9e9

if (strcmp(extras, "VixDiskLib.nfcAio.Session.BufSizeIn64KB=16") != 0) { // Must match datavolume_test

Yeah I think we can come up with a cleaner way,
You can either read pod logs or make this a part of the termination message struct

containerized-data-importer/pkg/importer/vddk-datasource_amd64.go

Lines 1011 to 1019 in 625a9e9

// GetTerminationMessage returns data to be serialized and used as the termination message of the importer.

func (vs *VDDKDataSource) GetTerminationMessage() *common.TerminationMessage {

return &common.TerminationMessage{

VddkInfo: &common.VddkInfo{

Version: vddkVersion,

Host: vddkHost,

},

}

}

akalenyu · 2024-12-18T13:25:29Z

tools/vddk-test/vddk-test-plugin.c

@@ -32,6 +33,26 @@ int fakevddk_config(const char *key, const char *value) {
    if (strcmp(key, "snapshot") == 0) {
        expected_arg_count = 9; // Expect one for 'snapshot' and one for 'transports'
    }
+    if (strcmp(key, "config") == 0) {


Are there plans to backport this? just making sure I have a maintenance path crafted.

Yes, I asked around and I guess the customer looking for this is currently on 4.15. So we will definitely be asking for another barrage of backports.

Thanks for the heads up. Is there any way to avoid the test plugin change? I guess not. I am just asking since it could spare making new test images for all releases.

I didn't realize this was an issue, the test is definitely not set up to help avoid generating images. I'm not sure how else to do it though, short of hiding the plugin in the importer image or something.

That's fine

akalenyu · 2024-12-18T13:27:28Z

pkg/image/nbdkit.go

+		return "", err
+	}
+	files := []fs.DirEntry{}
+	for _, file := range withHidden { // Ignore hidden files


hmm, how come mounting a config map generates hidden files?

The volume mount sets up the directory like this:

drwxr-sr-x. 2 root 107 4096 Dec 18 15:55 ..2024_12_18_15_55_18.1193095340 lrwxrwxrwx. 1 root 107 32 Dec 18 15:55 ..data -> ..2024_12_18_15_55_18.1193095340 lrwxrwxrwx. 1 root 107 18 Dec 18 15:55 vddk-key -> ..data/vddk-key

I don't really know why, I didn't look into the implementation details. The documented way to use it is to open the file with the name of the key from the ConfigMap, so I was just skipping the entries with leading dots.

I guess it would be nicer to pass down the name of the key somehow, but what I was really trying to do was get the ConfigMap contents passed down in a way that CDI doesn't have to parse any of it.

You could do something like this

containerized-data-importer/pkg/operator/resources/namespaced/controller.go

Lines 342 to 355 in 625a9e9

VolumeSource: corev1.VolumeSource{

ConfigMap: &corev1.ConfigMapVolumeSource{

LocalObjectReference: corev1.LocalObjectReference{

Name: "cdi-uploadserver-signer-bundle",

},

Items: []corev1.KeyToPath{

{

Key: "ca-bundle.crt",

Path: "ca-bundle.crt",

},

},

DefaultMode: &defaultMode,

},

},

This would be nice, but I was trying to not force a specific key name. With KeyToPath I would need to open the ConfigMap and look for the first key. Maybe it makes sense to just require a specific key name and avoid this issue entirely?

Yeah sounds cleaner.. whatever you prefer

akalenyu · 2024-12-18T13:29:16Z

tests/datavolume_test.go

+			if dv.Annotations == nil {
+				dv.Annotations = make(map[string]string)
+			}


could use controller.AddAnnotation() here

Much tidier, thanks. Done.

akalenyu · 2024-12-18T13:31:00Z

doc/datavolumes.md

@@ -349,6 +349,13 @@ spec:
 [Get VDDK ConfigMap example](../manifests/example/vddk-configmap.yaml)
 [Ways to find thumbprint](https://libguestfs.org/nbdkit-vddk-plugin.1.html#THUMBPRINTS)

+#### Extra VDDK Configuration Options
+
+The VDDK library itself looks in a configuration file (such as `/etc/vmware/config`) for extra options to fine tune data transfers. To pass these options through to the VDDK, store the configuration file contents in a ConfigMap and add a `cdi.kubevirt.io/storage.pod.vddk.extraargs` annotation to the DataVolume specification. The ConfigMap will be mounted to the importer pod as a volume, and the first file in the mounted directory will be passed to the VDDK. This means that the ConfigMap must be placed in the same namespace as the DataVolume, and the ConfigMap should only have one file entry.


I guess you considered making this an API on the DataVolume, but, since you need a backport, you prefer the annotation?

Yes, it didn't seem worth changing the CRDs and all the generated stuff for just for this uncommon fine-tuning configuration option. I can certainly change the API if that would be better.

@mhenriks wdyt? since this is backporting I am leaning to the annotation as well, but I am not sure.. usually any annotation becomes an API that we forget about

Issue: The scale and perf team found a way how to improve the transfer speeds. Right now the only way to enable this feature is to set the v2v extra vars. The v2v extra vars pass the configuration to the virt-v2v and virt-v2v-in-place. The v2v extra vars configuration is general and not specific for VDDK. This causes the warm migration which uses the virt-v2v-in-place to fail as it does not use any VDDK parameters. Those parameters should be passed to the CNV CDI instead. Fix: Add a way to easily enable and configure the AIO. This feature is VDDK and provider-specific as it requires to have specific vSphere and VDDK versions. So we can't enable this feature globally nor by default. So this PR adds the configuration to the Provider spec settings and create a configmap with the necessary configuration and either mounts the configmap to the guest conversion pod for cold migration or passes the configmap name to the CDI DV annotation. Example: ``` apiVersion: forklift.konveyor.io/v1beta1 kind: Provider metadata: name: vsphere namespace: forklift spec: settings: sdkEndpoint: vcenter useVddkAioOptimization: 'true' vddkAioBufSize: 16 // optional defaults to 16 vddkAioBufCount: 4 // optional defaults to 4 vddkInitImage: 'quay.io/xiaodwan/vddk:8' type: vsphere ``` Ref: - https://issues.redhat.com/browse/MTV-1804 - kubevirt/containerized-data-importer#3572 - https://docs.redhat.com/en/documentation/migration_toolkit_for_virtualization/2.7/html-single/installing_and_using_the_migration_toolkit_for_virtualization/index#mtv-aio-buffer_mtv Signed-off-by: Martin Necas <[email protected]>

Signed-off-by: Matthew Arnold <[email protected]>

mrnold · 2024-12-18T21:17:43Z

/retest

Issue: The scale and perf team found a way how to improve the transfer speeds. Right now the only way to enable this feature is to set the v2v extra vars. The v2v extra vars pass the configuration to the virt-v2v and virt-v2v-in-place. The v2v extra vars configuration is general and not specific for VDDK. This causes the warm migration which uses the virt-v2v-in-place to fail as it does not use any VDDK parameters. Those parameters should be passed to the CNV CDI instead. Fix: Add a way to easily enable and configure the AIO. This feature is VDDK and provider-specific as it requires to have specific vSphere and VDDK versions. So we can't enable this feature globally nor by default. So this PR adds the configuration to the Provider spec settings and create a configmap with the necessary configuration and either mounts the configmap to the guest conversion pod for cold migration or passes the configmap name to the CDI DV annotation. Example: ``` apiVersion: forklift.konveyor.io/v1beta1 kind: Provider metadata: name: vsphere namespace: forklift spec: settings: sdkEndpoint: vcenter useVddkAioOptimization: 'true' vddkAioBufSize: 16 // optional defaults to 16 vddkAioBufCount: 4 // optional defaults to 4 vddkInitImage: 'quay.io/xiaodwan/vddk:8' type: vsphere ``` Ref: - https://issues.redhat.com/browse/MTV-1804 - kubevirt/containerized-data-importer#3572 - https://docs.redhat.com/en/documentation/migration_toolkit_for_virtualization/2.7/html-single/installing_and_using_the_migration_toolkit_for_virtualization/index#mtv-aio-buffer_mtv Signed-off-by: Martin Necas <[email protected]>

Instead of listing the mounted VDDK arguments directory and filtering out hidden files, just hard-code the expected file name and ConfigMap key. Signed-off-by: Matthew Arnold <[email protected]>

kubevirt-bot · 2024-12-20T21:29:17Z

@mrnold: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cdi-unit-test-s390x	`aacc61a`	link	false	`/test pull-cdi-unit-test-s390x`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

mrnold added 3 commits December 16, 2024 13:09

Add functional test for VDDK args annotation.

eb0f44e

Signed-off-by: Matthew Arnold <[email protected]>

Add unit test for extra VDDK arguments annotation.

a55a611

Signed-off-by: Matthew Arnold <[email protected]>

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Dec 16, 2024

kubevirt-bot added the size/L label Dec 16, 2024

kubevirt-bot requested review from awels and mhenriks December 16, 2024 18:25

mnecas approved these changes Dec 16, 2024

View reviewed changes

Add documentation for extra VDDK arguments.

b260353

Signed-off-by: Matthew Arnold <[email protected]>

akalenyu reviewed Dec 18, 2024

View reviewed changes

mnecas mentioned this pull request Dec 18, 2024

MTV-1804 | Implement VDDK AIO buffer configuration kubev2v/forklift#1280

Open

Simplify new functional test annotation creation.

625a9e9

Signed-off-by: Matthew Arnold <[email protected]>

Look for specific file instead of first file.

aacc61a

Instead of listing the mounted VDDK arguments directory and filtering out hidden files, just hard-code the expected file name and ConfigMap key. Signed-off-by: Matthew Arnold <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CNV-52722: Pass through extra VDDK configuration options to importer pod. #3572

CNV-52722: Pass through extra VDDK configuration options to importer pod. #3572

mrnold commented Dec 16, 2024

kubevirt-bot commented Dec 16, 2024

mrnold commented Dec 16, 2024

coveralls commented Dec 16, 2024 •

edited

Loading

mnecas left a comment

mnecas Dec 16, 2024

mnecas Dec 16, 2024

kubevirt-bot commented Dec 16, 2024

mnecas commented Dec 16, 2024

mrnold commented Dec 18, 2024

akalenyu left a comment

akalenyu Dec 18, 2024

mrnold Dec 18, 2024

akalenyu Dec 19, 2024

akalenyu Dec 18, 2024

mrnold Dec 18, 2024

akalenyu Dec 19, 2024

mrnold Dec 19, 2024

akalenyu Dec 19, 2024

akalenyu Dec 18, 2024

mrnold Dec 18, 2024

akalenyu Dec 19, 2024

mrnold Dec 19, 2024

akalenyu Dec 19, 2024

akalenyu Dec 18, 2024

mrnold Dec 18, 2024

akalenyu Dec 18, 2024

mrnold Dec 18, 2024

akalenyu Dec 18, 2024

mrnold commented Dec 18, 2024

kubevirt-bot commented Dec 20, 2024

	// GetTerminationMessage returns data to be serialized and used as the termination message of the importer.
	func (vs VDDKDataSource) GetTerminationMessage() common.TerminationMessage {
	return &common.TerminationMessage{
	VddkInfo: &common.VddkInfo{
	Version: vddkVersion,
	Host: vddkHost,
	},
	}
	}

	VolumeSource: corev1.VolumeSource{
	ConfigMap: &corev1.ConfigMapVolumeSource{
	LocalObjectReference: corev1.LocalObjectReference{
	Name: "cdi-uploadserver-signer-bundle",
	},
	Items: []corev1.KeyToPath{
	{
	Key: "ca-bundle.crt",
	Path: "ca-bundle.crt",
	},
	},
	DefaultMode: &defaultMode,
	},
	},

CNV-52722: Pass through extra VDDK configuration options to importer pod. #3572

Are you sure you want to change the base?

CNV-52722: Pass through extra VDDK configuration options to importer pod. #3572

Conversation

mrnold commented Dec 16, 2024

kubevirt-bot commented Dec 16, 2024

mrnold commented Dec 16, 2024

coveralls commented Dec 16, 2024 • edited Loading

mnecas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubevirt-bot commented Dec 16, 2024

mnecas commented Dec 16, 2024

mrnold commented Dec 18, 2024

akalenyu 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

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

mrnold commented Dec 18, 2024

kubevirt-bot commented Dec 20, 2024

coveralls commented Dec 16, 2024 •

edited

Loading