Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow specifying download format in vmexport #10400

Merged
merged 3 commits into from
Sep 21, 2023
Merged

Allow specifying download format in vmexport #10400

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bd0d3bf
Allow specifying download format in vmexport
alromeros Sep 7, 2023
6037b06
Add flag to decompress gzip downloads
alromeros Sep 11, 2023
7dfa5ba
Merge format-related flags in vmexport into a single "--format" flag
alromeros Sep 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions pkg/virtctl/memorydump/memorydump.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ const (
storageClassArg = "storage-class"
accessModeArg = "access-mode"
portForwardArg = "port-forward"
rawArg = "raw"
decompressArg = "decompress"
localPortArg = "local-port"

configName = "config"
Expand All @@ -64,6 +66,8 @@ var (
claimName string
createClaim bool
portForward bool
rawImg bool
decompress bool
localPort string
storageClass string
accessMode string
Expand Down Expand Up @@ -118,6 +122,8 @@ func NewMemoryDumpCommand(clientConfig clientcmd.ClientConfig) *cobra.Command {
cmd.Flags().StringVar(&claimName, claimNameArg, "", "pvc name to contain the memory dump")
cmd.Flags().BoolVar(&createClaim, createClaimArg, false, "Create the pvc that will conatin the memory dump")
cmd.Flags().BoolVar(&portForward, portForwardArg, false, "Configure and set port-forward in a random port to download the memory dump")
cmd.Flags().BoolVar(&rawImg, rawArg, false, "Downloads in raw format.")
cmd.Flags().BoolVar(&decompress, decompressArg, false, "Downloads and decompress in the same step.")
cmd.Flags().StringVar(&localPort, localPortArg, "0", "Specify port for port-forward")
cmd.Flags().StringVar(&storageClass, storageClassArg, "", "The storage class for the PVC.")
cmd.Flags().StringVar(&accessMode, accessModeArg, "", "The access mode for the PVC.")
Expand Down Expand Up @@ -335,6 +341,8 @@ func downloadMemoryDump(namespace, vmName string, virtClient kubecli.KubevirtCli
ExportSource: exportSource,
PortForward: portForward,
LocalPort: localPort,
RawImg: rawImg,
Decompress: decompress,
}

if portForward {
Expand Down
38 changes: 38 additions & 0 deletions pkg/virtctl/memorydump/memorydump_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package memorydump_test

import (
"bytes"
"context"
"fmt"
"io"
Expand Down Expand Up @@ -413,6 +414,43 @@ var _ = Describe("MemoryDump", func() {
Expect(cmd.Execute()).To(Succeed())
})

It("should call download memory dump and decompress succesfully", func() {
vmexport.HandleHTTPRequest = func(client kubecli.KubevirtClient, vmexport *exportv1.VirtualMachineExport, downloadUrl string, insecure bool, exportURL string, headers map[string]string) (*http.Response, error) {
resp := http.Response{
StatusCode: http.StatusOK,
Body: io.NopCloser(bytes.NewReader([]byte{
0x1f, 0x8b, 0x08, 0x08, 0xc8, 0x58, 0x13, 0x4a,
0x00, 0x03, 0x68, 0x65, 0x6c, 0x6c, 0x6f, 0x2e,
0x74, 0x78, 0x74, 0x00, 0xcb, 0x48, 0xcd, 0xc9,
0xc9, 0x57, 0x28, 0xcf, 0x2f, 0xca, 0x49, 0xe1,
0x02, 0x00, 0x2d, 0x3b, 0x08, 0xaf, 0x0c, 0x00,
0x00, 0x00,
0x1f, 0x8b, 0x08, 0x08, 0xc8, 0x58, 0x13, 0x4a,
0x00, 0x03, 0x68, 0x65, 0x6c, 0x6c, 0x6f, 0x2e,
0x74, 0x78, 0x74, 0x00, 0xcb, 0x48, 0xcd, 0xc9,
0xc9, 0x57, 0x28, 0xcf, 0x2f, 0xca, 0x49, 0xe1,
0x02, 0x00, 0x2d, 0x3b, 0x08, 0xaf, 0x0c, 0x00,
0x00, 0x00,
})),
}
return &resp, nil
}
memorydump.WaitMemoryDumpComplete = waitForMemoryDumpDefault
vmexport := utils.VMExportSpecPVC(vmexportName, k8smetav1.NamespaceDefault, claimName, secretName)
vmexport.Status = utils.GetVMEStatus([]exportv1.VirtualMachineExportVolume{
{
Name: claimName,
Formats: utils.GetExportVolumeFormat(server.URL, exportv1.KubeVirtGz),
},
}, secretName)
utils.HandleSecretGet(coreClient, secretName)
utils.HandleVMExportCreate(vmExportClient, vmexport)

commandAndArgs := []string{"memory-dump", "download", "testvm", outputFileFlag, "--decompress"}
cmd := clientcmd.NewVirtctlCommand(commandAndArgs...)
Expect(cmd.Execute()).To(Succeed())
})

DescribeTable("should call download memory dump with port-forward", func(commandAndArgs []string) {
vmexport.HandleHTTPRequest = func(client kubecli.KubevirtClient, vmexport *exportv1.VirtualMachineExport, downloadUrl string, insecure bool, exportURL string, headers map[string]string) (*http.Response, error) {
Expect(downloadUrl).To(Equal("https://127.0.0.1:5432"))
Expand Down
56 changes: 50 additions & 6 deletions pkg/virtctl/vmexport/vmexport.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package vmexport

import (
"compress/gzip"
"context"
"crypto/tls"
"crypto/x509"
Expand Down Expand Up @@ -70,6 +71,8 @@ const (
SNAPSHOT_FLAG = "--snapshot"
INSECURE_FLAG = "--insecure"
KEEP_FLAG = "--keep-vme"
RAW_FLAG = "--raw"
DECOMPRESS_FLAG = "--decompress"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused here. Why two options? Don't they ultimately bean the same thing?

What do you think of this, forget about these to flags for a sec. What if we:

  1. always fetch the gzip image
  2. ad "-o" and/or "--output" arg that accepts "gzip" or "raw" (gzip is default to be consistent with current invocation)
  3. if raw is specified, unzip the image while downloading

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I initially wanted to allow both downloading the raw file (--raw) and downloading and decompressing the gzipped one (--decompress). I know they produce the same output but thought it was nice to allow both ways (some users might care about downloading bigger files, others won't). I also prefer boolean flags since they are more immediate and less confusing. But if you prefer to have a single flag to specify the output I can change it, just wanted to share the rationale behind this design.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a case of too many knobs leading to user confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay so I added a --format flag to specify either raw or gzip. --output is already a flag so couldn't use that one.

PVC_FLAG = "--pvc"
TTL_FLAG = "--ttl"
MANIFEST_FLAG = "--manifest"
Expand Down Expand Up @@ -127,7 +130,9 @@ var (
shouldCreate bool
includeSecret bool
exportManifest bool
rawImg bool
portForward bool
decompress bool
localPort string
serviceUrl string
volumeName string
Expand All @@ -153,6 +158,8 @@ type VMExportInfo struct {
KeepVme bool
IncludeSecret bool
ExportManifest bool
RawImg bool
Decompress bool
PortForward bool
LocalPort string
OutputFile string
Expand Down Expand Up @@ -261,6 +268,8 @@ func NewVirtualMachineExportCommand(clientConfig clientcmd.ClientConfig) *cobra.
cmd.Flags().StringVar(&localPort, "local-port", "0", "Defines the specific port to be used in port-forward.")
cmd.Flags().BoolVar(&includeSecret, "include-secret", false, "When used with manifest and set to true include a secret that contains proper headers for CDI to import using the manifest")
cmd.Flags().BoolVar(&exportManifest, "manifest", false, "Instead of downloading a volume, retrieve the VM manifest")
cmd.Flags().BoolVar(&rawImg, "raw", false, "Used to download the raw image. By default, we always attempt to download the compressed one")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to always download the compressed file but have virtctl unzip while downloading/writing to the target

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just added a --decompress flag so we allow both downloading raw and decompressing the gzipped file. wdyt?

cmd.Flags().BoolVar(&decompress, "decompress", false, "Used to unzip the compressed image.")
cmd.SetUsageTemplate(templates.UsageTemplate())

return cmd
Expand Down Expand Up @@ -350,6 +359,8 @@ func (c *command) initVMExportInfo(vmeInfo *VMExportInfo) error {
vmeInfo.ShouldCreate = shouldCreate
vmeInfo.Insecure = insecure
vmeInfo.KeepVme = keepVme
vmeInfo.RawImg = rawImg
vmeInfo.Decompress = decompress
vmeInfo.VolumeName = volumeName
vmeInfo.ServiceURL = serviceUrl
vmeInfo.OutputFormat = manifestOutputFormat
Expand Down Expand Up @@ -551,7 +562,7 @@ func downloadVolume(client kubecli.KubevirtClient, vmexport *exportv1.VirtualMac
}

// Lastly, copy the file to the expected output
if err := copyFileWithProgressBar(vmeInfo.OutputWriter, resp); err != nil {
if err := copyFileWithProgressBar(vmeInfo.OutputWriter, resp, vmeInfo.Decompress); err != nil {
return err
}

Expand Down Expand Up @@ -579,6 +590,7 @@ func GetUrlFromVirtualMachineExport(vmexport *exportv1.VirtualMachineExport, vme
var (
downloadUrl string
err error
format exportv1.VirtualMachineExportVolumeFormat
links *exportv1.VirtualMachineExportLink
)

Expand All @@ -597,21 +609,28 @@ func GetUrlFromVirtualMachineExport(vmexport *exportv1.VirtualMachineExport, vme
for _, exportVolume := range links.Volumes {
// Access the requested volume
if volumeNumber == 1 || exportVolume.Name == vmeInfo.VolumeName {
for _, format := range exportVolume.Formats {
// We always attempt to find and get the compressed file URL, so we only break the loop when one is found
for _, format = range exportVolume.Formats {
if format.Format == exportv1.KubeVirtGz || format.Format == exportv1.ArchiveGz || format.Format == exportv1.KubeVirtRaw {
downloadUrl, err = replaceUrlWithServiceUrl(format.Url, vmeInfo)
if err != nil {
return "", err
}
}
if format.Format == exportv1.KubeVirtGz || format.Format == exportv1.ArchiveGz {
// By default, we always attempt to find and get the compressed file URL, so we only break the loop when one is found.
// If --raw is set to true, we attempt to return the raw URL instead.
if (vmeInfo.RawImg && format.Format == exportv1.KubeVirtRaw) ||
(!vmeInfo.RawImg && (format.Format == exportv1.KubeVirtGz || format.Format == exportv1.ArchiveGz)) {
break
}
}
}
}

// No need to decompress file if format is not gzip
if format.Format == exportv1.KubeVirtRaw {
vmeInfo.Decompress = false
}

if downloadUrl == "" {
return "", fmt.Errorf("unable to get a valid URL from '%s/%s' VirtualMachineExport", vmexport.Namespace, vmexport.Name)
}
Expand Down Expand Up @@ -739,15 +758,28 @@ func getHTTPClient(transport *http.Transport, insecure bool) *http.Client {
}

// copyFileWithProgressBar serves as a wrapper to copy the file with a progress bar
func copyFileWithProgressBar(output io.Writer, resp *http.Response) error {
func copyFileWithProgressBar(output io.Writer, resp *http.Response, decompress bool) error {
var rd io.Reader
barTemplate := fmt.Sprintf(`{{ "Downloading file:" }} {{counters . }} {{ cycle . %s }} {{speed . }}`, progressBarCycle)

// start bar based on our template
bar := pb.ProgressBarTemplate(barTemplate).Start(0)
defer bar.Finish()
rd := bar.NewProxyReader(resp.Body)
barRd := bar.NewProxyReader(resp.Body)
rd = barRd
bar.Start()

if decompress {
// Create a new gzip reader
gzipReader, err := gzip.NewReader(barRd)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional nice to have: Is this code able to create a sparse file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can decompress the file and then rewrite it to be sparse but that sounds out of scope for the PR imo. gzip can't directly decompress it to be sparse, maybe there's some other package that does it. I can take a better look.

if err != nil {
return err
}
defer gzipReader.Close()
rd = gzipReader
fmt.Println("Decompressing image:")
}

_, err := io.Copy(output, rd)
return err
}
Expand Down Expand Up @@ -854,6 +886,12 @@ func handleCreateFlags() error {
if localPort != "0" {
return fmt.Errorf(ErrIncompatibleFlag, LOCAL_PORT_FLAG, CREATE)
}
if rawImg {
return fmt.Errorf(ErrIncompatibleFlag, RAW_FLAG, CREATE)
}
if decompress {
return fmt.Errorf(ErrIncompatibleFlag, DECOMPRESS_FLAG, CREATE)
}
if serviceUrl != "" {
return fmt.Errorf(ErrIncompatibleFlag, SERVICE_URL_FLAG, CREATE)
}
Expand Down Expand Up @@ -885,6 +923,12 @@ func handleDeleteFlags() error {
if localPort != "0" {
return fmt.Errorf(ErrIncompatibleFlag, LOCAL_PORT_FLAG, DELETE)
}
if rawImg {
return fmt.Errorf(ErrIncompatibleFlag, RAW_FLAG, DELETE)
}
if decompress {
return fmt.Errorf(ErrIncompatibleFlag, DECOMPRESS_FLAG, DELETE)
}
if serviceUrl != "" {
return fmt.Errorf(ErrIncompatibleFlag, SERVICE_URL_FLAG, DELETE)
}
Expand Down
96 changes: 92 additions & 4 deletions pkg/virtctl/vmexport/vmexport_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package vmexport_test

import (
"bytes"
"fmt"
"io"
"net/http"
Expand Down Expand Up @@ -358,6 +359,42 @@ var _ = Describe("vmexport", func() {
Expect(err).ToNot(HaveOccurred())
})

It("Succesfully create and download a VirtualMachineExport with decompress flag", func() {
virtctlvmexport.HandleHTTPRequest = func(client kubecli.KubevirtClient, vmexport *exportv1.VirtualMachineExport, downloadUrl string, insecure bool, exportURL string, headers map[string]string) (*http.Response, error) {
resp := http.Response{
StatusCode: http.StatusOK,
Body: io.NopCloser(bytes.NewReader([]byte{
0x1f, 0x8b, 0x08, 0x08, 0xc8, 0x58, 0x13, 0x4a,
0x00, 0x03, 0x68, 0x65, 0x6c, 0x6c, 0x6f, 0x2e,
0x74, 0x78, 0x74, 0x00, 0xcb, 0x48, 0xcd, 0xc9,
0xc9, 0x57, 0x28, 0xcf, 0x2f, 0xca, 0x49, 0xe1,
0x02, 0x00, 0x2d, 0x3b, 0x08, 0xaf, 0x0c, 0x00,
0x00, 0x00,
0x1f, 0x8b, 0x08, 0x08, 0xc8, 0x58, 0x13, 0x4a,
0x00, 0x03, 0x68, 0x65, 0x6c, 0x6c, 0x6f, 0x2e,
0x74, 0x78, 0x74, 0x00, 0xcb, 0x48, 0xcd, 0xc9,
0xc9, 0x57, 0x28, 0xcf, 0x2f, 0xca, 0x49, 0xe1,
0x02, 0x00, 0x2d, 0x3b, 0x08, 0xaf, 0x0c, 0x00,
0x00, 0x00,
})),
}
return &resp, nil
}
vmexport := utils.VMExportSpecPVC(vmexportName, metav1.NamespaceDefault, "test-pvc", secretName)
vmexport.Status = utils.GetVMEStatus([]exportv1.VirtualMachineExportVolume{
{
Name: volumeName,
Formats: utils.GetExportVolumeFormat(server.URL, exportv1.KubeVirtGz),
},
}, secretName)
utils.HandleSecretGet(kubeClient, secretName)
utils.HandleVMExportCreate(vmExportClient, vmexport)

cmd := clientcmd.NewRepeatableVirtctlCommand(commandName, virtctlvmexport.DOWNLOAD, vmexportName, virtctlvmexport.DECOMPRESS_FLAG, setflag(virtctlvmexport.PVC_FLAG, "test-pvc"), setflag(virtctlvmexport.VOLUME_FLAG, volumeName), setflag(virtctlvmexport.OUTPUT_FLAG, "test-pvc"), virtctlvmexport.INSECURE_FLAG)
err := cmd()
Expect(err).ToNot(HaveOccurred())
})

It("Succesfully download a VirtualMachineExport with just 'raw' links", func() {
vmexport := utils.VMExportSpecPVC(vmexportName, metav1.NamespaceDefault, "test-pvc", secretName)
vmexport.Status = utils.GetVMEStatus([]exportv1.VirtualMachineExportVolume{
Expand Down Expand Up @@ -391,6 +428,23 @@ var _ = Describe("vmexport", func() {
Expect(err).ToNot(HaveOccurred())
})

It("VirtualMachineExport download succeeds when --raw is used but only compressed is available", func() {
testInit(http.StatusOK)
vme := utils.VMExportSpecPVC(vmexportName, metav1.NamespaceDefault, "test-pvc", secretName)
vme.Status = utils.GetVMEStatus([]exportv1.VirtualMachineExportVolume{
{
Name: "no-test-volume",
Formats: utils.GetExportVolumeFormat(server.URL, exportv1.KubeVirtGz),
},
}, secretName)
utils.HandleSecretGet(kubeClient, secretName)
utils.HandleVMExportGet(vmExportClient, vme, vmexportName)

cmd := clientcmd.NewRepeatableVirtctlCommand(commandName, virtctlvmexport.DOWNLOAD, vmexportName, virtctlvmexport.RAW_FLAG, setflag(virtctlvmexport.OUTPUT_FLAG, "disk.img"), setflag(virtctlvmexport.VOLUME_FLAG, volumeName))
err := cmd()
Expect(err).ToNot(HaveOccurred())
})

It("VirtualMachineExport download succeeds when there's only one volume and no --volume has been specified", func() {
testInit(http.StatusOK)
vme := utils.VMExportSpecPVC(vmexportName, metav1.NamespaceDefault, "test-pvc", secretName)
Expand Down Expand Up @@ -439,10 +493,13 @@ var _ = Describe("vmexport", func() {

Context("getUrlFromVirtualMachineExport", func() {
// Mocking the minimum viable VMExportInfo struct
vmeinfo := &virtctlvmexport.VMExportInfo{
Name: vmexportName,
VolumeName: volumeName,
}
var vmeinfo *virtctlvmexport.VMExportInfo
BeforeEach(func() {
vmeinfo = &virtctlvmexport.VMExportInfo{
Name: vmexportName,
VolumeName: volumeName,
}
})

It("Should get compressed URL even when there's multiple URLs", func() {
vmExport := utils.VMExportSpecPVC(vmexportName, metav1.NamespaceDefault, "test-pvc", secretName)
Expand Down Expand Up @@ -487,6 +544,37 @@ var _ = Describe("vmexport", func() {
Expect(url).Should(Equal("raw"))
})

It("Should get raw URL if --raw is specified", func() {
vmExport := utils.VMExportSpecPVC(vmexportName, metav1.NamespaceDefault, "test-pvc", secretName)
vmExport.Status = utils.GetVMEStatus([]exportv1.VirtualMachineExportVolume{
{
Name: volumeName,
Formats: []exportv1.VirtualMachineExportVolumeFormat{
{
Format: exportv1.KubeVirtRaw,
Url: "raw",
},
{
Format: exportv1.KubeVirtRaw,
Url: "raw",
},
{
Format: exportv1.KubeVirtGz,
Url: "compressed",
},
{
Format: exportv1.KubeVirtRaw,
Url: "raw",
},
},
},
}, secretName)
vmeinfo.RawImg = true
url, err := virtctlvmexport.GetUrlFromVirtualMachineExport(vmExport, vmeinfo)
Expect(err).ToNot(HaveOccurred())
Expect(url).Should(Equal("raw"))
})

It("Should not get any URL when there's no valid options", func() {
vmExport := utils.VMExportSpecPVC(vmexportName, metav1.NamespaceDefault, "test-pvc", secretName)
vmExport.Status = utils.GetVMEStatus([]exportv1.VirtualMachineExportVolume{
Expand Down