ppc64le helm image build #1533

clnperez · 2019-06-07T15:48:26Z

build for ppc64le:

the sdk cli
the helm operator image

and create a "multi-arch" image for helm

Signed-off-by: Christy Norman christy@linux.vnet.ibm.com

Description of the change:

Motivation for the change:

openshift-ci-robot · 2019-06-07T15:48:46Z

Hi @clnperez. Thanks for your PR.

I'm waiting for a operator-framework or openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

clnperez · 2019-06-07T15:49:07Z

I tested this as much as possible using our internal Travis, but, we'll see what happens when it runs here.
🤞

clnperez · 2019-06-07T17:54:36Z

Oh, I've also tested out using the cli and the helm image to create and deploy an operator on Power (ppc64le) using https://github.com/IBM/charts/tree/master/stable/ibm-mqadvanced-server-dev.

joelanford · 2019-06-07T19:30:04Z

Cool!

A couple of quick comments.

Unless I'm forgetting something, I don't think we actually have to build ppc64le images on ppc64le machines, since go can cross-compile ppc64le from linux
It doesn't look like there are any changes for the base image in the Dockerfile. Do we have to do something special to pick up the ppc64le version of the registry.access.redhat.com/ubi7/ubi-minimal:latest base image? Maybe that something special is building on ppc64le? 🙂
If we're going to do this for helm, I think we should also do it for the scorecard and ansible base images as well.

@AlexNPavel Any consequences this would have on the transition to prow?

AlexNPavel · 2019-06-07T20:17:19Z

@joelanford If we use go cross-compilation, we should still be fine migrating to prow since we're not testing the ppc64 images, only building them.

AlexNPavel · 2019-06-07T20:18:19Z

/ok-to-test

joelanford · 2019-06-07T20:21:42Z

@AlexNPavel What about the need for ppc64le base images? I see that those exist for ubi7:ubi-minimal, but not sure where/how to inject that. And we may need ppc64le build VMs, if it turns out we can't build the ppc64le images on linux VMs. Do you know if that possibility exists in prow?

clnperez · 2019-06-07T20:25:37Z

The UBI images are multi-arch (actually manifest lists) already.

We could cross-compile instead of using the power nodes built into Travis, but then we'd need to know the hash of the ppc64le UBI image (unless they're pushed with an arch-name...which would be nice for this case, and I'm not sure whether they are or who to ask).

clnperez · 2019-06-07T20:30:11Z

To answer @joelanford 's other questions:

Unless I'm forgetting something, I don't think we actually have to build ppc64le images on ppc64le machines, since go can cross-compile ppc64le from linux

You can cross compile, but some of this, IIRC, does some things in docker (like setting permission bits of the cli binary, etc.) that will blow up if you try to pull down a power image and then do anything other than just COPY into it.

It doesn't look like there are any changes for the base image in the Dockerfile. Do we have to do something special to pick up the ppc64le version of the registry.access.redhat.com/ubi7/ubi-minimal:latest base image? Maybe that something special is building on ppc64le? slightly_smiling_face

That's the manifest list part that thankfully the redhat registry supports and the creators of that image went ahead and configured.

If we're going to do this for helm, I think we should also do it for the scorecard and ansible base images as well.

I had that thought as well, but, I was thinking one at a time and also to see how this one goes. :)

And if you want a ppc64le build node for prow, we can get you a free one hosted at OSU since this is an open-source project.

AlexNPavel · 2019-06-07T20:30:47Z

@joelanford That's good point. We might need to stick to travis for the ppc64le image builds. That shouldn't be too much of a problem though. The main thing we will be using prow for is testing, and while we will be doing image builds there, I don't think there's a problem doing ppc64le builds on travis.

clnperez · 2019-06-12T15:43:49Z

Has any more thought been put into whether merging this as-is is okay?

joelanford

What's the typical build/push workflow for multi-arch images? Is it to build and push all of the images then do a single manifest push? Is it possible to incrementally build the manifest as the individual images are being pushed?

.travis.yml

hack/image/build-helm-image.sh

joelanford · 2019-06-12T16:43:32Z

Makefile

-	./hack/image/build-helm-image.sh $(HELM_BASE_IMAGE):dev
+image/build/helm: build/operator-sdk-dev-${BIN_ARCH}-linux-gnu
+	./hack/image/build-helm-image.sh ${GOARCH} ${BIN_ARCH} $(HELM_BASE_IMAGE):dev
+	./hack/image/build-helm-image.sh ${GOARCH} ${BIN_ARCH} $(HELM_ARCH_IMAGE):dev


