fix for a bug #2226

[koehn]

This fixes a race condition that happens when velero looks at restic's stdout before it has output any lines. The bug (an array index out of bounds) causes velero to crash, which takes out the entire container including restic, which causes restic to fail to run its backup.

The fix checks for null or empty strings and handles them correctly. There are tests to make sure.

closes #2226

[ashish-amarnath]

Thanks for the quick PR!
I made a couple of minor comments.
Considering you are able to get a consistent repro of this, would you be willing to run a test image of Velero with this fix in your environment to confirm that the problem is fully fixed?

[koehn]

Considering you are able to get a consistent repro of this, would you be willing to run a test image of Velero with this fix in your environment to confirm that the problem is fully fixed?

I made some of the changes you mentioned and commented on the others.

I'm happy to run it for testing; how do I build an image? make all doesn't seem to do it.

[koehn]

OK, it looks like docker build -f Dockerfile-velero _output will build a workable image.

[ashish-amarnath]

Building a container image is not very friendly atm. Once you've pushed all your changes I can build a container and share it with you. That may be faster. lmk..

[ashish-amarnath]

@koehn this PR #2233 should make it easy to build container images.

[ashish-amarnath]

LGTM
Let me get you a container image with these changes that you can run :)

[ashish-amarnath]

@koehn you should be able to pull this image to test.

docker pull quay.io/ashish_amarnath/velero:koehn-pr-2231

If you are unable to pull that image for any reason, you can make a local change similar to #2233 and build your container image like
E.g.:

IMAGE=quay.io/ashish_amarnath/velero VERSION=koehn-pr-2231 make container

IMAGE will be your image repository and
VERSION will be your tag for that image.

Then you can use docker push to push it to your remote image repository

[koehn]

One more fix I uncovered in testing.

[koehn]

Thanks I was able to build and test it myself. It works fine; just added a change to avoid decoding a line w/o and JSON and logging an error.

[ashish-amarnath]

LGTM so far. I'll take a closer look tomorrow!

[ashish-amarnath]

LGMT.
Please let us know if you would like me to build you a container image for testing!

[skriss]

two minor comments mainly for your own edification, not merge-blocking. LGTM. Thanks for the PR!

[skriss]

it'd be more consistent with go style to invert this, so e.g.

if len(lastLine) == 0 {
    break
}

...

so the main logic can be less indented. this is not a merge-blocking comment, though.

[skriss]

you should be able to just do if len(b) == 0 { since a nil slice also has a length of zero. Not blocking.