[WIP] bundle: Parallel download and decompression #4504

vyasgun · 2024-12-06T05:33:17Z

Description

This pull request does the following:

Return a reader from the bundle Download function.
Use the reader to stream the bytes to Extract function.

This commit replaces grab client with the net/http client to ensure that the bytes are streamed come in correct order to the Extract func. Currently, only zst decompression is being used in the UncompressWithReader function as it is the primary compression algorithm being used in crc.

The download progress bar has been removed temporarily and will be added back as part of refactoring the code.

Fixes: #4336

Type of change

Bug fix (non-breaking change which fixes an issue)
Feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change
Chore (non-breaking change which doesn't affect codebase;
test, version modification, documentation, etc.)

Proposed changes

Return a reader from the bundle Download function.
Use the reader to stream the bytes to Extract function.

Testing

Contribution Checklist

openshift-ci · 2024-12-06T05:33:22Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2024-12-06T05:33:30Z

[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 adrianriobo 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

vyasgun · 2024-12-06T05:33:31Z

/test all

openshift-ci · 2024-12-06T05:42:04Z

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

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-crc	`6cf717b`	link	true	`/test e2e-crc`
ci/prow/images	`6cf717b`	link	true	`/test images`
ci/prow/security	`6cf717b`	link	false	`/test security`
ci/prow/integration-crc	`6cf717b`	link	true	`/test integration-crc`
ci/prow/e2e-microshift-crc	`6cf717b`	link	true	`/test e2e-microshift-crc`

Full PR test history. Your PR dashboard.

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.

This commit does the following: - Return a reader from the bundle Download function. - Use the reader to stream the bytes to Extract function. This commit replaces grab client with the net/http client to ensure that the bytes are streamed come in correct order to the Extract func. Currently, only zst decompression is being used in the UncompressWithReader function as it is the primary compression algorithm being used in crc.

redbeam

Overall, this is great work and it's functional.

My findings:

cancelling through web api (socket) works
better logging would be nice (currently it's skipping the download part) so that it's clear that the download and uncompression is being done simultaneously
progress bar could show more info about both processes
resuming interrupted download doesn't work - everything starts from the beginning
golangci-lint issues

Suggestions:

add (cli/config) option to disable this functionality (revert back to old behavior)

redbeam · 2024-12-12T19:24:29Z

pkg/download/download.go

 	}
+	client := http.Client{Transport: &http.Transport{}}

 	if ctx == nil {


This check might need to be moved higher, as I see that http.NewRequestWithContext(ctx, "GET", uri, nil) is already called earlier (and if ctx is null, might produce errors).

redbeam · 2024-12-13T20:03:33Z

pkg/crc/machine/bundle/metadata.go

 	}
 	return downloadInfo.Download(ctx, constants.GetDefaultBundlePath(preset), 0664)
 }

