Enable artifacts for s390x #383

BbolroC · 2024-05-07T08:08:24Z

This PR enables all artifacts (container images and binaries) for s390x alongside x86_64. The self-hosted runner to build them is already registered as s390x-runner-01 to the organisation.

The change was verified with a private runner as follows:

AS: https://github.com/BbolroC/trustee/actions/runs/8982469105
KBS: https://github.com/BbolroC/trustee/actions/runs/8982469108
KBS-client: https://github.com/BbolroC/trustee/actions/runs/8982469107

The artifacts are available at:

Fixes: #364

Signed-off-by: Hyounggyu Choi [email protected]

BbolroC · 2024-05-07T10:17:52Z

For reviewers, this PR does not cover the full enablement for s390x. Makefiles for AS and KBS are not yet enabled because cargo install is used in the workflows for them. I will raise a follow-up PR to deal with this by making the affected workflows use make. Thanks. 😉

fitzthum

Looks pretty good.

You might want to check the release helper script and make sure that it will account for the multiarch images you are addding.

fitzthum · 2024-05-07T16:52:28Z

.github/workflows/kbs-client-build-and-push.yaml

+          - instance: ubuntu-22.04
+            arch: x86_64
+          - instance: s390x
+            arch: s390x


Fundamentally we only have two cases here. Maybe we could just have a matrix with the two arches and then put a condition down in the runs-on step to select the instance based on the arch?

I think there is also support for nested arrays in a matrix which might allow you to specify the two pairs directly.

I guess it's fairly common to use this include syntax, but I'm not a huge fan.

Yep, what you are suggesting seems like:

jobs: build_and_push: strategy: matrix: arch: - x86_64 - s390x env: RUSTC_VERSION: 1.76.0 runs-on: ${{ matrix.arch == 'x86_64' ? 'ubuntu-22.04' : 's390x' }}

It looks more concise and cleaner. Thanks for the suggestion.

fitzthum · 2024-05-07T16:56:33Z

.github/workflows/kbs-client-build-and-push.yaml

+    - name: Take a pre-action for self-hosted runner
+      run: |
+        if [ -f "${HOME}/script/pre_action.sh" ]; then
+          "${HOME}/script/pre_action.sh" cc-trustee


Is this script tracked anywhere?

I got the same question for kata containers at kata-containers/kata-containers#8649 (review). The script is managed internally, but set -x could be set for tracking what it does.

fitzthum · 2024-05-07T16:57:13Z

kbs/Makefile

@@ -2,6 +2,8 @@ AS_TYPE ?= coco-as
 HTTPS_CRYPTO ?= rustls
 POLICY_ENGINE ?=

+ARCH := $(shell uname -m)


Should we have a check in case it is something other than the two we support?

It sounds good. I will bring the check. Thanks!

fitzthum · 2024-05-07T17:14:07Z

kbs/docker/Dockerfile

    libsgx-dcap-quote-verify-dev \
-    libtdx-attest-dev
+    libtdx-attest-dev; fi


I think this is fine, but really it might make more sense to connect all this TDX-specific stuff to a TDX-related flag rather than the arch. TDX could always be set to false for s390x

Yes, we could introduce a TDX variable in the build script and then set it to true/false based on the arch, even we could have more fine-grained control over different boolean variables for TEE types. But as discussed in Slack, this could be handled in a follow-up PR. wdyt, @Xynnn007 ?

reasonable. In future, we need to add all plugin flags and by default set them all to true.

fitzthum · 2024-05-07T17:23:46Z

attestation-service/attestation-service/Cargo.toml

@@ -4,7 +4,7 @@ version = "0.1.0"
 edition = "2021"

 [features]
-default = [ "restful-bin", "rvps-grpc", "rvps-builtin", "all-verifier" ]
+default = [ "restful-bin", "rvps-grpc", "rvps-builtin" ]


I'm on the fence about this. Maybe it would be better for us to add a dummy s390x feature or merge @huoqifeng's PR. wdyt @Xynnn007?

nvm i think this is fine. hadn't read the bottom part yet.

fitzthum · 2024-05-07T17:33:37Z

attestation-service/attestation-service/Cargo.toml

+verifier = { path = "../verifier", default-features = false, features = ["all-verifier"] }
+
+[target.'cfg(target_arch = "s390x")'.dependencies]
+verifier = { path = "../verifier", default-features = false, features = ["csv-verifier"] }


So once the s390x verifier is in, will that only be able to be built (and run) on an s390x machine? Will we be able to support validating s390x evidence on x86?

Yes, once the s390x verifier comes in, if it is included in all-verifier, we could see x86 support validating s390x evidence, but not vice versa.

