Streaming qcow2->raw conversion and resource limits on qemu-img subprocess #317

mhenriks · 2018-08-06T19:17:51Z

Two goals here:

Support streaming qcow2 to raw conversion. Don't want to have to create temporary file. Currently only for http or https source images. But maybe we want to avoid the extra copy for non gzipped/tarred as well?

Put resource limits on qemu-img. Basically, it is possible to craft a qcow2 file in such a way that it'll DOS your system. Initial checkin uses prlimit() to set the same cpu/memory limits used in Nova. Subsequent update will include call to qemu-img info to get details on source image to detect bad images before attempting conversion.
See this thread for more details about streaming conversion and DOS prevention: https://groups.google.com/forum/#!msg/kubevirt-dev/4h7R8fPq0eU/fyL2IaiSBAAJ

streaming qcow2 to raw conversion
cpu/memory limits for qemu-img
pre-validate qcow2 with qemu-img info
tests

Fixes #254

jeffvance · 2018-08-06T22:30:26Z

should we also use fedors28 in hack/build/docker/builder/Dockerfile?

jeffvance · 2018-08-06T22:42:25Z

pkg/image/qemu.go

 )

+// ConvertQcow2ToRaw converts a local qcou2 image to raw format


jeffvance · 2018-08-06T22:52:09Z

pkg/image/qemu.go

+	return nil
+}
+
+// ConvertQcow2ToRawStream converts an http accessible qcow2 image to raf format without locally caching the qcow2 image


nit: typo: assume "raw"?

jeffvance · 2018-08-06T22:57:11Z

pkg/importer/dataStream.go

@@ -467,8 +483,18 @@ func closeReaders(readers []Reader) (rtnerr error) {
 	return rtnerr
 }

