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

SLES import: Support conversion to on-demand #1368

Merged
merged 3 commits into from Oct 6, 2020

Conversation

EricEdens
Copy link
Contributor

This PR adds:

  • Support for importing on-demand SLES and SLES for SAP guests. The imported guest is not required to have an active SLES subscription.

OS targets

Eight targets are available for the --os flag, covering all combinations of:

  • Major version: 12, 15
  • Flavor: SLES, SLES for SAP
  • Subscription: BYOL, convert to on-demand

See cli_tools/common/utils/daisy/daisy_utils.go for the new targets.

Noteworthy changes

  • The on-demand conversion runs in a chroot. This is required since SLES's registration code
    fingerprints the instance. Running in a guestfs appliance fails registration.
  • Similarly, to support this fingerprinting, the Debian 9 import worker includes
    the license that will be applied to the final image.

Testing:

  1. Added e2e tests:
  • Each minor version has an unregistered image that is imported and verified (8 total)
  • Each major version has one registered image that is imported as BYOL (4 total)
  1. Ran SLES and OpenSUSE tests from ci-daisy-e2e

@google-oss-robot
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@EricEdens
Copy link
Contributor Author

/test all

@dntczdx
Copy link
Contributor

dntczdx commented Sep 22, 2020

There are many "no changes" file included, is it by accident?

@EricEdens
Copy link
Contributor Author

There are many "no changes" file included, is it by accident?

Good question -- the message means that it's an empty file. These occur for two reasons:

  1. init.py: These are for Python's package search: https://docs.python.org/3/tutorial/modules.html
  2. Some files in the test-data directory are empty. They're used when testing check_root, which in some cases is only looking for a file to be present.

Copy link
Contributor

@dntczdx dntczdx left a comment

Choose a reason for hiding this comment

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

Overall lgtm, e2e test is the real gate for the correctness

daisy_workflows/image_import/suse/run-translate.sh Outdated Show resolved Hide resolved
Comment on lines 25 to 26
If SUSE releases a new version of tarballs, copy them to a new `{timestamp}`
directory, and update the tarball metadata in `translate.py`.
Copy link
Contributor

Choose a reason for hiding this comment

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

To whom is this instruction addressed? Us?

Do we have a notification system in place for new versions of tarballs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Future maintainers is the intended audience. The updated README gives clearer instructions.

Regarding the notification system, my current thinking is that our e2e tests have sufficient coverage to detect problems with on-demand conversion. All minor versions, and all flavors are covered.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we chatted about it: maybe add instructions if those tests fail so we know what to do?

ff02::2 ip6-allrouters

10.128.0.14 inst-translator-import-image-946jb.c.edens-test.internal inst-translator-import-image-946jb # Added by Google
169.254.169.254 metadata.google.internal # Added by Google
Copy link
Contributor

Choose a reason for hiding this comment

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

Github is showing a missing new line in this file. Not sure it's an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with a trailing newline (I copied the file directly from a debian 9 worker instance).

@EricEdens
Copy link
Contributor Author

@zoran15 -- Added a debugging tip when the on-demand import tests fail (talked about this offline)

@EricEdens EricEdens requested a review from zoran15 October 1, 2020 19:26
@google-oss-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dntczdx, EricEdens, zoran15

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 [EricEdens,dntczdx,zoran15]

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

@google-oss-robot
Copy link
Collaborator

New changes are detected. LGTM label has been removed.

@EricEdens EricEdens merged commit dc64ab8 into GoogleCloudPlatform:master Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants