MGMT-10209: Pass in icsp file to be used for 'oc adm release extract' #4115

bfournie · 2022-07-11T17:12:21Z

Pass in an ICSP file to the 'oc' command to extract the installer. The ICSP file is created using the ImageContentSources in the install-config.

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

Assignees

/cc @
/cc @

Checklist

Title and description added to both, commit and PR.
Relevant issues have been associated (see CONTRIBUTING guide)
Reviewers have been listed
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?

eranco74

This ICSP configuration is used solely for extracting the installer binary from the release image in the assisted-service pod (running on the HUB cluster).
I think that the HUB cluster should have the ICSP (I think it's even mounted inside the pod)
so we probably would get it from there.
@avishayt perhaps we should define an API for installing from mirror registry and allow users to provide ICSP (currently they need to use install-config override)

eranco74 · 2022-07-11T17:33:19Z

pkg/generator/generator.go

@@ -32,6 +32,7 @@ type Config struct {
 	ReleaseImageMirror string
 	DummyIgnition      bool   `envconfig:"DUMMY_IGNITION"`
 	InstallInvoker     string `envconfig:"INSTALL_INVOKER" default:"assisted-installer"`
+	IcspFile           string `envconfig:"ICSP_FILE" default:""`


Who should set this env?
What should be the content of IcspFile is it the imageContentSourcePolicy content or file path?

This will be set when running BILLI

Yes this will be set when running BLLI, it will be the file path of a mounted ICSP API file.

We are currently passing into to the assisted-service container the registries.conf file, mounted in /etc/containers, and this is used here to set up the ImageContentSources in the install-config. We could also use that to build up an ImageContenSourcePolicy API to pass into the 'oc' command if that is the preferred way to go.

This should also be set in MCE. Doing this is a pre-requisite to switching to the official oc command instead of using a fork.
BILLI is the first user only because it has to use official builds.

avishayt · 2022-07-11T17:45:16Z

If linking to github in the commit message, please use a permalink

avishayt · 2022-07-11T17:47:59Z

This ICSP configuration is used solely for extracting the installer binary from the release image in the assisted-service pod (running on the HUB cluster). I think that the HUB cluster should have the ICSP (I think it's even mounted inside the pod) so we probably would get it from there. @avishayt perhaps we should define an API for installing from mirror registry and allow users to provide ICSP (currently they need to use install-config override)

We can consider adding it as an API, though it would probably be more useful in the k8s API which isn't being used by BILLI.
Why is this PR needed if BILLI can just use install-config override?

codecov · 2022-07-11T17:49:34Z

Codecov Report

Merging #4115 (b21f7e9) into master (960e50e) will increase coverage by 0.38%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master    #4115      +/-   ##
==========================================
+ Coverage   65.28%   65.66%   +0.38%     
==========================================
  Files         186      186              
  Lines       26342    26887     +545     
==========================================
+ Hits        17197    17655     +458     
- Misses       7567     7627      +60     
- Partials     1578     1605      +27

Impacted Files	Coverage Δ
internal/ignition/ignition.go	`57.27% <56.86%> (-0.64%)`	⬇️
internal/oc/release.go	`70.51% <77.77%> (-0.54%)`	⬇️
internal/host/hostcommands/install_cmd.go	`80.88% <0.00%> (-0.46%)`	⬇️
internal/common/test_configuration.go	`0.00% <0.00%> (ø)`
internal/bminventory/inventory.go	`69.81% <0.00%> (+0.01%)`	⬆️
internal/bminventory/inventory_v2_handlers.go	`62.63% <0.00%> (+2.77%)`	⬆️
...nal/controller/controllers/bmh_agent_controller.go	`70.23% <0.00%> (+3.16%)`	⬆️
internal/host/host.go	`81.41% <0.00%> (+7.39%)`	⬆️

bfournie · 2022-07-11T18:27:57Z

If linking to github in the commit message, please use a permalink

Fixed

Pass in an ICSP file via an environment variable to be used when running the oc command to extract the installer. Note that the ImageContentSources are also in the generated install-config https://github.com/openshift/assisted-service/blob/master/internal/installcfg/installcfg.go#L113. This PR allows the ICSP to be passed in, in the API format that the oc command expects.

bfournie · 2022-07-11T19:41:28Z

Why is this PR needed if BILLI can just use install-config override?

I'm not sure I understand the question. We weren't planning on doing a install-config override from BLLI, but even so I don't think that would help in passing the ICSP to 'oc'.

zaneb

My main question is whether it'd be possible to generate the ICSP from the /etc/containers/registries.conf file and write it to a tempfile. That would save having to configure it in two places, and possibly eliminate a lot of the boilerplate here.
Talk of adding an API for this makes it unclear whether that would be flexible enough for all use cases though.

zaneb · 2022-07-11T19:38:02Z

pkg/generator/generator.go

@@ -32,6 +32,7 @@ type Config struct {
 	ReleaseImageMirror string
 	DummyIgnition      bool   `envconfig:"DUMMY_IGNITION"`
 	InstallInvoker     string `envconfig:"INSTALL_INVOKER" default:"assisted-installer"`
+	IcspFile           string `envconfig:"ICSP_FILE" default:""`


This should also be set in MCE. Doing this is a pre-requisite to switching to the official oc command instead of using a fork.
BILLI is the first user only because it has to use official builds.

internal/ignition/ignition.go

eranco74

looks good, just add few logging

eranco74 · 2022-07-14T07:40:04Z

internal/ignition/ignition.go

+	if err != nil {
+		return "", err
+	}
+	if _, err := icspFile.Write(contents); err != nil {


Please add logging of the ICSP content.
It should allow us to debug issues in case something goes wrong

Good idea, added.

eranco74 · 2022-07-14T07:42:01Z

internal/ignition/ignition.go

+	}
+
+	if len(installCfg.ImageContentSources) == 0 {
+		// No ImageContentSources were defined


Consider logging here

Added logging.

zaneb · 2022-07-14T16:40:09Z

internal/ignition/ignition.go

+		return "", err
+	}
+	if contents == nil {
+		log.Infof("No ImageContentsSources in install-config to build ICSP file")


Need to pass log in to this function.

also typo in ImageContentSources

zaneb · 2022-07-14T17:08:37Z

/lgtm

zaneb · 2022-07-14T18:36:32Z

/lgtm

eranco74 · 2022-07-14T18:40:13Z

/test ?

openshift-ci · 2022-07-14T18:40:27Z

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

/test edge-ci-index
/test edge-e2e-ai-operator-ztp
/test edge-e2e-ai-operator-ztp-converged
/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-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 edge-e2e-ai-operator-ztp-3masters
/test edge-e2e-ai-operator-ztp-capi
/test edge-e2e-ai-operator-ztp-disconnected
/test edge-e2e-ai-operator-ztp-ipv4v6-3masters-ocp-410
/test edge-e2e-ai-operator-ztp-ipv4v6-3masters-ocp-49
/test edge-e2e-ai-operator-ztp-ipv4v6-sno-ocp-410
/test edge-e2e-ai-operator-ztp-ipv4v6-sno-ocp-49
/test edge-e2e-metal-assisted-4-10
/test edge-e2e-metal-assisted-4-11
/test edge-e2e-metal-assisted-4-8
/test edge-e2e-metal-assisted-4-9
/test edge-e2e-metal-assisted-capi
/test edge-e2e-metal-assisted-cnv
/test edge-e2e-metal-assisted-day2
/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-none
/test edge-e2e-metal-assisted-ocs
/test edge-e2e-metal-assisted-odf
/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-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-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 · 2022-07-14T18:41:44Z

/test edge-e2e-ai-operator-ztp-disconnected
/hold
let's make sure it's still working

eranco74 · 2022-07-14T18:41:58Z

/lgtm

eranco74 · 2022-07-14T19:14:47Z

/test edge-e2e-ai-operator-ztp-disconnected

zaneb · 2022-07-14T19:32:20Z

Need openshift/release#30488 to fix the tests.

zaneb · 2022-07-14T19:59:18Z

/test edge-e2e-ai-operator-ztp-disconnected
/test edge-e2e-ai-operator-ztp

zaneb · 2022-07-14T21:12:04Z

Even after openshift/release#30488, IPv6 dev-scripts jobs are still broken by https://bugzilla.redhat.com/show_bug.cgi?id=2087096

zaneb · 2022-07-15T14:49:18Z

Hopefully openshift/release#30508 should fix IPv6.
/test edge-e2e-ai-operator-ztp-disconnected

zaneb · 2022-07-15T17:37:45Z

W00t!
/hold cancel
/cherry-pick release-4.11

openshift-cherrypick-robot · 2022-07-15T17:37:46Z

@zaneb: once the present PR merges, I will cherry-pick it on top of release-4.11 in a new PR and assign it to you.

In response to this:

W00t!
/hold cancel
/cherry-pick release-4.11

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.

pawanpinjarkar

/approve

openshift-ci · 2022-07-15T17:42:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bfournie, pawanpinjarkar

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~~ [pawanpinjarkar]

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

openshift-ci · 2022-07-15T20:28:30Z

@bfournie: 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.

openshift-cherrypick-robot · 2022-07-15T20:35:02Z

@zaneb: new pull request created: #4141

In response to this:

W00t!
/hold cancel
/cherry-pick release-4.11

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#4115) * MGMT-10209: Pass in icsp file to be used for 'oc adm release extract' Pass in an ICSP file via an environment variable to be used when running the oc command to extract the installer. Note that the ImageContentSources are also in the generated install-config https://github.com/openshift/assisted-service/blob/master/internal/installcfg/installcfg.go#L113. This PR allows the ICSP to be passed in, in the API format that the oc command expects. * generate mockRelease * Get the ImageContentSources for icsp file from install-config * Fixes for defer * added logging and unit tests * fixes for logging * fixes for logging * fixes for logging * fixes for logging * revert PR 3700 and get latest oc image * revert PR 3700 and get latest oc image

openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 11, 2022

openshift-ci bot requested review from empovit and nmagnezi July 11, 2022 17:13

eranco74 reviewed Jul 11, 2022

View reviewed changes

bfournie force-pushed the icsp-file-for-extract branch from 81a7c75 to b342025 Compare July 11, 2022 18:30

zaneb reviewed Jul 11, 2022

View reviewed changes

bfournie added 2 commits July 11, 2022 17:48

generate mockRelease

caaf1b4

Get the ImageContentSources for icsp file from install-config

f71d515

bfournie changed the title ~~MGMT-10209: Pass in icsp file to be used for 'oc adm release extract'~~ [WIP] MGMT-10209: Pass in icsp file to be used for 'oc adm release extract' Jul 13, 2022

zaneb reviewed Jul 13, 2022

View reviewed changes

internal/ignition/ignition.go Show resolved Hide resolved

internal/ignition/ignition.go Outdated Show resolved Hide resolved

internal/ignition/ignition.go Show resolved Hide resolved

Fixes for defer

8c90167

eranco74 reviewed Jul 14, 2022

View reviewed changes

added logging and unit tests

a5f7054

bfournie changed the title ~~[WIP] MGMT-10209: Pass in icsp file to be used for 'oc adm release extract'~~ MGMT-10209: Pass in icsp file to be used for 'oc adm release extract' Jul 14, 2022

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 Jul 14, 2022

zaneb reviewed Jul 14, 2022

View reviewed changes

bfournie added 3 commits July 14, 2022 12:54

fixes for logging

9618453

fixes for logging

54e72f3

fixes for logging

bea6a97

openshift-ci bot assigned zaneb Jul 14, 2022

bfournie added 2 commits July 14, 2022 13:55

revert PR 3700 and get latest oc image

a0862e8

revert PR 3700 and get latest oc image

b21f7e9

openshift-ci bot added the kind/dependency-change Categorizes issue or PR as related to changing dependencies label Jul 14, 2022

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

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 14, 2022

openshift-ci bot assigned eranco74 Jul 14, 2022

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2022

pawanpinjarkar approved these changes Jul 15, 2022

View reviewed changes

openshift-ci bot assigned pawanpinjarkar Jul 15, 2022

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

openshift-ci bot merged commit 43cc033 into openshift:master Jul 15, 2022

openshift-cherrypick-robot mentioned this pull request Jul 15, 2022

[release-4.11] MGMT-10209: Pass in icsp file to be used for 'oc adm release extract' #4141

Merged

bfournie deleted the icsp-file-for-extract branch December 8, 2023 13:49

MGMT-10209: Pass in icsp file to be used for 'oc adm release extract' #4115

MGMT-10209: Pass in icsp file to be used for 'oc adm release extract' #4115

Conversation

bfournie commented Jul 11, 2022 • edited

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Assignees

Checklist

Reviewers Checklist

eranco74 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

bfournie Jul 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avishayt commented Jul 11, 2022

avishayt commented Jul 11, 2022

codecov bot commented Jul 11, 2022 • edited

Codecov Report

bfournie commented Jul 11, 2022

bfournie commented Jul 11, 2022

zaneb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eranco74 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

zaneb commented Jul 14, 2022

zaneb commented Jul 14, 2022

eranco74 commented Jul 14, 2022

openshift-ci bot commented Jul 14, 2022

eranco74 commented Jul 14, 2022

eranco74 commented Jul 14, 2022

eranco74 commented Jul 14, 2022

zaneb commented Jul 14, 2022

zaneb commented Jul 14, 2022

zaneb commented Jul 14, 2022

zaneb commented Jul 15, 2022

zaneb commented Jul 15, 2022

openshift-cherrypick-robot commented Jul 15, 2022

pawanpinjarkar left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 15, 2022

openshift-ci bot commented Jul 15, 2022

openshift-cherrypick-robot commented Jul 15, 2022

bfournie commented Jul 11, 2022 •

edited

bfournie Jul 11, 2022 •

edited

codecov bot commented Jul 11, 2022 •

edited