Xynnn007

Looks great. we might need to wait #345 to be merged and then merge this. because the feature flag in Cargo.toml is not prepared I think.

.github/workflows/as-build-and-push.yaml

Xynnn007 · 2024-05-08T02:51:58Z

attestation-service/Dockerfile.as-grpc


 WORKDIR /usr/src/attestation-service
 COPY . .

 # Install golang
-RUN wget https://go.dev/dl/go1.20.1.linux-amd64.tar.gz && \
-    tar -C /usr/local -xzf go1.20.1.linux-amd64.tar.gz
+RUN [ "${ARCH}" = "x86_64" ] && GOARCH="amd64" || GOARCH="${ARCH}" && \


why we need [ "${ARCH}" = "x86_64" ] && GOARCH="amd64" here?

Yeah, this could be confusing to those who are not familiar with bash. I will make it clearer with if else fi. Thanks!

attestation-service/Dockerfile.as-restful

Xynnn007 · 2024-05-08T03:38:31Z

kbs/docker/Dockerfile

    libsgx-dcap-quote-verify-dev \
-    libtdx-attest-dev
+    libtdx-attest-dev; fi


reasonable. In future, we need to add all plugin flags and by default set them all to true.

kbs/docker/Dockerfile

huoqifeng · 2024-05-08T03:58:24Z

Looks great. we might need to wait #345 to be merged and then merge this. because the feature flag in Cargo.toml is not prepared I think.

@Xynnn007 might a circle dependency here, the remain work in #345 requires we can build on s390x because of the IBM SE crate. What about we merge #345 and then this PR and then add the IBM SE crate?

BbolroC · 2024-05-08T06:55:30Z

Looks great. we might need to wait #345 to be merged and then merge this. because the feature flag in Cargo.toml is not prepared I think.

@Xynnn007 might a circle dependency here, the remain work in #345 requires we can build on s390x because of the IBM SE crate. What about we merge #345 and then this PR and then add the IBM SE crate?

Thanks for the comment. I also discussed it with @Xynnn007. We ended up with waiting until #345 is merged and updating Cargo.toml accordingly. 😉 Let's merge yours!!

BbolroC · 2024-05-08T10:23:29Z

Looks pretty good.

You might want to check the release helper script and make sure that it will account for the multiarch images you are addding.

It seems that container images will not be affected by this PR, but an oras artifact kbc-client, which is not handled by the script. So this PR does not hurt the release process. 😉

BbolroC · 2024-05-08T10:56:11Z

Here are the verification result for the last update:

The last verification will be fulfilled again when the verifier is updated by #345.

fitzthum

I'm still a bit suspicious of the release script but we can fix that up afterwards if needed.

BbolroC · 2024-05-09T04:02:49Z

I'm still a bit suspicious of the release script but we can fix that up afterwards if needed.

Now, I got your point. I was only considering whether the multi-arch images hurt the release helper script or not. But you wanted me to enable the script to publish the multi-arch release images. I have updated the PR. Please take a look. 😉

portersrc · 2024-05-10T19:03:13Z

hack/release-helper.sh

+        docker manifest create ${ghcr_repo}/${release_pkg_name}:${release_tag_full} \
+            --amend ${ghcr_repo}/${release_pkg_name}:${release_tag_full}-x86_64 \
+            --amend ${ghcr_repo}/${release_pkg_name}:${release_tag_full}-s390x
+        docker manifest push ${ghcr_repo}/${release_pkg_name}:${release_tag_full}


Thanks for handling this release-helper.sh change with this PR! I wonder if there was a minor bug in the earlier script, though: It never added a latest tag to the new release. If you want, maybe add something like that here:

docker manifest create ${ghcr_repo}/${release_pkg_name}:${latest} \ --amend ${ghcr_repo}/${release_pkg_name}:${release_tag_full}-x86_64 \ --amend ${ghcr_repo}/${release_pkg_name}:${release_tag_full}-s390x docker manifest push ${ghcr_repo}/${release_pkg_name}:${latest}

fitzthum · 2024-05-15T16:48:00Z

Are we waiting on #345 for this?

BbolroC · 2024-05-15T16:54:31Z

Are we waiting on #345 for this?

Yep!

genjuro214 · 2024-05-16T02:57:48Z

@BbolroC , I'm trying to build s390x kbs images based on this PR in our internal pipeline. The problem is we don't have s390x node in the pipeline. So I have to build s390x image on x86 platform.

First, I tried docker buildx with --platform like:

docker buildx build -t ${registry}/kbs:latest --platform linux/s390x --load --build-arg ARCH=s390x --build-arg HTTPS_CRYPTO=openssl . -f kbs/docker/Dockerfile

It works and we don't need more changes on your PR. But the performance is very low. It takes hours to build the rust in this way.

So I also tried to use rust cross-compile support, like:

RUSTFLAGS=" -C linker=s390x-linux-gnu-gcc" CGO_ENABLED=1 GOOS=linux GOARCH=s390x cargo install --path kbs/src/kbs --no-default-features --features coco-as-builtin,resource,openssl,opa --target "s390x-unknown-linux-gnu"

It also works, but requires some changes based on your PR. I think we did similar works in guest-components:
https://github.com/confidential-containers/guest-components/blob/main/attestation-agent/Makefile#L72

I will try those changes in my fork based on your fork, and raise PR after this PR is merged.

BbolroC · 2024-05-17T13:26:16Z

@genjuro214 thanks for your work. I think the best resolution would be not using the self-hosted runner in terms of capability and maintenance. Why don't you create a new PR based on this for the best resolution and merge it instead of this one?

Or you could give me a patch and I could apply it here. 😉

This commit make the existing build/test for attestation agent running on s390x. We will enable `cargo test` after an image for kbs is ready. (confidential-containers/trustee#383) The build option for attestor is not configured here, but a new attester for s390x will be added by confidential-containers#492.

This commit make the existing build/test for attestation agent running on s390x. We will enable `cargo test` after an image for kbs is ready. (confidential-containers/trustee#383) The build option for attestor is not configured here, but a new attester for s390x will be added by confidential-containers#492. Signed-off-by: Hyounggyu Choi <[email protected]>

This commit make the existing build/test for attestation agent running on s390x. We will enable `cargo test` after an image for kbs is ready. (confidential-containers/trustee#383) The build option is configured to use `se-attester`. Signed-off-by: Hyounggyu Choi <[email protected]>

We are encountering an issue on s390x when compiling AS with `all-verifier`. The error message is as follows: ``` error: failed to run custom build command for `tss-esapi-sys v0.5.0` ``` A platform-specific verifier (e.g., `se-verifier`) is used here. (confidential-containers#345) Although we can easily configure the verifier using `--features`, this approach lacks flexibility when the crate is selectively called from outside (e.g., kbs) based on `target_arch`. The optimal solution would be to open up room for configuring the verifier at a `dependencies` level rather than a `features` level. This commit aims to remove `all-verifier` from the default feature set and configure it differently for s390x. Signed-off-by: Hyounggyu Choi <[email protected]>

`.github/actionlint.yaml` is required to make a label `s390x` tolerant against actionlint. Signed-off-by: Hyounggyu Choi <[email protected]>

This commit introduces a job matrix to allow a s390x self-hosted runner to run alongside the existing x86_64 one. Additionally, two new steps (e.g., {pre,post} action) are introduced to manage the self-hosted runner since it is not provisioned instantly for CI. The make target `cli-static-x86_64-linux` is renamed to `cli-static-linux` for platform independence. Signed-off-by: Hyounggyu Choi <[email protected]>

This commit introduces a job matrix to allow a s390x self-hosted runner to run alongside the existing x86_64 one. Additionally, two new steps (e.g., {pre,post} action) are introduced to manage the self-hosted runner since it is not provisioned instantly for CI. To make the published images support multiple architecture, a new job `publish_multi_arch_image` is also introduced. Signed-off-by: Hyounggyu Choi <[email protected]>

This commit introduces a job matrix to allow a s390x self-hosted runner to run alongside the existing x86_64 one. Additionally, two new steps (e.g., {pre,post} action) are introduced to manage the self-hosted runner since it is not provisioned instantly for CI. To run the build steps `kbs` and `kbs-grpc-as` in parallel, the build step is generalized into one step and the relevant configuration values are parameterized into the job matrix. Signed-off-by: Hyounggyu Choi <[email protected]>

We need to update the release helper script accordingly to publish multi-arch images. Signed-off-by: Hyounggyu Choi <[email protected]>

BbolroC · 2024-06-12T15:01:13Z

@Xynnn007 Could you resume reviewing this? I have rebased this onto main as #345 is merged.

Xynnn007

lgtm. Thanks!

This commit make the existing build/test for attestation agent running on s390x. We will enable `cargo test` after an image for kbs is ready. (confidential-containers/trustee#383) The build option is configured to use `se-attester`. Signed-off-by: Hyounggyu Choi <[email protected]>

BbolroC requested a review from sameo as a code owner May 7, 2024 08:08

BbolroC force-pushed the enable-s390x-images branch 3 times, most recently from 763914b to 7d80038 Compare May 7, 2024 08:30

BbolroC changed the title ~~Enable s390x images~~ Enable s390x artifacts May 7, 2024

BbolroC changed the title ~~Enable s390x artifacts~~ Enable artifacts for s390x May 7, 2024

fitzthum reviewed May 7, 2024

View reviewed changes

Xynnn007 reviewed May 8, 2024

View reviewed changes

BbolroC force-pushed the enable-s390x-images branch from 7d80038 to 4890f58 Compare May 8, 2024 10:53

portersrc approved these changes May 8, 2024

View reviewed changes

fitzthum approved these changes May 8, 2024

View reviewed changes

BbolroC force-pushed the enable-s390x-images branch from 4890f58 to 367a0de Compare May 9, 2024 03:58

portersrc reviewed May 10, 2024

View reviewed changes

Xynnn007 marked this pull request as draft May 21, 2024 07:58

BbolroC force-pushed the enable-s390x-images branch 2 times, most recently from 1d351ce to b4b83b8 Compare June 3, 2024 09:07

Xynnn007 marked this pull request as ready for review June 3, 2024 09:09

BbolroC force-pushed the enable-s390x-images branch 3 times, most recently from c6a00bb to 0902814 Compare June 3, 2024 13:31

BbolroC added 3 commits June 12, 2024 16:33

gha: Add actionlint.yaml

0195f1e

`.github/actionlint.yaml` is required to make a label `s390x` tolerant against actionlint. Signed-off-by: Hyounggyu Choi <[email protected]>

BbolroC force-pushed the enable-s390x-images branch 2 times, most recently from bf99277 to 001d905 Compare June 12, 2024 14:40

BbolroC added 3 commits June 12, 2024 16:40

Make release helper script publish multi-arch images

f6947f3

We need to update the release helper script accordingly to publish multi-arch images. Signed-off-by: Hyounggyu Choi <[email protected]>

BbolroC force-pushed the enable-s390x-images branch from 001d905 to f6947f3 Compare June 12, 2024 14:40

Xynnn007 approved these changes Jun 13, 2024

View reviewed changes

Xynnn007 merged commit 25582b0 into confidential-containers:main Jun 13, 2024
17 checks passed

BbolroC deleted the enable-s390x-images branch June 13, 2024 04:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable artifacts for s390x #383

Enable artifacts for s390x #383

BbolroC commented May 7, 2024 •

edited

Loading

BbolroC commented May 7, 2024

fitzthum left a comment

fitzthum May 7, 2024

BbolroC May 7, 2024

fitzthum May 7, 2024

BbolroC May 7, 2024

fitzthum May 7, 2024

BbolroC May 7, 2024

fitzthum May 7, 2024

BbolroC May 7, 2024

Xynnn007 May 8, 2024

fitzthum May 7, 2024

fitzthum May 7, 2024

fitzthum May 7, 2024

BbolroC May 7, 2024

Xynnn007 left a comment

Xynnn007 May 8, 2024

BbolroC May 8, 2024

Xynnn007 May 8, 2024

huoqifeng commented May 8, 2024

BbolroC commented May 8, 2024

BbolroC commented May 8, 2024

BbolroC commented May 8, 2024 •

edited

Loading

fitzthum left a comment

BbolroC commented May 9, 2024

portersrc May 10, 2024

fitzthum commented May 15, 2024

BbolroC commented May 15, 2024

genjuro214 commented May 16, 2024

BbolroC commented May 17, 2024 •

edited

Loading

BbolroC commented Jun 12, 2024

Xynnn007 left a comment

Enable artifacts for s390x #383

Enable artifacts for s390x #383

Conversation

BbolroC commented May 7, 2024 • edited Loading

BbolroC commented May 7, 2024

fitzthum 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

Xynnn007 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

huoqifeng commented May 8, 2024

BbolroC commented May 8, 2024

BbolroC commented May 8, 2024

BbolroC commented May 8, 2024 • edited Loading

fitzthum left a comment

Choose a reason for hiding this comment

BbolroC commented May 9, 2024

Choose a reason for hiding this comment

fitzthum commented May 15, 2024

BbolroC commented May 15, 2024

genjuro214 commented May 16, 2024

BbolroC commented May 17, 2024 • edited Loading

BbolroC commented Jun 12, 2024

Xynnn007 left a comment

Choose a reason for hiding this comment

BbolroC commented May 7, 2024 •

edited

Loading

BbolroC commented May 8, 2024 •

edited

Loading

BbolroC commented May 17, 2024 •

edited

Loading