See my comment in build-helm-image.sh. I could be forgetting something, but I don't think we need to build the image twice. Can we leave these lines unchanged and rely on the fact that our build host handles the architecture specific things?

That was me being lazy and not wanting to redo the script. The image doesn't build twice b/c of docker's caching. It just gets you both names. the arch image is needed for pushing, but the base one is needed for testing locally. I can find a non-lazy solution to that if you'd prefer.

Actually I could just put in a docker tag there.

We already do docker tag in the push script to change the image tag from :dev to whatever tags we want to push. I'm wondering if we can update the push script to also include the architecture?

i think i tackled that. will see what happens in travis land

i made a dryrun test function and this still needs more work so hold off on reviewing for the time being

Makefile

hack/ci/get-dep.sh

clnperez · 2019-06-12T19:45:59Z

Is it to build and push all of the images then do a single manifest push?

That's how I set this up.

Is it possible to incrementally build the manifest as the individual images are being pushed?

Nope. The manifest list references image layers in a registry so you have to do the build and push of all the images, and then do the manifest create (to get those layers) and push (to put the list on the registry too).

hack/image/push-image-tags.sh

joelanford · 2019-09-04T19:03:14Z

@clnperez Sorry, we've been pretty busy over the past week. Last time I tried, I was still having issues with the quay manifest list support, and I didn't have time to dig into it further. I should have more time this week to pick this back up.

joelanford · 2019-09-06T20:32:00Z

@clnperez I was able to manually run the make targets locally and get things pushed to quay and then confirm that the manifest and manifests lists made it there correctly with curl. So I think the quay side of the equation is fixed.

However, when trying to push the manifest list from travis, I'm still getting a "No such manifest" error. I tried adding a docker pull command to fetch the manifests locally first, but that doesn't seem to help.

Have you had success with travis pushing the manifest list to quay? One difference I see between travis and my local environment is that I'm on docker 19.03.1-ce and travis is on 18.06.0-ce.

clnperez · 2019-09-09T22:23:22Z

@joelanford it looks like your Travis local docker doesn't have the quay certs:

https://travis-ci.org/joelanford/operator-sdk/jobs/581841450#L365 --
open /etc/docker/certs.d/quay.io: permission denied

Here's where I fixed that issue (only seen, IIRC, with rhel/centos distros):
docker/cli#1378

You can try adding the --insecure flag, or you can import the certs for the registry, or you can chmod the dir (I think. It's been a minute... :D )

BTW, you don't need to pull the images down. The manifest info is always pulled from the registry.

joelanford

Thanks for the tip about that permissions error! I had been ignoring it since all of the other push/pull commands seemed to be working. Looks like we need to chmod /etc/docker.

.travis.yml

clnperez · 2019-09-12T16:00:25Z

@joelanford it looks like the failing travis job was cancelled but i can't see why or by whom. any ideas? can we kick that off again?

joelanford · 2019-09-12T21:46:41Z

Not sure why it was cancelled. Just restarted it. I've also got a separate build on my fork that enables the deploy tasks, so I'm babysitting that one too :)

https://travis-ci.org/joelanford/operator-sdk/builds/583868166

clnperez · 2019-09-13T15:39:19Z

thanks @joelanford. looks like all the tests here passed 🎉

joelanford

Sorry I haven't done this review sooner. I was consumed with getting the travis manifest list push working.

I think after this round we'll be very close, if not ready for merge.

/cc @estroz @hasbro17 @jmrodri
PTAL when you get a chance.

joelanford · 2019-09-13T16:45:43Z

hack/image/push-manifest-list.sh

+  push_image=$1; shift || fatal "${FUNCNAME} usage error"
+  arches=$@
+
+  image_name=$push_image # @TODO bug workaround


I think we can remove this line that sets image_name if we add the line in docker_login that I mentioned in this comment

ah, yah that's better. i misread your comment then. thanks. fixed.

joelanford · 2019-09-13T16:46:57Z

hack/image/push-image-tags.sh

-  docker_login
-  check_can_push || return 0
+
+  image_name=$push_image


Ditto with this line.

joelanford · 2019-09-13T16:52:41Z

hack/image/push-image-tags.sh

+  image_name=$push_image
+  docker_login $image_name
+
+  check_can_push


Is there a reason we no longer return 0 if check_can_push fails?

joelanford · 2019-09-13T16:54:01Z

hack/image/push-image-tags.sh

+
+  check_can_push
+
+  docker tag $source_image $push_image


Is this docker tag necessary? I think we're doing this in line 31, which also includes the correct image tags.

i played around with it, trying to remember how it had gotten there. pretty sure it's not, so im taking it out

joelanford · 2019-09-13T16:54:49Z

