-
Notifications
You must be signed in to change notification settings - Fork 269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CNV-52722: Pass through extra VDDK configuration options to importer pod. #3572
base: main
Are you sure you want to change the base?
Changes from 4 commits
a64c2a6
eb0f44e
a55a611
b260353
625a9e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
apiVersion: cdi.kubevirt.io/v1beta1 | ||
kind: DataVolume | ||
metadata: | ||
name: "vddk-dv" | ||
namespace: "cdi" | ||
annotations: | ||
cdi.kubevirt.io/storage.pod.vddk.extraargs: vddk-arguments | ||
spec: | ||
source: | ||
vddk: | ||
backingFile: "[iSCSI_Datastore] vm/vm_1.vmdk" # From 'Hard disk'/'Disk File' in vCenter/ESX VM settings | ||
url: "https://vcenter.corp.com" | ||
uuid: "52260566-b032-36cb-55b1-79bf29e30490" | ||
thumbprint: "20:6C:8A:5D:44:40:B3:79:4B:28:EA:76:13:60:90:6E:49:D9:D9:A3" # SSL fingerprint of vCenter/ESX host | ||
secretRef: "vddk-credentials" | ||
initImageURL: "registry:5000/vddk-init:latest" | ||
storage: | ||
accessModes: | ||
- ReadWriteOnce | ||
resources: | ||
requests: | ||
storage: "32Gi" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
apiVersion: v1 | ||
kind: ConfigMap | ||
metadata: | ||
namespace: cdi | ||
name: vddk-arguments | ||
data: | ||
vddk-config-file: -| | ||
VixDiskLib.nfcAio.Session.BufSizeIn64KB=16 | ||
VixDiskLib.nfcAio.Session.BufCount=4 |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -4,6 +4,7 @@ import ( | |||||||||||||||||||||||||||||
"bufio" | ||||||||||||||||||||||||||||||
"fmt" | ||||||||||||||||||||||||||||||
"io" | ||||||||||||||||||||||||||||||
"io/fs" | ||||||||||||||||||||||||||||||
"os" | ||||||||||||||||||||||||||||||
"os/exec" | ||||||||||||||||||||||||||||||
"path/filepath" | ||||||||||||||||||||||||||||||
|
@@ -168,6 +169,13 @@ func NewNbdkitVddk(nbdkitPidFile, socket string, args NbdKitVddkPluginArgs) (Nbd | |||||||||||||||||||||||||||||
pluginArgs = append(pluginArgs, "-D", "nbdkit.backend.datapath=0") | ||||||||||||||||||||||||||||||
pluginArgs = append(pluginArgs, "-D", "vddk.datapath=0") | ||||||||||||||||||||||||||||||
pluginArgs = append(pluginArgs, "-D", "vddk.stats=1") | ||||||||||||||||||||||||||||||
config, err := getVddkConfig() | ||||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||||
return nil, err | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
if config != "" { | ||||||||||||||||||||||||||||||
pluginArgs = append(pluginArgs, "config="+config) | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
p := getVddkPluginPath() | ||||||||||||||||||||||||||||||
n := &Nbdkit{ | ||||||||||||||||||||||||||||||
NbdPidFile: nbdkitPidFile, | ||||||||||||||||||||||||||||||
|
@@ -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 | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||||||||||||||||
// pass that through nbdkit via the "config=" option. | ||||||||||||||||||||||||||||||
func getVddkConfig() (string, error) { | ||||||||||||||||||||||||||||||
withHidden, err := os.ReadDir(common.VddkArgsDir) | ||||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||||
if os.IsNotExist(err) { // No extra arguments ConfigMap specified, so mount directory does not exist | ||||||||||||||||||||||||||||||
return "", nil | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NP: Please add a comment that the user did not specify the vddk additional config |
||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
return "", err | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
files := []fs.DirEntry{} | ||||||||||||||||||||||||||||||
for _, file := range withHidden { // Ignore hidden files | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, how come mounting a config map generates hidden files? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The volume mount sets up the directory like this:
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could do something like this containerized-data-importer/pkg/operator/resources/namespaced/controller.go Lines 342 to 355 in 625a9e9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah sounds cleaner.. whatever you prefer |
||||||||||||||||||||||||||||||
if !strings.HasPrefix(file.Name(), ".") { | ||||||||||||||||||||||||||||||
files = append(files, file) | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
if len(files) < 1 { | ||||||||||||||||||||||||||||||
return "", fmt.Errorf("no VDDK configuration files found under %s", common.VddkArgsDir) | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
path := filepath.Join(common.VddkArgsDir, files[0].Name()) | ||||||||||||||||||||||||||||||
return path, nil | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
func (n *Nbdkit) getSourceArg(s string) string { | ||||||||||||||||||||||||||||||
var source string | ||||||||||||||||||||||||||||||
switch n.plugin { | ||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -1184,6 +1184,28 @@ var _ = Describe("[vendor:[email protected]][level:component]DataVolume tests", | |||||||||||||||||||||
return dv | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
createVddkDataVolumeWithExtraArgs := func(dataVolumeName, size, url string) *cdiv1.DataVolume { | ||||||||||||||||||||||
dv := createVddkDataVolumeWithInitImageURL(dataVolumeName, size, url) | ||||||||||||||||||||||
extraArguments := v1.ConfigMap{ | ||||||||||||||||||||||
ObjectMeta: metav1.ObjectMeta{ | ||||||||||||||||||||||
Namespace: f.Namespace.Name, | ||||||||||||||||||||||
Name: "vddk-extra-args-map", | ||||||||||||||||||||||
}, | ||||||||||||||||||||||
Data: map[string]string{ // Must match vddk-test-plugin | ||||||||||||||||||||||
"vddk-config-file": "VixDiskLib.nfcAio.Session.BufSizeIn64KB=16", | ||||||||||||||||||||||
}, | ||||||||||||||||||||||
} | ||||||||||||||||||||||
_, err := f.K8sClient.CoreV1().ConfigMaps(f.Namespace.Name).Create(context.TODO(), &extraArguments, metav1.CreateOptions{}) | ||||||||||||||||||||||
if !k8serrors.IsAlreadyExists(err) { | ||||||||||||||||||||||
Expect(err).ToNot(HaveOccurred()) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
if dv.Annotations == nil { | ||||||||||||||||||||||
dv.Annotations = make(map[string]string) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could use controller.AddAnnotation() here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Much tidier, thanks. Done. |
||||||||||||||||||||||
dv.Annotations[controller.AnnVddkExtraArgs] = extraArguments.Name | ||||||||||||||||||||||
return dv | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
// Similar to previous table, but with additional cleanup steps to save and restore VDDK image config map | ||||||||||||||||||||||
DescribeTable("should", Serial, func(args dataVolumeTestArguments) { | ||||||||||||||||||||||
_, err := utils.CopyConfigMap(f.K8sClient, f.CdiInstallNs, common.VddkConfigMap, f.CdiInstallNs, savedVddkConfigMap, "") | ||||||||||||||||||||||
|
@@ -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{ | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you want to assert something in the final step of this test? like checking the config was applied? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh okay I see
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
|
||||||||||||||||||||||
name: "dv-import-vddk", | ||||||||||||||||||||||
size: "1Gi", | ||||||||||||||||||||||
url: vcenterURL, | ||||||||||||||||||||||
dvFunc: createVddkDataVolumeWithExtraArgs, | ||||||||||||||||||||||
eventReason: dvc.ImportSucceeded, | ||||||||||||||||||||||
phase: cdiv1.Succeeded, | ||||||||||||||||||||||
checkPermissions: false, | ||||||||||||||||||||||
readyCondition: &cdiv1.DataVolumeCondition{ | ||||||||||||||||||||||
Type: cdiv1.DataVolumeReady, | ||||||||||||||||||||||
Status: v1.ConditionTrue, | ||||||||||||||||||||||
}, | ||||||||||||||||||||||
boundCondition: &cdiv1.DataVolumeCondition{ | ||||||||||||||||||||||
Type: cdiv1.DataVolumeBound, | ||||||||||||||||||||||
Status: v1.ConditionTrue, | ||||||||||||||||||||||
Message: "PVC dv-import-vddk Bound", | ||||||||||||||||||||||
Reason: "Bound", | ||||||||||||||||||||||
}, | ||||||||||||||||||||||
runningCondition: &cdiv1.DataVolumeCondition{ | ||||||||||||||||||||||
Type: cdiv1.DataVolumeRunning, | ||||||||||||||||||||||
Status: v1.ConditionFalse, | ||||||||||||||||||||||
Message: "Import Complete", | ||||||||||||||||||||||
Reason: "Completed", | ||||||||||||||||||||||
}}), | ||||||||||||||||||||||
) | ||||||||||||||||||||||
|
||||||||||||||||||||||
// similar to other tables but with check of quota | ||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
#define NBDKIT_API_VERSION 2 | ||
#include <nbdkit-plugin.h> | ||
#include <fcntl.h> | ||
#include <stdio.h> | ||
#include <string.h> | ||
#include <sys/stat.h> | ||
#include <sys/types.h> | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there plans to backport this? just making sure I have a maintenance path crafted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's fine |
||
expected_arg_count = 8; | ||
nbdkit_debug("Extra config option set to: %s\n", value); | ||
|
||
FILE *f = fopen(value, "r"); | ||
if (f == NULL) { | ||
nbdkit_error("Failed to open VDDK extra configuration file %s!\n", value); | ||
return -1; | ||
} | ||
char extras[50]; | ||
if (fgets(extras, 50, f) == NULL) { // Expect only one line of test data | ||
nbdkit_error("Failed to read VDDK extra configuration file %s! Error was: %s", value, strerror(errno)); | ||
return -1; | ||
} | ||
if (strcmp(extras, "VixDiskLib.nfcAio.Session.BufSizeIn64KB=16") != 0) { // Must match datavolume_test | ||
nbdkit_error("Unexpected content in VDDK extra configuration file %s: %s\n", value, extras); | ||
return -1; | ||
} | ||
fclose(f); | ||
} | ||
return 0; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you considered making this an API on the DataVolume, but, since you need a backport, you prefer the annotation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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