Skip to content

Commit

Permalink
[release-1.57] Backport 3385 VDDK: pass snapshot ID through to nbdkit. (
Browse files Browse the repository at this point in the history 
kubevirt#3501)

* nbdkit datapath debug (kubevirt#3361)

* pkg/image/nbdkit: Enable controlpath debugging

Disabling datapath debugging reduces noise, but controlpath debugging
is actually useful, not very noisy, and usually you would want it to
be enabled.

The difference is explained here:

https://libguestfs.org/nbdkit.1.html#SERVER-DEBUG-FLAGS

Virt-v2v disables datapath debugging but keeps controlpath debugging
enabled.

Signed-off-by: Richard W.M. Jones <[email protected]>

* pkg/image/nbdkit: Enable recommended nbdkit VDDK debugging options

VDDK datapath debugging logs every VDDK read and write which is noisy
and unnecessary.

Enabling VDDK stats gives useful information about how long each VDDK
call took, for virtually no overhead.  This information is printed to
stderr when nbdkit closes the connection.

This matches the upstream recommendations here, and also the flags
used by virt-v2v:

https://libguestfs.org/nbdkit-vddk-plugin.1.html#Troubleshooting-performance-problems

Signed-off-by: Richard W.M. Jones <[email protected]>

---------

Signed-off-by: Richard W.M. Jones <[email protected]>

* nbdkit: Use password=+filename to send passwords securely (kubevirt#3363)

Passing passwords on the command line is not very secure since a user
on the same machine can see them using 'ps' etc.  nbdkit lets you pass
them via several secure mechanisms - for this I used the
password=+filename method.

Signed-off-by: Richard W.M. Jones <[email protected]>

* VDDK: pass snapshot ID through to nbdkit. (kubevirt#3385)

* VDDK: pass snapshot ID through to nbdkit.

Opening VMware snapshots can sometimes fail with "Error 13 (You do not
have access rights to this file)" if the snapshot MOref is not provided.
This specifically seems to affect Windows VMs with VMware guest tools
installed.

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

* SonarCloud: Move VDDK plugin arguments to struct.

Adding 'snapshot' exceeded the seven-parameter limit.

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

* Change vddk-test expected snapshot argument count.

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

* Remove "san" from VDDK transports list.

Some versions of the VDDK crash when determining system support for its
"san" transport mode, if there are no SCSI disks present. This transport
mode is active by default, and is not used by CDI, so avoid the crash by
setting the list of allowable transports to not include "san".

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

---------

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

---------

Signed-off-by: Richard W.M. Jones <[email protected]>
Signed-off-by: Matthew Arnold <[email protected]>
Co-authored-by: rwmjones <[email protected]>
  • Loading branch information
@mrnold @rwmjones
mrnold and rwmjones authored Nov 5, 2024
1 parent 32413b2 commit 043fe5d
Show file tree
Hide file tree
Showing 5 changed files with 117 additions and 31 deletions.
81 changes: 60 additions & 21 deletions pkg/image/nbdkit.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ func NewNbdkit(plugin NbdkitPlugin, nbdkitPidFile string) *Nbdkit {
}

// NewNbdkitCurl creates a new Nbdkit instance with the curl plugin
func NewNbdkitCurl(nbdkitPidFile, user, password, certDir, socket string, extraHeaders, secretExtraHeaders []string) NbdkitOperation {
func NewNbdkitCurl(nbdkitPidFile, user, password, certDir, socket string, extraHeaders, secretExtraHeaders []string) (NbdkitOperation, error) {
var pluginArgs []string
var redactArgs []string
args := []string{"-r"}
Expand All @@ -93,7 +93,11 @@ func NewNbdkitCurl(nbdkitPidFile, user, password, certDir, socket string, extraH
pluginArgs = append(pluginArgs, "user="+user)
}
if password != "" {
pluginArgs = append(pluginArgs, "password="+password)
passwordfile, err := writePasswordFile(password)
if err != nil {
return nil, err
}
pluginArgs = append(pluginArgs, "password=+"+passwordfile)
}
if certDir != "" {
pluginArgs = append(pluginArgs, fmt.Sprintf("cainfo=%s/%s", certDir, "tls.crt"))
Expand All @@ -119,33 +123,51 @@ func NewNbdkitCurl(nbdkitPidFile, user, password, certDir, socket string, extraH
n.AddFilter(NbdkitReadAheadFilter)
// Should be last filter
n.AddFilter(NbdkitRetryFilter)
return n
return n, nil
}

// NewNbdkitVddk creates a new Nbdkit instance with the vddk plugin
func NewNbdkitVddk(nbdkitPidFile, socket, server, username, password, thumbprint, moref string) (NbdkitOperation, error) {
// Keep these in a struct to keep NewNbdkitVddk from going over the argument limit
type NbdKitVddkPluginArgs struct {
Server string
Username string
Password string
Thumbprint string
Moref string
Snapshot string
}

// NewNbdkitVddk creates a new Nbdkit instance with the vddk plugin
func NewNbdkitVddk(nbdkitPidFile, socket string, args NbdKitVddkPluginArgs) (NbdkitOperation, error) {
pluginArgs := []string{
"libdir=" + nbdVddkLibraryPath,
}
if server != "" {
pluginArgs = append(pluginArgs, "server="+server)
if args.Server != "" {
pluginArgs = append(pluginArgs, "server="+args.Server)
}
if username != "" {
pluginArgs = append(pluginArgs, "user="+username)
if args.Username != "" {
pluginArgs = append(pluginArgs, "user="+args.Username)
}
if password != "" {
pluginArgs = append(pluginArgs, "password="+password)
if args.Password != "" {
passwordfile, err := writePasswordFile(args.Password)
if err != nil {
return nil, err
}
pluginArgs = append(pluginArgs, "password=+"+passwordfile)
}
if thumbprint != "" {
pluginArgs = append(pluginArgs, "thumbprint="+thumbprint)
if args.Thumbprint != "" {
pluginArgs = append(pluginArgs, "thumbprint="+args.Thumbprint)
}
if moref != "" {
pluginArgs = append(pluginArgs, "vm=moref="+moref)
if args.Moref != "" {
pluginArgs = append(pluginArgs, "vm=moref="+args.Moref)
}
if args.Snapshot != "" {
pluginArgs = append(pluginArgs, "snapshot="+args.Snapshot)
pluginArgs = append(pluginArgs, "transports=file:nbdssl:nbd")
}
pluginArgs = append(pluginArgs, "--verbose")
pluginArgs = append(pluginArgs, "-D", "nbdkit.backend.controlpath=0")
pluginArgs = append(pluginArgs, "-D", "nbdkit.backend.datapath=0")
pluginArgs = append(pluginArgs, "-D", "vddk.datapath=0")
pluginArgs = append(pluginArgs, "-D", "vddk.stats=1")
p := getVddkPluginPath()
n := &Nbdkit{
NbdPidFile: nbdkitPidFile,
Expand All @@ -162,6 +184,25 @@ func NewNbdkitVddk(nbdkitPidFile, socket, server, username, password, thumbprint
return n, nil
}

func writePasswordFile(password string) (string, error) {
f, err := os.CreateTemp("", "password")
if err != nil {
return "", err
}
defer f.Close()
// This would be nice but we don't want to delete it until
// program exit.
// defer os.Remove(f.Name())

if _, err := f.Write([]byte(password)); err != nil {
return "", err
}
if err := f.Close(); err != nil {
return "", err
}
return f.Name(), nil
}

// AddEnvVariable adds an environmental variable to the nbdkit command
func (n *Nbdkit) AddEnvVariable(v string) {
env := os.Environ()
Expand Down Expand Up @@ -234,9 +275,7 @@ func (n *Nbdkit) StartNbdkit(source string) error {

quotedArgs := make([]string, len(argsNbdkit))
for index, value := range argsNbdkit {
if strings.HasPrefix(value, "password=") {
quotedArgs[index] = "'password=*****'"
} else if isRedacted(value) {
if isRedacted(value) {
if strings.HasPrefix(value, "header=") {
quotedArgs[index] = "'header=/secret redacted/'"
} else {
Expand Down Expand Up @@ -400,8 +439,8 @@ func (n *Nbdkit) validatePlugin() error {
type mockNbdkit struct{}

// NewMockNbdkitCurl creates a mock nbdkit curl plugin for testing
func NewMockNbdkitCurl(nbdkitPidFile, user, password, certDir, socket string, extraHeaders, secretExtraHeaders []string) NbdkitOperation {
return &mockNbdkit{}
func NewMockNbdkitCurl(nbdkitPidFile, user, password, certDir, socket string, extraHeaders, secretExtraHeaders []string) (NbdkitOperation, error) {
return &mockNbdkit{}, nil
}

func (m *mockNbdkit) StartNbdkit(source string) error {
Expand Down
6 changes: 5 additions & 1 deletion pkg/importer/http-datasource.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,11 @@ func NewHTTPDataSource(endpoint, accessKey, secKey, certDir string, contentType
brokenForQemuImg: brokenForQemuImg,
contentLength: contentLength,
}
httpSource.n = createNbdkitCurl(nbdkitPid, accessKey, secKey, certDir, nbdkitSocket, extraHeaders, secretExtraHeaders)
httpSource.n, err = createNbdkitCurl(nbdkitPid, accessKey, secKey, certDir, nbdkitSocket, extraHeaders, secretExtraHeaders)
if err != nil {
cancel()
return nil, err
}
// We know this is a counting reader, so no need to check.
countingReader := httpReader.(*util.CountingReader)
go httpSource.pollProgress(countingReader, 10*time.Minute, time.Second)
Expand Down
14 changes: 11 additions & 3 deletions pkg/importer/vddk-datasource_amd64.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,16 @@ type NbdKitLogWatcherVddk struct {
}

// createNbdKitWrapper starts nbdkit and returns a process handle for further management
func createNbdKitWrapper(vmware *VMwareClient, diskFileName string) (*NbdKitWrapper, error) {
n, err := image.NewNbdkitVddk(nbdPidFile, nbdUnixSocket, vmware.url.Host, vmware.username, vmware.password, vmware.thumbprint, vmware.moref)
func createNbdKitWrapper(vmware *VMwareClient, diskFileName, snapshot string) (*NbdKitWrapper, error) {
args := image.NbdKitVddkPluginArgs{
Server: vmware.url.Host,
Username: vmware.username,
Password: vmware.password,
Thumbprint: vmware.thumbprint,
Moref: vmware.moref,
Snapshot: snapshot,
}
n, err := image.NewNbdkitVddk(nbdPidFile, nbdUnixSocket, args)
if err != nil {
klog.Errorf("Error validating nbdkit plugins: %v", err)
return nil, err
Expand Down Expand Up @@ -887,7 +895,7 @@ func createVddkDataSource(endpoint string, accessKey string, secKey string, thum
}
klog.Infof("Set disk file name from current snapshot: %s", diskFileName)
}
nbdkit, err := newNbdKitWrapper(vmware, diskFileName)
nbdkit, err := newNbdKitWrapper(vmware, diskFileName, currentCheckpoint)
if err != nil {
klog.Errorf("Unable to start nbdkit: %v", err)
return nil, err
Expand Down
38 changes: 35 additions & 3 deletions pkg/importer/vddk-datasource_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -276,9 +276,9 @@ var _ = Describe("VDDK data source", func() {
var returnedDiskName string

newVddkDataSource = createVddkDataSource
newNbdKitWrapper = func(vmware *VMwareClient, fileName string) (*NbdKitWrapper, error) {
newNbdKitWrapper = func(vmware *VMwareClient, fileName, snapshot string) (*NbdKitWrapper, error) {
returnedDiskName = fileName
return createMockNbdKitWrapper(vmware, fileName)
return createMockNbdKitWrapper(vmware, fileName, snapshot)
}
currentVMwareFunctions.Properties = func(ctx context.Context, ref types.ManagedObjectReference, property []string, result interface{}) error {
switch out := result.(type) {
Expand Down Expand Up @@ -312,6 +312,38 @@ var _ = Describe("VDDK data source", func() {
Entry("should fail if backing file is not found in snapshot tree", "[teststore] testvm/testfile.vmdk", "wrong disk 1.vmdk", "wrong disk 2.vmdk", "wrong disk 1.vmdk", false),
)

It("should pass snapshot reference through to nbdkit", func() {
expectedSnapshot := "snapshot-10"

var receivedSnapshotRef string
newVddkDataSource = createVddkDataSource
newNbdKitWrapper = func(vmware *VMwareClient, fileName, snapshot string) (*NbdKitWrapper, error) {
receivedSnapshotRef = snapshot
return createMockNbdKitWrapper(vmware, fileName, snapshot)
}

currentVMwareFunctions.Properties = func(ctx context.Context, ref types.ManagedObjectReference, property []string, result interface{}) error {
switch out := result.(type) {
case *mo.VirtualMachine:
if property[0] == "config.hardware.device" {
out.Config = createVirtualDiskConfig("disk1", 12345)
} else if property[0] == "snapshot" {
out.Snapshot = createSnapshots(expectedSnapshot, "snapshot-11")
}
case *mo.VirtualMachineSnapshot:
out.Config = *createVirtualDiskConfig("snapshot-disk", 123456)
disk := out.Config.Hardware.Device[0].(*types.VirtualDisk)
parent := disk.Backing.(*types.VirtualDiskFlatVer1BackingInfo).Parent
parent.FileName = "snapshot-root"
}
return nil
}

_, err := NewVDDKDataSource("http://vcenter.test", "user", "pass", "aa:bb:cc:dd", "1-2-3-4", "disk1", expectedSnapshot, "", "", v1.PersistentVolumeFilesystem)
Expect(err).ToNot(HaveOccurred())
Expect(receivedSnapshotRef).To(Equal(expectedSnapshot))
})

It("should find two snapshots and get a list of changed blocks", func() {
newVddkDataSource = createVddkDataSource
diskName := "testdisk.vmdk"
Expand Down Expand Up @@ -629,7 +661,7 @@ func createMockVMwareClient(endpoint string, accessKey string, secKey string, th
}, nil
}

func createMockNbdKitWrapper(vmware *VMwareClient, diskFileName string) (*NbdKitWrapper, error) {
func createMockNbdKitWrapper(vmware *VMwareClient, diskFileName, snapshot string) (*NbdKitWrapper, error) {
u, _ := url.Parse("http://vcenter.test")
return &NbdKitWrapper{
n: &image.Nbdkit{},
Expand Down
9 changes: 6 additions & 3 deletions tools/vddk-test/vddk-test-plugin.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,24 +20,27 @@

#define THREAD_MODEL NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS

#define EXPECTED_ARG_COUNT 7
int arg_count = 0;
int expected_arg_count = 7;

void fakevddk_close(void *handle) {
close(*((int *) handle));
}

int fakevddk_config(const char *key, const char *value) {
arg_count++;
if (strcmp(key, "snapshot") == 0) {
expected_arg_count = 9; // Expect one for 'snapshot' and one for 'transports'
}
return 0;
}

int fakevddk_config_complete(void) {
nbdkit_debug("VMware VixDiskLib (1.2.3) Release build-12345");
if (arg_count == EXPECTED_ARG_COUNT) {
if (arg_count == expected_arg_count) {
return 0;
} else {
nbdkit_error("Expected %d arguments to fake VDDK test plugin, but got %d!\n", EXPECTED_ARG_COUNT, arg_count);
nbdkit_error("Expected %d arguments to fake VDDK test plugin, but got %d!\n", expected_arg_count, arg_count);
return -1;
}
}
Expand Down

0 comments on commit 043fe5d

Please sign in to comment.