Multiarch image support

[Prajyot-Parab]

• added multi-arch image support

closes #1645

[nrb]

Several inline comments. Also, can you provide documentation for how this should be invoked?

I tried the following on linux amd64:

x1c in /home/nrb/go/src/github.com/heptio/velero (git) Prajyot-Parab-multiarch-image-support
% make all-manifest
make[1]: Entering directory '/home/nrb/go/src/github.com/heptio/velero'
no such manifest: gcr.io/heptio-images/velero-amd64:master
make[1]: *** [Makefile:215: .manifest-gcr.io/heptio-images/velero] Error 1
make[1]: Leaving directory '/home/nrb/go/src/github.com/heptio/velero'
make: *** [Makefile:102: all-manifest] Error 2

x1c in /home/nrb/go/src/github.com/heptio/velero (git) Prajyot-Parab-multiarch-image-support
% make all-manifest
make[1]: Entering directory '/home/nrb/go/src/github.com/heptio/velero'
no such manifest: gcr.io/heptio-images/velero-amd64:master
make[1]: *** [Makefile:215: .manifest-gcr.io/heptio-images/velero] Error 1
make[1]: Leaving directory '/home/nrb/go/src/github.com/heptio/velero'
make: *** [Makefile:102: all-manifest] Error 2

x1c in /home/nrb/go/src/github.com/heptio/velero (git) Prajyot-Parab-multiarch-image-support
% make manifest
no such manifest: gcr.io/heptio-images/velero-amd64:master
make: *** [Makefile:215: .manifest-gcr.io/heptio-images/velero] Error 1

[carlisia]

👀 but deferring this.

[Prajyot-Parab]

Several inline comments. Also, can you provide documentation for how this should be invoked?

I tried the following on linux amd64:

x1c in /home/nrb/go/src/github.com/heptio/velero (git) Prajyot-Parab-multiarch-image-support
% make all-manifest
make[1]: Entering directory '/home/nrb/go/src/github.com/heptio/velero'
no such manifest: gcr.io/heptio-images/velero-amd64:master
make[1]: *** [Makefile:215: .manifest-gcr.io/heptio-images/velero] Error 1
make[1]: Leaving directory '/home/nrb/go/src/github.com/heptio/velero'
make: *** [Makefile:102: all-manifest] Error 2

x1c in /home/nrb/go/src/github.com/heptio/velero (git) Prajyot-Parab-multiarch-image-support
% make all-manifest
make[1]: Entering directory '/home/nrb/go/src/github.com/heptio/velero'
no such manifest: gcr.io/heptio-images/velero-amd64:master
make[1]: *** [Makefile:215: .manifest-gcr.io/heptio-images/velero] Error 1
make[1]: Leaving directory '/home/nrb/go/src/github.com/heptio/velero'
make: *** [Makefile:102: all-manifest] Error 2

x1c in /home/nrb/go/src/github.com/heptio/velero (git) Prajyot-Parab-multiarch-image-support
% make manifest
no such manifest: gcr.io/heptio-images/velero-amd64:master
make: *** [Makefile:215: .manifest-gcr.io/heptio-images/velero] Error 1