-func Download(ctx context.Context, preset crcPreset.Preset, bundleURI string, enableBundleQuayFallback bool) (string, error) {
+func Download(ctx context.Context, preset crcPreset.Preset, bundleURI string, enableBundleQuayFallback bool) (io.Reader, string, error) {
+	var reader io.Reader


I think this line is kind of "hidden" here above the big comment, I suggest moving it down below it.

The variable does not seem to be required in this block, the last line of the function can return nil instead of reader

redbeam · 2024-12-13T20:11:18Z

pkg/crc/image/image.go

@@ -116,43 +116,43 @@ func GetPresetName(imageName string) crcpreset.Preset {
 	return preset
 }

-func PullBundle(ctx context.Context, imageURI string) (string, error) {
+func PullBundle(ctx context.Context, imageURI string) (io.Reader, string, error) {


Would it be possible to add support for pulling from container image repositories as well?

I'm not sure I understand your question, PullBundle pulls from a container image registry.

If PullBundle never returns a bundle, I would not change its signature, and do something like this in Download instead of return image.PullBundle(ctx, bundleURI)

path, err := image.PullBundle(ctx, bundleURI) return nil, path, err

But maybe you have plans to make it return a reader later?

redbeam · 2024-12-13T22:26:28Z

pkg/crc/machine/bundle/repository.go

@@ -198,8 +229,14 @@ func Use(bundleName string) (*CrcBundleInfo, error) {
 	return defaultRepo.Use(bundleName)
 }

-func Extract(ctx context.Context, path string) (*CrcBundleInfo, error) {
-	if err := defaultRepo.Extract(ctx, path); err != nil {
+func Extract(ctx context.Context, reader io.Reader, path string) (*CrcBundleInfo, error) {


We could use just one return statement and one error check in this case:

var err error if reader == nil { err = defaultRepo.Extract(ctx, path) } else { err = defaultRepo.ExtractWithReader(ctx, reader, path) } if err != nil { return nil, err } return defaultRepo.Get(filepath.Base(path))

Add more blank lines as you see fit.

I was about to make the same suggestion

redbeam · 2024-12-13T22:31:40Z

pkg/crc/machine/bundle/repository.go

@@ -124,6 +125,36 @@ func (bundle *CrcBundleInfo) createSymlinkOrCopyPodmanRemote(binDir string) erro
 	return bundle.copyExecutableFromBundle(binDir, PodmanExecutable, constants.PodmanRemoteExecutableName)
 }

+func (repo *Repository) ExtractWithReader(ctx context.Context, reader io.Reader, path string) error {


This function and Extract are very similar, could they be merged in some way?

Extract could probably use os.Open and call into ExtractWithReader

redbeam · 2024-12-13T22:46:09Z

pkg/extract/extract.go

+		return nil, err
+	}
+	return untar(ctx, reader, targetDir, fileFilter, showProgress)
+


nitpick: empty line 47

redbeam · 2024-12-13T22:49:25Z

pkg/extract/extract.go

@@ -86,6 +101,9 @@ func uncompress(ctx context.Context, tarball, targetDir string, fileFilter func(
 	}
 }

+func Untar(ctx context.Context, reader io.Reader, targetDir string, fileFilter func(string) bool, showProgress bool) ([]string, error) {


I suppose this is for some future functionality?

cfergeau · 2024-12-20T16:41:41Z

pkg/crc/machine/bundle/repository.go

@@ -124,6 +125,36 @@ func (bundle *CrcBundleInfo) createSymlinkOrCopyPodmanRemote(binDir string) erro
 	return bundle.copyExecutableFromBundle(binDir, PodmanExecutable, constants.PodmanRemoteExecutableName)
 }

+func (repo *Repository) ExtractWithReader(ctx context.Context, reader io.Reader, path string) error {


Extract could probably use os.Open and call into ExtractWithReader

cfergeau · 2024-12-20T16:47:20Z

cmd/crc-embedder/cmd/embed.go

@@ -163,7 +163,7 @@ func downloadDataFiles(goos string, components []string, destDir string) ([]stri
 		if !shouldDownload(components, componentName) {
 			continue
 		}
-		filename, err := download.Download(context.TODO(), dl.url, destDir, dl.permissions, nil)
+		_, filename, err := download.Download(context.TODO(), dl.url, destDir, dl.permissions, nil)


I think I'd add a download.DownloadFile(…) (string, error) to make it clear when we don't need the reader.

cfergeau · 2024-12-20T16:51:14Z

pkg/crc/image/image.go

@@ -116,43 +116,43 @@ func GetPresetName(imageName string) crcpreset.Preset {
 	return preset
 }

-func PullBundle(ctx context.Context, imageURI string) (string, error) {
+func PullBundle(ctx context.Context, imageURI string) (io.Reader, string, error) {


If PullBundle never returns a bundle, I would not change its signature, and do something like this in Download instead of return image.PullBundle(ctx, bundleURI)

path, err := image.PullBundle(ctx, bundleURI) return nil, path, err

But maybe you have plans to make it return a reader later?

cfergeau · 2024-12-20T16:55:35Z

pkg/crc/machine/bundle/metadata.go

 	}
 	return downloadInfo.Download(ctx, constants.GetDefaultBundlePath(preset), 0664)
 }

-func Download(ctx context.Context, preset crcPreset.Preset, bundleURI string, enableBundleQuayFallback bool) (string, error) {
+func Download(ctx context.Context, preset crcPreset.Preset, bundleURI string, enableBundleQuayFallback bool) (io.Reader, string, error) {
+	var reader io.Reader


The variable does not seem to be required in this block, the last line of the function can return nil instead of reader

cfergeau · 2024-12-20T16:58:54Z

pkg/crc/machine/bundle/repository.go

@@ -198,8 +229,14 @@ func Use(bundleName string) (*CrcBundleInfo, error) {
 	return defaultRepo.Use(bundleName)
 }

-func Extract(ctx context.Context, path string) (*CrcBundleInfo, error) {
-	if err := defaultRepo.Extract(ctx, path); err != nil {
+func Extract(ctx context.Context, reader io.Reader, path string) (*CrcBundleInfo, error) {


I was about to make the same suggestion

cfergeau · 2024-12-20T17:05:54Z

pkg/crc/machine/start.go

 	logging.Infof("Extracting bundle: %s...", bundleName)
-	if _, err := bundle.Extract(ctx, bundlePath); err != nil {
+	if _, err := bundle.Extract(ctx, reader, bundlePath); err != nil {


Having a bundlePath and a reader feels a bit redundant, ideally we could pass one or the other, but I'm not sure it is currently that easy.

openshift-ci bot added the do-not-merge/work-in-progress label Dec 6, 2024

vyasgun requested review from cfergeau, redbeam and praveenkumar December 6, 2024 05:33

vyasgun force-pushed the pr/parallel-decompress branch 8 times, most recently from 7766bc1 to bb0b17c Compare December 7, 2024 17:32

vyasgun force-pushed the pr/parallel-decompress branch from bb0b17c to 3a62d1a Compare December 7, 2024 17:34

redbeam reviewed Dec 13, 2024

View reviewed changes

cfergeau reviewed Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] bundle: Parallel download and decompression #4504

[WIP] bundle: Parallel download and decompression #4504

vyasgun commented Dec 6, 2024 •

edited

Loading

openshift-ci bot commented Dec 6, 2024

openshift-ci bot commented Dec 6, 2024

vyasgun commented Dec 6, 2024

openshift-ci bot commented Dec 6, 2024

redbeam left a comment

redbeam Dec 12, 2024

redbeam Dec 13, 2024

cfergeau Dec 20, 2024

redbeam Dec 13, 2024

cfergeau Dec 20, 2024

cfergeau Dec 20, 2024

redbeam Dec 13, 2024 •

edited

Loading

cfergeau Dec 20, 2024

redbeam Dec 13, 2024

cfergeau Dec 20, 2024

redbeam Dec 13, 2024

redbeam Dec 13, 2024

cfergeau Dec 20, 2024

cfergeau Dec 20, 2024

cfergeau Dec 20, 2024

cfergeau Dec 20, 2024

cfergeau Dec 20, 2024

cfergeau Dec 20, 2024

[WIP] bundle: Parallel download and decompression #4504

Are you sure you want to change the base?

[WIP] bundle: Parallel download and decompression #4504

Conversation

vyasgun commented Dec 6, 2024 • edited Loading

Description

Type of change

Proposed changes

Testing

Contribution Checklist

openshift-ci bot commented Dec 6, 2024

openshift-ci bot commented Dec 6, 2024

vyasgun commented Dec 6, 2024

openshift-ci bot commented Dec 6, 2024

redbeam 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

redbeam Dec 13, 2024 • 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

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

vyasgun commented Dec 6, 2024 •

edited

Loading

redbeam Dec 13, 2024 •

edited

Loading