close stream, check size, and fix backingstore and other minor size assignement

[MalloZup]

this is a logical backport of what we are doing in utils.volume.go file already

The history behind this is a tentative to fix an issue we have currently when updating the cloudinit.iso and creating a vol on remote-kvm server.

[dmacvicar]

Please hold until I merge #479 which is more important and touches also the same file. Then we rebase this one.

[MalloZup]

@dmacvicar #479 merged, we can merge this. feel free to merge once you have time

[dmacvicar]

I was reviewing the code again, and I am not sure this is the right thing (nor the current code).

https://github.com/dmacvicar/terraform-provider-libvirt/blob/master/libvirt/stream.go implements Close:

// Close closes the stream
func (sio *StreamIO) Close() error {
	return sio.Stream.Finish()
}

However, this is not part of Reader nor Writer, and it is not used. We actually defer a Finish() just after creating the stream and before wrapping it:

// download ISO file
stream, err := virConn.NewStream(0)
if err != nil {
	return tmpFile, err
}
defer stream.Finish()

If we look at the wrapper of stream in go:

// See also https://libvirt.org/html/libvirt-libvirt-stream.html#virStreamRecv
func (v *Stream) Recv(p []byte) (int, error) {
	var err C.virError
	n := C.virStreamRecvWrapper(v.ptr, (*C.char)(unsafe.Pointer(&p[0])), C.size_t(len(p)), &err)
	if n < 0 {
		return 0, makeError(&err)
	}
	if n == 0 {
		return 0, io.EOF
	}

	return int(n), nil
}

The C documentation for virStreamFinish states:

Indicate that there is no further data to be transmitted on the stream. For output streams this should be called once all data has been written. For input streams this should be called once virStreamRecv returns end-of-file.

This method is a synchronization point for all asynchronous errors, so if this returns a success code the application can be sure that all data has been successfully processed.

One can confirm the workflow in virStreamRecv docs example:

• Loop
  • Recv
  • if err is EOF, Finish stream, and break
  • If err is other either when reading or writing to our sink, Abort stream
• If the stream is not aborted, call Finish again and see if an error is reported (sync point for error)
• in all cases, Free the stream

The alternative code that we have for volumes is:

defer func() {
	if uint64(bytesCopied) != info.Capacity {
		stream.Abort()
	} else {
		stream.Finish()
	}
	stream.Free()
}()

I can't see the full logic in that defer. For example, the last stream.Finish() may hide an error.

I suspect that code was modified to check for bytesCopied vs info.Capacity because the copy failed, but returned no error, because the libvirt docs were ignored and the finish to get the error was not called. The caller noticed in this failure the bytes copied were not full and therefore compared this.

If I had to copy the logic, and given that io.Copy stops at the EOF, I think we should:

• wrap the stream
  • defer Free
• io.Copy
  • if error either in copy or elsewhere, Abort the stream
  • otherwise, Finish the stream
    • if finishing the stream returns error, report error (this error may not had shown in the io.Copy loop)

And probably I would, unless we find a way to encapsulate this logic in the stream wrapper itself, remove the Close function.

[MalloZup]

tests coverage after running make testacc

[MalloZup]

@dmacvicar @inercia i have repushed things according to @dmacvicar comments.

(The close call was not needed.) i added the test coverage for showing that now the finish is called.

[MalloZup]

cloudinit stream

[dmacvicar]

Did this help with the issues you were facing?

[MalloZup]

It was not tested yet but i think is better to have it ( it is more clear)@moio @Bischoff we might give a try with this one, let.me know when we could try it out

[Bischoff]

I tested the initial version from Dario (not the latest version from here).

I don't really want to test the latest version as we gave up this cloudinit thing, and I don't think it would solve the cloudinit problem anyway.

But the code looks good to me and I'm still in favour of integrating it (once it's tested of course).

[moio]

Currently I have no urgency to bring cloudinit back to sumaform. I will ping next time someone needs it.

[MalloZup]

to me we can merge this as standalone improvement for better handling error on stream and also the mechanism finish.stream is better then before. If the sumaform community has different Prios now, i can understand, however i would have enjoyed to see this applied because at least we could detect the root cause of the error or even fix it.(sumaform cloudinit) I think with this we could print the error where before was not printed but yop, i can understand. On the kubic side we will continue to use cloudinit and so far we had not issues with it. tia 🌻

[MalloZup]

setting this to wip just for doing some tests

[MalloZup]

@dmacvicar i'm finding something interesting... namely that in some cases the volume.size that we set in the download/upload function is 0 as lenght(fortunately the call works even with 0) but i don't think this is what we want.

We are failing here regurlarly
7e51f32#diff-36a249a04d1767fbf36755ed81a8fd73R262

I think the fact that we were checking the size in defer function, and we didn't catch the error, has hidden this bug.

i can reproduce with make testacc regularly. I think we detect already the image size but i think the variable size get reset to 0 by another call somewhere in the volume.go (when it should not) , i will come later with more details on this.

[MalloZup]

@dmacvicar so i just found out why i was getting the error. It was a small bug when we set the Size for backingStore volume and also when assign to the capacity we should ensure it that the Size is not 0, or it set, this with the https://godoc.org/github.com/hashicorp/terraform/helper/schema#ResourceData.GetOk
see 6409471
Fortunately i could find it out with the new code, before the defer code was shadowing this error.

Now the tests pass again, and imho we can merge it 🌻