The error is observed as the image tagging is changed and now it's looking for images like
gcr.io/heptio-images/velero-amd64:master
gcr.io/heptio-images/velero-ppc64le:master
(once these images have been pushed you won't encounter above issue in future)
as now the final manifest will be tagged as gcr.io/heptio-images/velero:master

Try this workflow

make all-build all-containers all-push all-manifest

1. Build binaries for all the platforms part of ( CLI_PLATFORMS list)
   Binaries will be created for : linux-amd64 linux-arm linux-arm64 darwin-amd64 windows-amd64 linux-ppc64le

2. Build images for all the platforms part of ( CONTAINER_PLATFORMS list)
   Images will be created for : linux-amd64 linux-ppc64le
   Example:
   gcr.io/heptio-images/velero-amd64:master
gcr.io/heptio-images/velero-ppc64le:master

3. Push all the images part of ( CONTAINER_PLATFORMS list)
   Push images for : linux-amd64 linux-ppc64le i.e above images

4. Create and push manifest
   Example of manifest:
   gcr.io/heptio-images/velero:master

[carlisia]

The commented out code should be removed if not used now. We can always recover it later by going thru the git history if we want to bring any of it back.

[Prajyot-Parab]

The commented out code should be removed if not used now. We can always recover it later by going thru the git history if we want to bring any of it back.

Agreed

[clnperez]

@nrb have all your concerns been addressed?

[skriss]

Just a couple small comments. I would like to do some testing on this to make sure all the things we rely on continue to work as expected. It would also be great if we could get the upstream restic PR merged so that we could change to downloading that binary from the GitHub release itself.

[clnperez]

@nrb -- where do you think makes the most sense for the all-manifests target to be called?

[nrb]

@clnperez I'm not sure I understand your question entirely; do you mean where will it be invoked by CI/CD?

If that's the case, I'm currently working on getting our container images published via a deploy script on Travis CI, which is likely where it would go.

[clnperez]

@nrb -- yep. wasn't sure if that was something you wanted @Prajyot-Parab to add to the PR or if it was something you needed to add

[nrb]

@clnperez Cool - I think to prevent your team from constantly changing this, we could merge this before the Travis work. The biggest hang up I think we have is that the Power restic binary is coming from a 3rd party instead of the original developer's repo.

I recognize that the restic repo has slowed in merging changes and making releases, so I'll see if we can talk to our legal team to see what they'd advise.

[rpsene]

@nrb let me know if you want to have some validation of the content we have at Unicamp, I can create a new build mechanism while we do not get the merge approved on the restic side. We can audit anything you want to ensure the package integrity.

[nrb]

@nrb -- where do you think makes the most sense for the all-manifests target to be called?

I have a more definitive answer on this now - it would be at https://github.com/heptio/velero/blob/master/hack/gcr-push.sh#L75

restic has merged their PR, so my main request has now been address. Removing this change request now, and re-reviewing.

[Prajyot-Parab]

@Prajyot-Parab Could you provide workflow described in this comment as documentation for building the manifest in this PR?

@nrb Please take a look at this Manifest doc and let me know about your thoughts on the same. I will add it to PR (after your review/approval)

CC: @rpsene @clnperez

[skriss]

@Prajyot-Parab I had a few additional comments/questions here.

I'd also definitely like to test this on a branch - @nrb, not sure if you're working on that? If not, I can help out.

[nrb]

Thanks @Prajyot-Parab and @clnperez for being patient with us and addressing Steve's comments on the Makefile changes.

Also, to be transparent, I'm in the process of moving from Travis CI to GitHub Actions. (#2104) However, since our team is going to be out from Dec 23 2019 to Jan 6 2020, I'm not going to push forward on moving container image builds from Travis to GitHub Actions.

That shouldn't affect this PR, as so far there's nothing touching travis files right and we can simply invoke the appropriate make targets in the GitHub Action files, but I wanted to be sure that everyone was aware that the CI runner work was happening in parallel.

[rpsene]

Thanks @Prajyot-Parab and @clnperez for being patient with us and addressing Steve's comments on the Makefile changes.

Also, to be transparent, I'm in the process of moving from Travis CI to GitHub Actions. (#2104) However, since our team is going to be out from Dec 23 2019 to Jan 6 2020, I'm not going to push forward on moving container image builds from Travis to GitHub Actions.

That shouldn't affect this PR, as so far there's nothing touching travis files right and we can simply invoke the appropriate make targets in the GitHub Action files, but I wanted to be sure that everyone was aware that the CI runner work was happening in parallel.

I would like to state the GH Actions still does not work on Power, how could we get builds/pr validated on Power? Can we keep Travis in addition to GH Actions?

[nrb]

I would like to state the GH Actions still does not work on Power, how could we get builds/pr validated on Power? Can we keep Travis in addition to GH Actions?

Is there something I'm missing in this PR that makes the jobs on Travis run on Power? The Velero builds are happening via Go cross-compilation, and the restic builds are downloaded, correct?

[Prajyot-Parab]

I would like to state the GH Actions still does not work on Power, how could we get builds/pr validated on Power? Can we keep Travis in addition to GH Actions?

Is there something I'm missing in this PR that makes the jobs on Travis run on Power? The Velero builds are happening via Go cross-compilation, and the restic builds are downloaded, correct?

@nrb Yes Velero builds are cross-compiled, and the restic builds are downloaded. I think @rpsene may be referring to having builds in future for Power (if needed since Travis provides Power node). @rpsene is my understanding inline with yours ?

[rpsene]

I would like to state the GH Actions still does not work on Power, how could we get builds/pr validated on Power? Can we keep Travis in addition to GH Actions?

Is there something I'm missing in this PR that makes the jobs on Travis run on Power? The Velero builds are happening via Go cross-compilation, and the restic builds are downloaded, correct?

@nrb Yes Velero builds are cross-compiled, and the restic builds are downloaded. I think @rpsene may be referring to having builds in future for Power (if needed since Travis provides Power node). @rpsene is my understanding inline with yours ?

@Prajyot-Parab yep, you are right! I'm looking for the future, but as we are cross-compiling, we are good to go!

[nrb]

Thanks for clarifying! We're moving away from Travis because of longevity and support concerns, for what it's worth.

Hopefully GitHub Actions will get broader capabilities, including PowerPC support, in the future but for this project I think the cross-compilation addresses it.

[skriss]

We also need to update the docs -- https://github.com/vmware-tanzu/velero/blob/master/site/docs/master/build-from-source.md and https://github.com/vmware-tanzu/velero/blob/master/site/docs/v1.2.0/build-from-source.md -- to reflect the Makefile changes.

Specifically, under the "Making images and updating Velero", we need to revise the instructions to cover the manifest flow. At a minimum, we need to indicate that make push will push an image named e.g. velero-amd64:<tag>, and that the user needs to run make manifest to get the velero:<tag> manifest list pushed. It would be nice to also cover that the user can run e.g. MANIFEST_PLATFORMS=amd64 make manifest to push a manifest that has just the amd64 image, if they aren't interested in a ppc64le one (or vice versa).

FWIW, the command I use for building and pushing a container image was previously:

REGISTRY=steveheptio VERSION=foo make container push

and will now be:

REGISTRY=steveheptio VERSION=foo MANIFEST_PLATFORMS=amd64 make container push manifest

So if you want to include that in the doc as an example all-encompassing command, that would be great too.

[nrb]

I second Steve's request for some documentation changes, and there's a TODO comment in the Makefile that can be removed now that multiarch manifests are being implemented in this PR.

Once those things are done, I'm good to approve of this change.

[Prajyot-Parab]

We also need to update the docs -- https://github.com/vmware-tanzu/velero/blob/master/site/docs/master/build-from-source.md and https://github.com/vmware-tanzu/velero/blob/master/site/docs/v1.2.0/build-from-source.md -- to reflect the Makefile changes.

Specifically, under the "Making images and updating Velero", we need to revise the instructions to cover the manifest flow. At a minimum, we need to indicate that make push will push an image named e.g. velero-amd64:<tag>, and that the user needs to run make manifest to get the velero:<tag> manifest list pushed. It would be nice to also cover that the user can run e.g. MANIFEST_PLATFORMS=amd64 make manifest to push a manifest that has just the amd64 image, if they aren't interested in a ppc64le one (or vice versa).

FWIW, the command I use for building and pushing a container image was previously:

REGISTRY=steveheptio VERSION=foo make container push

and will now be:

REGISTRY=steveheptio VERSION=foo MANIFEST_PLATFORMS=amd64 make container push manifest

So if you want to include that in the doc as an example all-encompassing command, that would be great too.

@skriss @nrb I updated the above-mentioned docs and added them to this PR. Do let me know in case of any issues with the same.

[skriss]

@Prajyot-Parab thanks for the docs updates, will look shortly.

Just a note on timing - given that most of the Velero maintainers are about to head out of office for the holidays, we're probably going to defer merging this until the new year. From my point of view, the code looks in good shape (I'll take one more look today) so it should be good to go when we pick it up in 2020.

[skriss]

LGTM. @nrb I'd appreciate a once-over from you as well before merge.

[nrb]

Will add to my queue for today. From what I remember before break it should be good, but I'll do one more double check just to refresh my memory.

[skriss]

@Prajyot-Parab just wanted to circle back here and say thanks for your patience, and for seeing this through!