hack/image/push-manifest-list.sh

+  image_name=$push_image # @TODO bug workaround
+  docker_login $push_image
+
+  check_can_push || exit 0


Should this return like we do in other places, or do we need to exit here instead?

Suggested change

check_can_push || exit 0

check_can_push || return 0

yah, changed that to return

joelanford · 2019-09-13T16:58:58Z

.travis.yml

@@ -68,6 +69,24 @@ x_base_steps:
    services:
      - docker

+  # Manifest list deploy job
+  - &manifest-deploy
+    stage: deploy-manifest


Nit (and I'm not positive this works): Can this be the following:

Suggested change

stage: deploy-manifest

stage: "Deploy multi-arch manifest lists"

yes, that's fine. b/c you can create a manifest list with x86_64 windows and x86_64 linux images :)

oh, did you mean you're not sure if it works functionally b/c there are spaces @joelanford? that i don't know!

that does indeed work just fine

👍 Great! Let's make this change then.

jmrodri · 2019-09-13T18:25:57Z

Verified it created a ppc binary

$ file operator-sdk-v0.10.0-42-g03b2b4b1-ppc64le-linux-gnu 
operator-sdk-v0.10.0-42-g03b2b4b1-ppc64le-linux-gnu: ELF 64-bit LSB executable, 64-bit PowerPC or cisco 7500, version 1 (SYSV), statically linked, Go BuildID=01P6JY3bk0SyQ1MRiRFE/q93uPCNdWuq-wKeS2DJW/_Qq-PbxaUGmR5cY25unK/IYO0QrWb3GLWi58OAaDH, not stripped

jmrodri · 2019-09-13T18:26:03Z

/lgtm

jmrodri

ack

jmrodri · 2019-09-15T19:24:44Z

localhost:~$ arch
ppc64le
localhost:~$ uname -a
Linux localhost 4.19.67-0-vanilla #1-Alpine SMP Mon Aug 19 09:42:34 UTC 2019 ppc64le Linux
localhost:~$ file /tmp/operator-sdk-v0.10.0-17-ge3c3974a-ppc64le-linux-gnu 
-ash: file: not found
localhost:~$ /tmp/operator-sdk-v0.10.0-17-ge3c3974a-ppc64le-linux-gnu --help
An SDK for building operators with ease

Usage:
  operator-sdk [command]

Available Commands:
  add         Adds a controller or resource to the project
  alpha       Run an alpha subcommand
  build       Compiles code and builds artifacts
  completion  Generators for shell completions
  generate    Invokes specific generator
  help        Help about any command
  migrate     Adds source code to an operator
  new         Creates a new operator application
  olm-catalog Invokes a olm-catalog command
  print-deps  Print Golang packages and versions required to run the operator
  run         Runs a generic operator
  scorecard   Run scorecard tests
  test        Tests the operator
  up          Launches the operator
  version     Prints the version of operator-sdk

Flags:
  -h, --help      help for operator-sdk
      --verbose   Enable verbose logging

Use "operator-sdk [command] --help" for more information about a command.

the sdk cli the helm operator image and create a "multi-arch" image for helm Signed-off-by: Christy Norman <christy@linux.vnet.ibm.com>

clnperez · 2019-09-16T18:06:38Z

@joelanford all those changes made and pushed after a little more verification this morning

joelanford

/lgtm

Thanks SO MUCH for the patience @clnperez

/cc @estroz or @hasbro17 can I get one more review of this one before we merge?

clnperez · 2019-09-18T14:28:46Z

thank you for all your reviews and attention to detail @joelanford!

hasbro17 · 2019-09-18T21:56:21Z

Makefile


-release: clean $(release_x86_64) $(release_x86_64:=.asc)
+release: clean $(release_builds) $(release_builds:=.asc)


If we're supporting ppc64le release binaries now then we should update the release doc to mention that.
https://github.com/operator-framework/operator-sdk/blob/master/doc/dev/release.md#operating-systems-and-architectures

ok for a followon pr so it dosn't invalidate this lgtm again?

~~Let's go ahead and fix it here. I can babysit the builds if necessary :)~~

EDIT: on second thought, wordsmithing that change might involve some back and forth, so maybe it is best left for a follow-up.

@hasbro17 Are you okay with that?

Yep we can do a follow up. Just a nit.

hasbro17

1 nit but LGTM overall.

openshift-ci-robot requested review from joelanford and theishshah June 7, 2019 15:48

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 7, 2019

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 7, 2019

joelanford requested review from lilic, AlexNPavel, estroz and hasbro17 June 7, 2019 19:30

joelanford self-assigned this Jun 7, 2019

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 7, 2019

joelanford reviewed Jun 12, 2019

View reviewed changes

hack/image/push-image-tags.sh Outdated Show resolved Hide resolved

joelanford reviewed Jun 12, 2019

View reviewed changes

hack/image/push-image-tags.sh Outdated Show resolved Hide resolved

joelanford reviewed Jun 12, 2019

View reviewed changes

hack/image/push-image-tags.sh Outdated Show resolved Hide resolved

clnperez force-pushed the ppc64le branch 3 times, most recently from f598f32 to 27fe8fb Compare June 17, 2019 21:18

joelanford reviewed Sep 10, 2019

View reviewed changes

.travis.yml Show resolved Hide resolved

clnperez force-pushed the ppc64le branch from 3163eef to 6ac7c19 Compare September 11, 2019 21:17

joelanford reviewed Sep 13, 2019

View reviewed changes

openshift-ci-robot requested a review from jmrodri September 13, 2019 17:01

openshift-ci-robot assigned jmrodri Sep 13, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 13, 2019

jmrodri approved these changes Sep 13, 2019

View reviewed changes

build for ppc64le

726b54b

the sdk cli the helm operator image and create a "multi-arch" image for helm Signed-off-by: Christy Norman <christy@linux.vnet.ibm.com>

clnperez force-pushed the ppc64le branch from 6ac7c19 to 726b54b Compare September 16, 2019 18:06

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2019

joelanford approved these changes Sep 17, 2019

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 17, 2019

joelanford mentioned this pull request Sep 18, 2019

WIP: upgrade images from ubi7 to ubi8 and python version from 2 which is deprecated to 3 #1895

Closed

hasbro17 reviewed Sep 18, 2019

View reviewed changes

hasbro17 approved these changes Sep 18, 2019

View reviewed changes

joelanford merged commit 8bdd7c5 into operator-framework:master Sep 19, 2019

joelanford mentioned this pull request Sep 19, 2019

doc/dev/release.md: add statement ppcle64 support #1940

Merged

hasbro17 mentioned this pull request Sep 30, 2019

Add operator-sdk support to aarch64 architecture #1996

Closed

clnperez mentioned this pull request Nov 12, 2019

host operator sdk cli binary for Power (ppc64le) #2184

Closed

	stage: deploy-manifest
	stage: "Deploy multi-arch manifest lists"


		release: clean $(release_x86_64) $(release_x86_64:=.asc)
		release: clean $(release_builds) $(release_builds:=.asc)

ppc64le helm image build #1533

ppc64le helm image build #1533

Conversation

clnperez commented Jun 7, 2019 • edited Loading

openshift-ci-robot commented Jun 7, 2019

clnperez commented Jun 7, 2019

clnperez commented Jun 7, 2019 • edited Loading

joelanford commented Jun 7, 2019

AlexNPavel commented Jun 7, 2019

AlexNPavel commented Jun 7, 2019

joelanford commented Jun 7, 2019

clnperez commented Jun 7, 2019 • edited Loading

clnperez commented Jun 7, 2019 • edited Loading

AlexNPavel commented Jun 7, 2019

clnperez commented Jun 12, 2019

joelanford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clnperez Jun 12, 2019 • 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

clnperez commented Jun 12, 2019 • edited Loading

joelanford commented Sep 4, 2019

joelanford commented Sep 6, 2019

clnperez commented Sep 9, 2019 • edited Loading

joelanford left a comment

Choose a reason for hiding this comment

clnperez commented Sep 12, 2019

joelanford commented Sep 12, 2019 • edited Loading

clnperez commented Sep 13, 2019

joelanford 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

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

jmrodri commented Sep 13, 2019

jmrodri commented Sep 13, 2019

jmrodri left a comment

Choose a reason for hiding this comment

jmrodri commented Sep 15, 2019 • edited Loading

clnperez commented Sep 16, 2019

joelanford left a comment

Choose a reason for hiding this comment

clnperez commented Sep 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelanford Sep 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasbro17 left a comment

Choose a reason for hiding this comment

clnperez commented Jun 7, 2019 •

edited

Loading

clnperez commented Jun 7, 2019 •

edited

Loading

clnperez commented Jun 7, 2019 •

edited

Loading

clnperez commented Jun 7, 2019 •

edited

Loading

clnperez Jun 12, 2019 •

edited

Loading

clnperez commented Jun 12, 2019 •

edited

Loading

clnperez commented Sep 9, 2019 •

edited

Loading

joelanford commented Sep 12, 2019 •

edited

Loading

jmrodri commented Sep 15, 2019 •

edited

Loading

joelanford Sep 18, 2019 •

edited

Loading