+func (d dataStream) isHTTPQcow2() bool {
+	// assuming len(d.Readers) == 3 is [http, mr, mr]
+	// should prob get rid of the redundant MultiReader


agree. Where is the extra mr coming from?

the loop in constructReaders() always adds a MultiReader to the stack, when matchHeader() returns a match. This is a problem in the qcow2 case because unlike the other possible matches, no qcow2 reader is added to the stack.

I think there is a pretty straightforward fix for this.

Ok, my bad and thanks for finding this. If you want to fix this in a separate pr let me know asap, else I'll fix it. Thanks.

@jeffvance I spent a bit of time looking into this earlier. Turns out a fix is not as simple as I thought. I think a separate PR would be appropriate.

I just added pr #339. Thanks for mentioning this as it turned out to be a bug in constructReaders and the unit test too.

jeffvance · 2018-08-06T22:59:26Z

pkg/importer/dataStream.go

+func (d dataStream) isHTTPQcow2() bool {
+	// assuming len(d.Readers) == 3 is [http, mr, mr]
+	// should prob get rid of the redundant MultiReader
+	return (d.Url.Scheme == "http" || d.Url.Scheme == "https") && d.qemu && len(d.Readers) == 3


== 3? vs. >=3 (or 2 if mr fix). It looks like we only invoke streaming if the endpoint has not been compressed and/or archived, correct? I thought http(s) supports some form of inline decompression. If this is true, is true, can we use this so that additional endpoint formats based on qemu can be imported via qemu streaming?

Yeah, qemu-img can't work directly on compressed/tarred files.

I'm not aware of any http features that can help us here. An http client can request that the server compress data for transmission via the Accept-Encoding header. But I'm not aware of any protocol support for decompressing a request for a gzipped file.

Also, qemu-img convert doesn't support reading from stdin so we can't pipe data into it from a qzip reader. If the file is compressed/tarred, I think we have to copy it locally,

jeffvance · 2018-08-06T23:08:59Z

pkg/importer/dataStream.go

 // Copy endpoint to dest based on passed-in reader.
 func (d dataStream) copy(dest string) error {
+	if d.isHTTPQcow2() {
+		glog.V(Vuser).Infoln("Doing streaming qcow2 to raw conversion")


perhaps logging level here should be Vadmin? Just thinking that the end user doesn't really care if we use qemu streaming, but the admin might due to SLA or perf guarantees.

Makes sense

jeffvance · 2018-08-08T01:33:09Z

pkg/image/qemu.go

 	return nil
 }

-// ConvertQcow2ToRawStream converts an http accessible qcow2 image to raf format without locally caching the qcow2 image
+// ConvertQcow2ToRawStream converts an http accessible qcow2 image to raw format without locally caching the qcow2 image


what do you think of having a single ConvertQcow2ToRaw func with an "optional" json arg? This fits the DRY principle...

jeffvance · 2018-08-08T01:36:27Z

pkg/image/qemu.go

+		BackingFile string `json:"backing-filename"`
+	}
+
+	output, err := execWithLimits("qemu-img", "info", "--output=json", image)


I do not know how qemu-img does reads on qcow2 files. But, does this func cause the entire (or a lot of?) the qcow2 file to be read?

Pretty sure it doesn't read the whole file. Would be surprised if it reads more than the header.

right, it's just reading the header.

jeffvance · 2018-08-08T01:37:04Z

pkg/image/qemu.go

+
+func validate(image, format string) error {
+	type imageInfo struct {
+		Format      string `json:"format"`


do these fields need to be exported? Maybe for unmarshaliing? (but the struct type is not exported)?

Since the scope of that struct is limited to the function I didn't think much about public/private. I guess I can make members private.

Get this warning if I make private: struct field format has json tag but is not exported so will keep exported.

hmmm, thanks for checking.

jeffvance · 2018-08-08T01:39:40Z

pkg/image/qemu.go

+	}
+
+	if info.Format != format {
+		return errors.Wrapf(err, "Invalid format %s for image %s", info.Format, image)


but err is nil here so no need to wrap.

good catch!

jeffvance · 2018-08-08T01:40:00Z

pkg/image/qemu.go

+	}
+
+	if len(info.BackingFile) > 0 {
+		return errors.Wrapf(err, "Image %s is invalid because it has backing file %s", image, info.BackingFile)


ditto re nil err

mhenriks · 2018-08-09T13:29:02Z

pkg/image/prlimit_test.go

+
+const killedByTestError = "Had to kill process"
+
+func TestLimits(t *testing.T) {


Wondering if these tests are really necessary and if so should they be functional tests?

I am not sure we've ever found a real image in the wild that can cause problems. Maybe we can run using a fake qemu binary that spinloops or something?

mhenriks · 2018-08-09T13:29:32Z

pkg/image/qemu.go

 	return nil
 }
+
+// ConvertQcow2ToRawStream converts an http accessible qcow2 image to raw format without locally caching the qcow2 image
+func ConvertQcow2ToRawStream(url *url.URL, dest string) error {


Should this be a functional test?

I'd say so. Will depend on @copejon 's server container in the environment.

@aglitke this test actually starts an embedded http server that serves up image files. but yeah may be redundant in functional test environment

jeffvance · 2018-08-09T19:24:10Z

test/scripts/memory-consumer.sh

+
+arr=()
+for ((i=1; i<=$1; i++)); do
+    random="$(head -c 4096 /dev/urandom | base64)"


why pipe to base64 from the perspective of consuming memory?

bash didn't like certain characters (definitely nulls and maybe other stuff) so just base64 encoded everything.

aglitke · 2018-09-06T14:53:26Z

@mhenriks Can you rebase this? Do the existing functional tests cover this feature satisfactorily?

…s on qemu-img

aglitke · 2018-09-07T13:37:20Z

@mhenriks Can we remove the WIP label?

mhenriks · 2018-09-07T14:13:07Z

@aglitke I honestly have no idea how to remove the WIP label! Tried to do it awhile ago. Maybe because I still have restricted permissions to this repo?

aglitke · 2018-09-07T15:20:20Z

@mhenriks Actually the PROW integration will really help with this.

awels · 2018-09-07T20:20:59Z

lgtm

jeffvance added the WIP Work in Progress label Aug 6, 2018

jeffvance reviewed Aug 6, 2018

View reviewed changes

pkg/image/qemu.go Outdated

)

// ConvertQcow2ToRaw converts a local qcou2 image to raw format

Copy link

Contributor

jeffvance Aug 6, 2018

Choose a reason for hiding this comment

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

nit: typo

jeffvance reviewed Aug 6, 2018

View reviewed changes

jeffvance added the no-merge-til-e2e-testing-done label Aug 7, 2018

mhenriks force-pushed the streaming-qcow branch from 866dd05 to 3d765d6 Compare August 7, 2018 18:39

jeffvance reviewed Aug 8, 2018

View reviewed changes

mhenriks force-pushed the streaming-qcow branch from 20ffd3e to a4e7105 Compare August 9, 2018 13:27

mhenriks changed the title ~~[WIP] Streaming qcow2->raw conversion and resource limits on qemu-img subprocess~~ Streaming qcow2->raw conversion and resource limits on qemu-img subprocess Aug 9, 2018

mhenriks commented Aug 9, 2018

View reviewed changes

mhenriks force-pushed the streaming-qcow branch 7 times, most recently from 2a9a74e to 422a42a Compare August 9, 2018 19:17

jeffvance reviewed Aug 9, 2018

View reviewed changes

mhenriks force-pushed the streaming-qcow branch 4 times, most recently from ec230db to 6246025 Compare August 9, 2018 20:31

mhenriks force-pushed the streaming-qcow branch 3 times, most recently from 0912a2a to add4373 Compare August 23, 2018 15:19

mhenriks added 13 commits September 6, 2018 20:15

do streaming qcow2->raw conversion if we can. also put resource limit…

a35d878

…s on qemu-img

add simple qcow2 image validation

46dbf28

initial unit tests

b9559ed

test mem/cpu limit code and incorporate pr comments

e60775e

do dnf clean after installing packages

9f08b40

refactor some tests to not call qemu-img binaries

b989bc1

refactor data stream tests to not call qemu-img

d070396

update tests for better coverage

6b13c7f

add tests that resource limits are actually enforced

7469b1a

rebase and update for dataStream changes

f0472ac

move prlimit/exec stuff from util to system

696aec8

use deferred in test functions that override default interfaces

d1b8223

rebase and refactoring

46528ff

mhenriks force-pushed the streaming-qcow branch from add4373 to 22051a2 Compare September 7, 2018 00:16

don't attempt to stream-convert http files that require creds

d9bbc4e

mhenriks force-pushed the streaming-qcow branch from 22051a2 to d9bbc4e Compare September 7, 2018 00:41

aglitke removed the no-merge-til-e2e-testing-done label Sep 7, 2018

aglitke removed the WIP Work in Progress label Sep 7, 2018

Merge branch 'master' into streaming-qcow

e4c54af

Merge branch 'master' into streaming-qcow

0705741

aglitke merged commit 368e676 into kubevirt:master Sep 7, 2018

Davo911 mentioned this pull request May 3, 2024

Prlimit Unit Test fail on s390x with go1.21.5 #3226

Closed

		)

		// ConvertQcow2ToRaw converts a local qcou2 image to raw format


		const killedByTestError = "Had to kill process"

		func TestLimits(t *testing.T) {

Streaming qcow2->raw conversion and resource limits on qemu-img subprocess #317

Streaming qcow2->raw conversion and resource limits on qemu-img subprocess #317

Conversation

mhenriks commented Aug 6, 2018 • edited Loading

jeffvance commented Aug 6, 2018

Choose a reason for hiding this comment

jeffvance Aug 6, 2018 • 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

jeffvance Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffvance Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffvance Aug 8, 2018 • 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

jeffvance Aug 8, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aglitke commented Sep 6, 2018

aglitke commented Sep 7, 2018

mhenriks commented Sep 7, 2018

aglitke commented Sep 7, 2018

awels commented Sep 7, 2018

mhenriks commented Aug 6, 2018 •

edited

Loading

jeffvance Aug 6, 2018 •

edited

Loading

jeffvance Aug 6, 2018 •

edited

Loading

jeffvance Aug 6, 2018 •

edited

Loading

jeffvance Aug 8, 2018 •

edited

Loading

jeffvance Aug 8, 2018 •

edited

Loading