Skip to content

MIXEDARCH-353: Add yq-v4 to the upi-installer image for CI and copy yq3 from a previous stage's manifest-list image#7567

Merged
openshift-ci[bot] merged 1 commit intoopenshift:masterfrom
aleskandro:upi-yq-v4
Oct 25, 2023
Merged

MIXEDARCH-353: Add yq-v4 to the upi-installer image for CI and copy yq3 from a previous stage's manifest-list image#7567
openshift-ci[bot] merged 1 commit intoopenshift:masterfrom
aleskandro:upi-yq-v4

Conversation

@aleskandro
Copy link
Member

@aleskandro aleskandro commented Oct 10, 2023

While new steps are being implemented in Prow, users may want to pivot to yq-v4. This commit installs yq-v4 in the upi-installer image.
Moreover, by copying the yq-v3 and yq-v4 binaries from previous stages based on manifest-list images, we guarantee the yq binaries are the ones for the architecture of the upi image being built.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Oct 10, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Oct 10, 2023

@aleskandro: This pull request references MIXEDARCH-353 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.15.0" version, but no target version was set.

Details

In response to this:

While new steps are being implemented in Prow, users may want to pivot to yq-v4. This commit adds a layer to download and install yq-v4 in the upi-installer image. Moreover, let the yq-v3 and yq-v4 be downloaded for arm64 if we build the image on arm64.

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.

@aleskandro
Copy link
Member Author

/cc @jianlinliu
due to the previous conversation.

Hi @r4f4 , I'm not sure the way I'm doing the (per-arch) integrity verification is the best in terms of multi-arch enablement... Any suggestions?

/cc @r4f4

@r4f4
Copy link
Contributor

r4f4 commented Oct 10, 2023

Hi @r4f4 , I'm not sure the way I'm doing the (per-arch) integrity verification is the best in terms of multi-arch enablement... Any suggestions?

/cc @r4f4

A few observations:

  1. We are trying to get rid of the rhel7 Dockerfile.upi.ci in favor of the rhel8-based one. See images: Cleanup CI Dockerfiles #7507.
  2. One option would be to have both yqs installed but suffixed with the arch: yq-amd64 and yq-arm64. Then the ci steps can just use the appropriate one; or
  3. Another option is to create a manifest-listed container image with yq and import it like we do with govc: https://github.com/openshift/installer/blob/master/images/installer/Dockerfile.upi.ci.rhel8#L13

Your change works for yq specifically but, as you noted, it's better if we can establish the pattern we want for multi-arch enablement going forward.

@patrickdillon any opinions, suggestions?

@jianlinliu
Copy link
Contributor

jianlinliu commented Oct 10, 2023

One option would be to have both yqs installed but suffixed with the arch: yq-amd64 and yq-arm64. Then the ci steps can just use the appropriate one;

The step may be run in arm64 or amd64 build farm, but the yq command in the step is using a hard-code yq binary name, it would be hard to be compatible with different arch of build farms, right?

@aleskandro
Copy link
Member Author

aleskandro commented Oct 10, 2023

One option would be to have both yqs installed but suffixed with the arch: yq-amd64 and yq-arm64. Then the ci steps can just use the appropriate one;

The step may be run in arm64 or amd64 build farm, but the yq command in the step is using a hard-code yq binary name, it would be hard to be compatible with different arch of build farms, right?

Mmm, I'm against that as it moves the logic to handle the arch of the binary from build time to runtime.
As of now, we download the correct binary based on the architecture of the builder agent, meaning that this is compatible with a future manifest-list-based build of the image (which I hope we will get eventually).

My main concern is about the integrity verification with the hash as I needed to add another build arg (ARG) to track the arm64 hash. Alternatively, we could use bash associative arrays and loose the possibility of overriding the versions to install via --build-arg.

We are trying to get rid of the rhel7 Dockerfile.upi.ci in favor of the rhel8-based one. See #7507.

All right, if you think #7507 could land before this, I can remove the changes to the rhel7 image dockerfile.

Another option is to create a manifest-listed container image with yq and import it like we do with govc: https://github.com/openshift/installer/blob/master/images/installer/Dockerfile.upi.ci.rhel8#L13

Yeah, this is interesting. I'll check if the yq repo links any upstream manifest-list-based images we can copy the binary from.

aleskandro added a commit to aleskandro/openshift-release that referenced this pull request Oct 10, 2023
With openshift/installer#7567, the upi-installer image will depend on the quay.io/multi-arch/yq image to install those binaries in compliance with the architecture of the built image.
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Oct 10, 2023

@aleskandro: This pull request references MIXEDARCH-353 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.15.0" version, but no target version was set.

Details

In response to this:

While new steps are being implemented in Prow, users may want to pivot to yq-v4. This commit installs yq-v4 in the upi-installer image.
Moreover, by copying the yq-v3 and yq-v4 binaries from previous stages based on manifest-list images, we guarantee the yq binaries are the ones for the architecture of the upi image being built.

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.

@aleskandro aleskandro changed the title MIXEDARCH-353: Add yq-v4 to the upi-installer image for CI MIXEDARCH-353: Add yq-v4 to the upi-installer image for CI and copy yq3 from a previous stage's manifest-list image Oct 10, 2023
@aleskandro
Copy link
Member Author

Blocked by #7507

Depends on openshift/release#44166

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 10, 2023
@aleskandro
Copy link
Member Author

/jira refresh

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Oct 10, 2023

@aleskandro: This pull request references MIXEDARCH-353 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.15.0" version, but it targets "openshift-4.15" instead.

Details

In response to this:

/jira refresh

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.

@aleskandro
Copy link
Member Author

aleskandro commented Oct 10, 2023

Hi @jeffdyoung @tvardema , we would check-in yq-v4 in the upi-installer image so that steps in openshift/release do not need to download it at runtime (for example, ipi-install-heterogeneous).

While on this issue, I would download the binaries for yq3 and yq4 according to the architecture of the built image.

Rafael suggested using an intermediate stage based on a multi-arch image with these binaries to streamline the issue more easily than (1) downloading the proper binary according to the result of uname -m, and (2) storing hashes for each architecture to perform the integrity verification here at build time. See aleskandro@7738ab9

Using an intermediate-stage multi-arch image will allow us to delegate the integrity check to the image registry facilities and the build process of the consumed images.

However, while yq4 has an upstream manifest-list image in the docker hub, the yq3 image is a single-arch amd64-only manifest yet... Therefore, I added a manually built yq3 image in quay.io/multi-arch/yq using the binaries from the release page in GitHub and mirrored yq4 from docker.io too, for consistency.

Therefore, the upi-installer image we use in CI will depend on quay.io/multi-arch/yq:3.3.0 and quay.io/multi-arch/yq:4.30.5. Do you have any concerns from your side with this?

Refer to only 69e391e74d83a4acdb0468bcab397579f1b1104b (and openshift/release#44166) for review. This PR will need rebase once the other, including the first commit, lands.

…ous stage's manifest-list image

While new steps are being implemented in Prow, users may want to pivot to yq-v4. This commit installs yq-v4 in the upi-installer image.
Moreover, by copying the yq-v3 and yq-v4 binaries from previous stages based on manifest-list images, we guarantee the yq binaries are the ones for the architecture of the upi image being built.
@aleskandro
Copy link
Member Author

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 19, 2023
@sadasu
Copy link
Contributor

sadasu commented Oct 19, 2023

/retest-required

1 similar comment
@aleskandro
Copy link
Member Author

/retest-required

@r4f4
Copy link
Contributor

r4f4 commented Oct 20, 2023

/test e2e-gcp-ovn e2e-azure-ovn

@aleskandro
Copy link
Member Author

/retest-required

openshift-ci bot pushed a commit to openshift/release that referenced this pull request Oct 20, 2023
With openshift/installer#7567, the upi-installer image will depend on the quay.io/multi-arch/yq image to install those binaries in compliance with the architecture of the built image.
Copy link
Contributor

@r4f4 r4f4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2023
@aleskandro
Copy link
Member Author

/retest-required

@r4f4
Copy link
Contributor

r4f4 commented Oct 21, 2023

/assign @sadasu
The tests are looking good. e2e-azure-ovn-upi is showing some e2e failures which are unrelated to the yq binaries.

@r4f4
Copy link
Contributor

r4f4 commented Oct 24, 2023

/override ci/prow/e2e-azure-ovn-upi
Just some e2e failures not related to the PR.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 24, 2023

@r4f4: Overrode contexts on behalf of r4f4: ci/prow/e2e-azure-ovn-upi

Details

In response to this:

/override ci/prow/e2e-azure-ovn-upi
Just some e2e failures not related to the PR.

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.

@sadasu
Copy link
Contributor

sadasu commented Oct 24, 2023

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 24, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sadasu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 24, 2023
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 5d0755f and 2 for PR HEAD 3e8ec67 in total

@aleskandro
Copy link
Member Author

/retest-required

@r4f4
Copy link
Contributor

r4f4 commented Oct 25, 2023

/test e2e-aws-ovn e2e-aws-ovn-upi e2e-azure-ovn-upi

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 25, 2023

@aleskandro: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-openstack-nfv-intel 69e391e74d83a4acdb0468bcab397579f1b1104b link false /test e2e-openstack-nfv-intel
ci/prow/e2e-openstack-upi 69e391e74d83a4acdb0468bcab397579f1b1104b link false /test e2e-openstack-upi
ci/prow/okd-e2e-aws-ovn 3e8ec67 link false /test okd-e2e-aws-ovn
ci/prow/okd-e2e-aws-ovn-upgrade 3e8ec67 link false /test okd-e2e-aws-ovn-upgrade
ci/prow/okd-scos-e2e-aws-ovn 3e8ec67 link false /test okd-scos-e2e-aws-ovn

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@aleskandro
Copy link
Member Author

/retest-required

@r4f4
Copy link
Contributor

r4f4 commented Oct 25, 2023

/override ci/prow/e2e-azure-ovn-upi ci/prow/e2e-gcp-ovn-upi
e2e failures not related.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 25, 2023

@r4f4: Overrode contexts on behalf of r4f4: ci/prow/e2e-azure-ovn-upi, ci/prow/e2e-gcp-ovn-upi

Details

In response to this:

/override ci/prow/e2e-azure-ovn-upi ci/prow/e2e-gcp-ovn-upi
e2e failures not related.

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.

@openshift-ci openshift-ci bot merged commit 8b1b663 into openshift:master Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants