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.

Sign up for GitHub

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

Jump to bottom

MGMT-2977 Cluster specific minimal iso #932

Merged

openshift-merge-robot merged 10 commits into openshift:master from carbonin:cluster_specific_minimal_iso

Jan 25, 2021

Member

carbonin commented Jan 14, 2021 •

edited

Loading

This PR allows a user to select a "minimal" iso for discovery which does not contain a rootfs. This reduces the iso size to 93M from 876M. This is required for users where the virtual media interface is significantly slower than the booted machine's network.

See coreos/fedora-coreos-tracker#661 for a larger discussion

This PR customizes the "template" minimal iso which was created in #911 by downloading the template iso, unpacking it locally, adding the ignition, and recreating the iso. A user specifies they want the minimal iso using the new API parameter (image_type) when requesting an image be generated. Installation then proceeds as it would otherwise.

Requires ~~#930 and #931~~ MERGED

~~TODO: add tests~~ Done, thanks @danielerez

openshift-ci-robot added do-not-merge/work-in-progress api-review labels

openshift-ci-robot requested review from omertuc and tsorya

January 14, 2021 20:37

openshift-ci-robot added the approved label

carbonin mentioned this pull request

[WIP] MGMT-2977 Create cluster-specific minimal iso #926

Closed

carbonin force-pushed the cluster_specific_minimal_iso branch from 6adff31 to eaf06aa Compare

January 15, 2021 14:23

openshift-ci-robot added the needs-rebase label

carbonin changed the title ~~[WIP] Cluster specific minimal iso~~ [WIP] MGMT-2977 Cluster specific minimal iso

carbonin commented

View reviewed changes

internal/bminventory/inventory_test.go Outdated Show resolved Hide resolved

danielerez force-pushed the cluster_specific_minimal_iso branch 2 times, most recently from 5e209c2 to f306878 Compare

January 20, 2021 10:41

openshift-ci-robot removed the needs-rebase label

carbonin changed the title ~~[WIP] MGMT-2977 Cluster specific minimal iso~~ MGMT-2977 Cluster specific minimal iso

openshift-ci-robot removed the do-not-merge/work-in-progress label

carbonin mentioned this pull request

MGMT-3776 added ISO_IMAGE_TYPE env var openshift/assisted-test-infra#431

Merged

Contributor

danielerez commented Jan 20, 2021

@avishayt Can you please take a look?

avishayt reviewed

View reviewed changes

swagger.yaml

Comment on lines +2925 to +2929

+                      type: string
+                      description: Type of image that should be generated.
+                      $ref: '#/definitions/image_type'

Contributor

avishayt Jan 20, 2021

I'm not sure it's legal to have a string with a ref?

Contributor

danielerez Jan 21, 2021 •

edited

Loading

Seems it behaves as expected, works with 'host-stage' at least :)

assisted-service/swagger.yaml

Lines 3792 to 3794 in f557006

    
           current_stage: 
        
             type: string 
        
             $ref: '#/definitions/host-stage'

danielerez force-pushed the cluster_specific_minimal_iso branch from f306878 to 0942ead Compare

January 21, 2021 13:42

avishayt reviewed

View reviewed changes

Contributor

avishayt left a comment

looks good, just one small comment

pkg/s3wrapper/upload_iso.go Outdated

-              	}
-              	if err := gzipWriter.Close(); err != nil {
-              		err = errors.Wrapf(err, "Failed to gzip ignition config")
+              	imageBytes, err := isoeditor.IgnitionImageArchive(ignitionConfig)

Contributor

avishayt Jan 24, 2021

Suggested change

      
            	imageBytes, err := isoeditor.IgnitionImageArchive(ignitionConfig)
          
            	ignitionBytes, err := isoeditor.IgnitionImageArchive(ignitionConfig)

Contributor

danielerez Jan 24, 2021

sure, done

carbonin and others added 8 commits

January 24, 2021 10:32


          MGMT-297 Add discovery image type

7df42b6

This adds a type enum to both the cluster image info and the
image creation parameters


          MGMT-2977 Move tempfile handling into create()

ceaa0cb


          MGMT-2977 Move logic for creating ignition archive to isoeditor

9273adf

This will make it easier to share between the existing logic
which creates the archive then uploads it to s3 and the new logic
which will write the archive to the fs before bundling it into the
minimal iso directly


          MGMT-2977 Add the full ignition to the iso before re-creating it

b2e191e

This allows us to create the entire iso locally and skip the process
of uploading the ignition to S3


          MGMT-2977 Create a cluster-specific minimal iso when requested

c3baae1


          MGMT-2977 Create a factory interface for isoeditor

ba9d4ca

This will be a good place to add "ratelimiting" for concurrent iso
editing if that's something we need.

Additionally it makes things easier to test by giving us somewhere
to inject mock objects at the inventory API level.

Authored-by: Nick Carboni <ncarboni@redhat.com>


          MGMT-2977 extract generate cluster minimal iso logic

25ef4ef

Extracted generate cluster minimal ISO logic to 'generateClusterMinimalISO'
function in order to avoid a cyclomatic complexity error.


          MGMT-2977 negative unit-tests for generate minimal iso

danielerez force-pushed the cluster_specific_minimal_iso branch from 0942ead to 5f447d9 Compare

January 24, 2021 09:01

avishayt approved these changes

View reviewed changes

openshift-ci-robot assigned avishayt

openshift-ci-robot added the lgtm label

openshift-ci-robot commented Jan 24, 2021

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [avishayt,carbonin]

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

Contributor

openshift-bot commented Jan 24, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

12 similar comments

Contributor

openshift-bot commented Jan 24, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Jan 24, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Jan 24, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Jan 24, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Jan 24, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Jan 24, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Jan 24, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Jan 24, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Jan 24, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Jan 24, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Jan 24, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Jan 24, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

danielerez added 2 commits

January 24, 2021 18:49


          MGMT-2977 unit-test for regenerating iso template

ff8e091

Added "Regenerates the iso for a new type" unit-test.


          MGMT-2977 added minimal-iso ImageType to subsystem tests

a535916

danielerez force-pushed the cluster_specific_minimal_iso branch from 5f447d9 to a535916 Compare

January 24, 2021 16:52

openshift-ci-robot removed the lgtm label

Contributor

danielerez commented Jan 25, 2021

/retest

Contributor

danielerez commented Jan 25, 2021

/lgtm

openshift-ci-robot assigned danielerez

openshift-ci-robot added the lgtm label

openshift-merge-robot merged commit 5514de2 into openshift:master

carbonin deleted the cluster_specific_minimal_iso branch

January 25, 2021 15:01

osherdp pushed a commit to osherdp/assisted-service that referenced this pull request


          MGMT-2977 Cluster specific minimal iso (openshift#932)

e68d4d3

* MGMT-297 Add discovery image type

This adds a type enum to both the cluster image info and the
image creation parameters

* MGMT-2977 Move tempfile handling into create()

* MGMT-2977 Move logic for creating ignition archive to isoeditor

This will make it easier to share between the existing logic
which creates the archive then uploads it to s3 and the new logic
which will write the archive to the fs before bundling it into the
minimal iso directly

* MGMT-2977 Add the full ignition to the iso before re-creating it

This allows us to create the entire iso locally and skip the process
of uploading the ignition to S3

* MGMT-2977 Create a cluster-specific minimal iso when requested

* MGMT-2977 Create a factory interface for isoeditor

This will be a good place to add "ratelimiting" for concurrent iso
editing if that's something we need.

Additionally it makes things easier to test by giving us somewhere
to inject mock objects at the inventory API level.

Authored-by: Nick Carboni <ncarboni@redhat.com>

* MGMT-2977 extract generate cluster minimal iso logic

Extracted generate cluster minimal ISO logic to 'generateClusterMinimalISO'
function in order to avoid a cyclomatic complexity error.

* MGMT-2977 negative unit-tests for generate minimal iso

* MGMT-2977 unit-test for regenerating iso template

Added "Regenerates the iso for a new type" unit-test.

* MGMT-2977 added minimal-iso ImageType to subsystem tests

Co-authored-by: Daniel Erez <derez@redhat.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-review approved lgtm