Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARMOCP-417: enable arm64 for agent installer #4441

Merged
merged 1 commit into from
Feb 6, 2023

Conversation

jeffdyoung
Copy link
Contributor

@jeffdyoung jeffdyoung commented Sep 26, 2022

Translate arm64-> aarch64 in instances where we reference rhcos (should mirror x86_64 -> amd64 mapping)
In parallel with openshift-installer change for agent installer: openshift/installer#6401

List all the issues related to this PR

  • New Feature
  • Enhancement
  • Bug fix
  • Tests
  • Documentation
  • CI/CD

What environments does this code impact?

  • Automation (CI, tools, etc)
  • Cloud
  • Operator Managed Deployments
  • None

How was this code tested?

  • assisted-test-infra environment
  • dev-scripts environment (dev-scripts needs additional work to support arm)
  • Reviewer's test appreciated
  • Waiting for CI to do a full test run
  • Manual (Elaborate on how it was tested)
    Manually created agent.iso and installed OCP with arm64 BM HW in our RDU2 lab
  • [] No tests needed

Checklist

  • Title and description added to both, commit and PR.
  • Relevant issues have been associated (see CONTRIBUTING guide)
  • This change does not require a documentation update (docstring, docs, README, etc)
  • Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

  • Are the title and description (in both PR and commit) meaningful and clear?
  • Is there a bug required (and linked) for this change?
  • Should this PR be backported?

@openshift-ci openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 26, 2022
@openshift-ci openshift-ci bot requested review from filanov and flaper87 September 26, 2022 13:49
@codecov
Copy link

codecov bot commented Sep 26, 2022

Codecov Report

Merging #4441 (6d466b5) into master (194fafb) will decrease coverage by 0.01%.
The diff coverage is 47.36%.

❗ Current head 6d466b5 differs from pull request most recent head 17d6603. Consider uploading reports for the commit 17d6603 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4441      +/-   ##
==========================================
- Coverage   67.56%   67.55%   -0.01%     
==========================================
  Files         203      203              
  Lines       30300    30312      +12     
==========================================
+ Hits        20471    20477       +6     
- Misses       8021     8027       +6     
  Partials     1808     1808              
Impacted Files Coverage Δ
internal/common/common.go 30.76% <0.00%> (-1.32%) ⬇️
internal/versions/osimages.go 90.99% <0.00%> (-1.67%) ⬇️
internal/bminventory/inventory.go 69.66% <100.00%> (+<0.01%) ⬆️
internal/oc/release.go 72.31% <100.00%> (+1.29%) ⬆️
internal/versions/versions.go 87.87% <100.00%> (+0.30%) ⬆️

@jeffdyoung
Copy link
Contributor Author

/retest

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 5, 2022
@openshift-ci openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. api-review Categorizes an issue or PR as actively needing an API review. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 5, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 5, 2022
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2022
@jeffdyoung jeffdyoung force-pushed the arm branch 3 times, most recently from 1b1c7d7 to bf0f742 Compare October 14, 2022 19:28
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 15, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 15, 2022
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 19, 2022
swagger.yaml Outdated Show resolved Hide resolved
@jeffdyoung jeffdyoung force-pushed the arm branch 2 times, most recently from 1b36342 to 12a4e28 Compare January 20, 2023 15:08
@carbonin
Copy link
Member

carbonin commented Jan 20, 2023

Do we think we need to normalize these?

This is comparing the architecture in the infraEnv CR with the one in the preprovisioningimage (detected by ironic in some way).

Although I don't think this PR will change either of those as one is provided by the user and the other is provided by ironic so maybe it's fine (or at least as fine as it was before).

@jeffdyoung
Copy link
Contributor Author

Do we think we need to normalize these?

I haven't hit this in my local testing w/agent installer. FWIW, my first attempts was normalizing all comparisons, didn't work and was kinda messy. Normalizing the inputs seems to be the best route.

This is comparing the architecture in the infraEnv CR with the one in the preprovisioningimage (detected by ironic in some way).

Although I don't think this PR will change either of those as one is provided by the user and the other is provided by ironic so maybe it's fine (or at least as fine as it was before).

@carbonin
Copy link
Member

I haven't hit this in my local testing w/agent installer.

You wouldn't, this is a ZTP-specific section of the code, but I think we can leave it for now.

Looks like you'll need to address the subsystem test failure though.

@openshift-ci
Copy link

openshift-ci bot commented Jan 24, 2023

@jeffdyoung: 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/edge-e2e-metal-assisted-capi 49e395d link false /test edge-e2e-metal-assisted-capi
ci/prow/edge-e2e-metal-assisted-static-ip-suite 00ec0bd link false /test edge-e2e-metal-assisted-static-ip-suite

Full PR test history. Your PR dashboard.

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.

swagger.yaml Outdated Show resolved Hide resolved
@jeffdyoung jeffdyoung changed the title ARMOCP-417: enable arm64 for agent installer [WIP]ARMOCP-417: enable arm64 for agent installer Jan 24, 2023
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 24, 2023
@jeffdyoung jeffdyoung changed the title [WIP]ARMOCP-417: enable arm64 for agent installer ARMOCP-417: enable arm64 for agent installer Jan 24, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 24, 2023
@jeffdyoung jeffdyoung requested review from carbonin and removed request for flaper87 and filanov February 6, 2023 04:23
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 6, 2023
@openshift-ci
Copy link

openshift-ci bot commented Feb 6, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carbonin, jeffdyoung

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

The pull request process is described here

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 Feb 6, 2023
@carbonin
Copy link
Member

carbonin commented Feb 6, 2023

FWIW this also resolves https://issues.redhat.com/browse/MGMT-13386

@openshift-merge-robot openshift-merge-robot merged commit 3c7a420 into openshift:master Feb 6, 2023
danielerez pushed a commit to danielerez/assisted-service that referenced this pull request Oct 15, 2023
aleskandro added a commit to aleskandro/assisted-service that referenced this pull request Jan 24, 2025
There is an inconsistency about the name of the architecture to use in the InfraEnv and other CRDs using the OSImage object.
Users can easily provide a different string for the CPUArchitecture fields in such CRs and no error would arise from them as they are not validated, making it harder for the user to debug and understand how to proceed with their deployment.

This commit adds validation for the architecture field among the values in the set {arm64, ppc64le, s390x, x86_64}

Related to openshift#4441
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review Categorizes an issue or PR as actively needing an API review. 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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants