MGMT-15150: Use same installer binary for all platform types #5334

zaneb · 2023-07-03T13:17:19Z

There's nothing special about platform:none that requires it to use a different installer binary. This was originally done to avoid a problem with the openshift-baremetal-install binary not being available for non-x86 targets, but this was resolved by 1d025b8 (MGMT-9206). We no longer need all this complexity.

This partially reverts commit f9b2f3d.

List all the issues related to this PR

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
Reviewer's test appreciated
Waiting for CI to do a full test run
Manual (Elaborate on how it was tested)
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-robot · 2023-07-03T13:17:22Z

openshift-ci-robot · 2023-07-03T13:17:26Z

codecov · 2023-07-03T13:52:05Z

Codecov Report

Merging #5334 (e73b9c5) into master (7c7a001) will increase coverage by 0.02%.
The diff coverage is 76.92%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5334      +/-   ##
==========================================
+ Coverage   67.55%   67.57%   +0.02%     
==========================================
  Files         226      226              
  Lines       33227    33296      +69     
==========================================
+ Hits        22446    22501      +55     
- Misses       8763     8772       +9     
- Partials     2018     2023       +5

Impacted Files	Coverage Δ
internal/bminventory/inventory.go	`69.83% <ø> (-0.03%)`	⬇️
internal/ignition/dummy.go	`0.00% <0.00%> (ø)`
internal/ignition/ignition.go	`59.84% <0.00%> (ø)`
internal/installercache/installercache.go	`70.51% <100.00%> (ø)`
internal/oc/release.go	`71.83% <100.00%> (+0.46%)`	⬆️

... and 7 files with indirect coverage changes

There's nothing special about platform:none that requires it to use a different installer binary. This was originally done (in f9b2f3d) to avoid a problem with the openshift-baremetal-install binary not being available for non-x86 targets, but this was resolved by 1d025b8 (MGMT-9206).

Since we no longer use different installer binaries for different platforms, we no longer need to pass the platform type down. This allows us to clean up a lot of function signatures. This partially reverts commit f9b2f3d.

eranco74 · 2023-07-09T09:08:54Z

/test ?

openshift-ci · 2023-07-09T09:08:58Z

@eranco74: The following commands are available to trigger required jobs:

/test e2e-agent-compact-ipv4
/test edge-assisted-operator-catalog-publish-verify
/test edge-ci-index
/test edge-e2e-ai-operator-ztp
/test edge-e2e-ai-operator-ztp-sno-day2-workers
/test edge-e2e-ai-operator-ztp-sno-day2-workers-late-binding
/test edge-e2e-metal-assisted
/test edge-e2e-metal-assisted-4-10
/test edge-e2e-metal-assisted-4-11
/test edge-e2e-metal-assisted-4-12
/test edge-e2e-metal-assisted-4-9
/test edge-e2e-metal-assisted-cnv
/test edge-e2e-metal-assisted-lvm
/test edge-e2e-metal-assisted-odf
/test edge-images
/test edge-lint
/test edge-subsystem-aws
/test edge-subsystem-kubeapi-aws
/test edge-unit-test
/test edge-verify-generated-code
/test images
/test mce-images

The following commands are available to trigger optional jobs:

/test e2e-agent-ha-dualstack
/test e2e-agent-sno-ipv6
/test edge-e2e-ai-operator-ztp-3masters
/test edge-e2e-ai-operator-ztp-capi
/test edge-e2e-ai-operator-ztp-compact-day2-masters
/test edge-e2e-ai-operator-ztp-compact-day2-workers
/test edge-e2e-ai-operator-ztp-disconnected
/test edge-e2e-ai-operator-ztp-hypershift-zero-nodes
/test edge-e2e-ai-operator-ztp-multiarch-3masters-ocp-411
/test edge-e2e-ai-operator-ztp-multiarch-sno-ocp-411
/test edge-e2e-ai-operator-ztp-node-labels
/test edge-e2e-ai-operator-ztp-sno-day2-masters
/test edge-e2e-ai-operator-ztp-sno-day2-workers-ignitionoverride
/test edge-e2e-metal-assisted-4-13
/test edge-e2e-metal-assisted-4-14
/test edge-e2e-metal-assisted-day2
/test edge-e2e-metal-assisted-day2-arm-workers
/test edge-e2e-metal-assisted-day2-single-node
/test edge-e2e-metal-assisted-ipv4v6
/test edge-e2e-metal-assisted-ipv6
/test edge-e2e-metal-assisted-kube-api-late-binding-single-node
/test edge-e2e-metal-assisted-kube-api-late-unbinding-ipv4-single-node
/test edge-e2e-metal-assisted-kube-api-net-suite
/test edge-e2e-metal-assisted-mce
/test edge-e2e-metal-assisted-mce-4-10
/test edge-e2e-metal-assisted-mce-sno
/test edge-e2e-metal-assisted-metallb
/test edge-e2e-metal-assisted-none
/test edge-e2e-metal-assisted-onprem
/test edge-e2e-metal-assisted-single-node
/test edge-e2e-metal-assisted-static-ip-suite
/test edge-e2e-metal-assisted-tang
/test edge-e2e-metal-assisted-tpmv2
/test edge-e2e-metal-assisted-upgrade-agent
/test edge-e2e-nutanix-assisted
/test edge-e2e-nutanix-assisted-2workers
/test edge-e2e-vsphere-assisted
/test edge-e2e-vsphere-assisted-4-12
/test edge-e2e-vsphere-assisted-4-13
/test edge-e2e-vsphere-assisted-umn
/test edge-push-pr-image
/test push-pr-image

Use /test all to run the following jobs that were automatically triggered:

pull-ci-openshift-assisted-service-master-e2e-agent-compact-ipv4
pull-ci-openshift-assisted-service-master-edge-ci-index
pull-ci-openshift-assisted-service-master-edge-e2e-ai-operator-ztp
pull-ci-openshift-assisted-service-master-edge-e2e-metal-assisted
pull-ci-openshift-assisted-service-master-edge-images
pull-ci-openshift-assisted-service-master-edge-lint
pull-ci-openshift-assisted-service-master-edge-subsystem-aws
pull-ci-openshift-assisted-service-master-edge-subsystem-kubeapi-aws
pull-ci-openshift-assisted-service-master-edge-unit-test
pull-ci-openshift-assisted-service-master-edge-verify-generated-code
pull-ci-openshift-assisted-service-master-images
pull-ci-openshift-assisted-service-master-mce-images

In response to this:

/test ?

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.

eranco74 · 2023-07-09T09:10:02Z

/test edge-e2e-metal-assisted-none

eranco74

/approve

openshift-ci · 2023-07-09T09:59:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eranco74, zaneb

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:

~~OWNERS~~ [eranco74]

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

eranco74 · 2023-07-09T10:00:40Z

/cc @danielerez

avishayt · 2023-07-09T10:51:14Z

/lgtm

danielerez · 2023-07-09T11:01:54Z

internal/bminventory/inventory.go

@@ -6443,15 +6443,14 @@ func (b *bareMetalInventory) GetKnownApprovedHosts(clusterId strfmt.UUID) ([]*co
 	return b.hostApi.GetKnownApprovedHosts(clusterId)
 }

-// In case cpu architecture is not x86_64 and platform is baremetal, we should extract openshift-baremetal-installer
+// In case cpu architecture is not x86_64, we should extract openshift-baremetal-installer
 // from x86_64 release image as there is no x86_64 openshift-baremetal-installer executable in arm image.
 // This flow does not affect the multiarch release images and is meant purely for using arm64 release image with the x86 hub.
 // Implementation of handling the multiarch images is done directly in the `oc` binary and relies on the fact that `oc adm release extract`
 // will automatically use the image matching the Hub's architecture.
 func isBaremetalBinaryFromAnotherReleaseImageRequired(cpuArchitecture, version string, platform *models.PlatformType) bool {


The name of the func might be a bit confusing now... Maybe worth adding a comment about none platform clusters (i.e. that also the baremetal binary variant can be used).

It's still the -baremetal- binary, so I think the name is still accurate.
TBH it might be less confusing than before, where the logic here said platform == baremetal and in the other location platform != none, and we were relying on the fact that no other platforms are supported on different architectures.

openshift-ci · 2023-07-09T13:08:06Z

@zaneb: all tests passed!

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.

filanov · 2023-08-01T09:46:33Z

/cherry-pick release-ocm-2.8

openshift-cherrypick-robot · 2023-08-01T09:47:21Z

@filanov: #5334 failed to apply on top of branch "release-ocm-2.8":

Applying: MGMT-15150: Use same installer binary for all platform types
Using index info to reconstruct a base tree...
M	internal/bminventory/inventory.go
M	internal/oc/release.go
M	internal/oc/release_test.go
Falling back to patching base and 3-way merge...
Auto-merging internal/oc/release_test.go
CONFLICT (content): Merge conflict in internal/oc/release_test.go
Auto-merging internal/oc/release.go
Auto-merging internal/bminventory/inventory.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 MGMT-15150: Use same installer binary for all platform types
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-ocm-2.8

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.

…ft#5334) * MGMT-15150: Use same installer binary for all platform types There's nothing special about platform:none that requires it to use a different installer binary. This was originally done (in f9b2f3d) to avoid a problem with the openshift-baremetal-install binary not being available for non-x86 targets, but this was resolved by 1d025b8 (MGMT-9206). * Remove platform argument when extracting installer Since we no longer use different installer binaries for different platforms, we no longer need to pass the platform type down. This allows us to clean up a lot of function signatures. This partially reverts commit f9b2f3d.

https://issues.redhat.com/browse/MGMT-15413 --- MGMT-15150: Use same installer binary for all platform types (openshift#5334) * MGMT-15150: Use same installer binary for all platform types There's nothing special about platform:none that requires it to use a different installer binary. This was originally done (in f9b2f3d) to avoid a problem with the openshift-baremetal-install binary not being available for non-x86 targets, but this was resolved by 1d025b8 (MGMT-9206). * Remove platform argument when extracting installer Since we no longer use different installer binaries for different platforms, we no longer need to pass the platform type down. This allows us to clean up a lot of function signatures. This partially reverts commit f9b2f3d.

https://issues.redhat.com/browse/MGMT-15413 --- MGMT-15150: Use same installer binary for all platform types (#5334) * MGMT-15150: Use same installer binary for all platform types There's nothing special about platform:none that requires it to use a different installer binary. This was originally done (in f9b2f3d) to avoid a problem with the openshift-baremetal-install binary not being available for non-x86 targets, but this was resolved by 1d025b8 (MGMT-9206). * Remove platform argument when extracting installer Since we no longer use different installer binaries for different platforms, we no longer need to pass the platform type down. This allows us to clean up a lot of function signatures. This partially reverts commit f9b2f3d. Co-authored-by: Zane Bitter <zbitter@redhat.com>

…ft#5334) * MGMT-15150: Use same installer binary for all platform types There's nothing special about platform:none that requires it to use a different installer binary. This was originally done (in f9b2f3d) to avoid a problem with the openshift-baremetal-install binary not being available for non-x86 targets, but this was resolved by 1d025b8 (MGMT-9206). * Remove platform argument when extracting installer Since we no longer use different installer binaries for different platforms, we no longer need to pass the platform type down. This allows us to clean up a lot of function signatures. This partially reverts commit f9b2f3d.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 3, 2023

openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 3, 2023

openshift-ci bot requested review from javipolo and ori-amizur July 3, 2023 13:18

zaneb force-pushed the mgmt-15150 branch from 209c528 to 50763c2 Compare July 4, 2023 00:13

zaneb added 2 commits July 6, 2023 17:03

Remove platform argument when extracting installer

e73b9c5

Since we no longer use different installer binaries for different platforms, we no longer need to pass the platform type down. This allows us to clean up a lot of function signatures. This partially reverts commit f9b2f3d.

zaneb force-pushed the mgmt-15150 branch from 50763c2 to e73b9c5 Compare July 6, 2023 05:09

eranco74 reviewed Jul 9, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 9, 2023

openshift-ci bot requested a review from danielerez July 9, 2023 10:00

openshift-ci bot assigned avishayt Jul 9, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2023

danielerez reviewed Jul 9, 2023

View reviewed changes

openshift-merge-robot merged commit 820a271 into openshift:master Jul 9, 2023
15 checks passed

zaneb mentioned this pull request Jul 9, 2023

[release-4.13] MGMT-15150: Use same installer binary for all platform types #5346

Merged

CrystalChun mentioned this pull request Aug 2, 2023

[release-ocm-2.8] MGMT-15413: Use same installer binary for all platform types (#5334) #5408

Merged

20 tasks

adriengentil mentioned this pull request Sep 14, 2023

cherry pick MGMT 15047 #5489

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MGMT-15150: Use same installer binary for all platform types #5334

MGMT-15150: Use same installer binary for all platform types #5334

zaneb commented Jul 3, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Jul 3, 2023 •

edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Jul 3, 2023 •

edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

codecov bot commented Jul 3, 2023 •

edited

eranco74 commented Jul 9, 2023

openshift-ci bot commented Jul 9, 2023

eranco74 commented Jul 9, 2023

eranco74 left a comment

openshift-ci bot commented Jul 9, 2023

eranco74 commented Jul 9, 2023

avishayt commented Jul 9, 2023

danielerez Jul 9, 2023

zaneb Jul 9, 2023

openshift-ci bot commented Jul 9, 2023

filanov commented Aug 1, 2023

openshift-cherrypick-robot commented Aug 1, 2023

MGMT-15150: Use same installer binary for all platform types #5334

MGMT-15150: Use same installer binary for all platform types #5334

Conversation

zaneb commented Jul 3, 2023 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Jul 3, 2023 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Jul 3, 2023 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

codecov bot commented Jul 3, 2023 • edited

Codecov Report

eranco74 commented Jul 9, 2023

openshift-ci bot commented Jul 9, 2023

eranco74 commented Jul 9, 2023

eranco74 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 9, 2023

eranco74 commented Jul 9, 2023

avishayt commented Jul 9, 2023

danielerez Jul 9, 2023

Choose a reason for hiding this comment

zaneb Jul 9, 2023

Choose a reason for hiding this comment

openshift-ci bot commented Jul 9, 2023

filanov commented Aug 1, 2023

openshift-cherrypick-robot commented Aug 1, 2023

zaneb commented Jul 3, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Jul 3, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Jul 3, 2023 •

edited by openshift-ci bot

codecov bot commented Jul 3, 2023 •

edited