Bug 1806143: OpenStack: start using image import when possible #3162

Fedosin · 2020-02-22T12:34:38Z

Now we use legacy image uploading that doesn't support image conversion. It leads to the fact that we upload qcow2 images to Ceph backend, which doesn't support this format:
https://access.redhat.com/solutions/2434691

By using the Image Import mechanism we make sure that all uploaded images will be automatically converted to Raw for Ceph (and only for Ceph) backends.

If the image import mechanism is not available we fallback to the legacy uploading.

openshift-ci-robot · 2020-02-22T12:34:43Z

@Fedosin: This pull request references Bugzilla bug 1806143, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1806143: OpenStack: start using image import when possible

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.

Fedosin · 2020-02-22T14:55:43Z

There are some CI issues, the code itself works well:
level=debug msg="Creating a Glance image for RHCOS..."
level=debug msg="Image 5y1xwbnp-1bfcb-tdvkm-rhcos was created."
level=debug msg="Checking if the image import mechanism is supported"
level=debug msg="Using legacy API to upload RHCOS to the image 5y1xwbnp-1bfcb-tdvkm-rhcos with ID fcc8d3d5-6294-4bf4-bc89-e4f3656f2994"
level=debug msg="The data was uploaded."

Fedosin · 2020-02-22T16:07:55Z

/retest

mandre · 2020-02-23T11:08:44Z

data/data/openstack/variables-openstack.tf

@@ -15,12 +15,6 @@ variable "openstack_base_image_name" {
  description = "Name of the base image to use for the nodes."


We no longer need the openstack_base_image_name variable it seems.

I was going to remove it too, but then I realized that we also need to support overridden image names (which are not created by the installer). It means that we have to to provide these names to Terraform somehow, and openstack_base_image_name is the only way to do it.

Oh yeah, of course. I didn't notice it was still used on line 79 👍

mandre · 2020-02-23T11:15:52Z

pkg/tfvars/openstack/rhcos_image.go

+	imageCreateOpts := images.CreateOpts{
+		Name:            imageName,
+		ContainerFormat: "bare",
+		DiskFormat:      "raw",


Shouldn't DiskFormat depends on the actual image type rather than hardcoding it to raw?

I find it weird that we need to set this when importing an image, because the whole point is to let glance decide what it the best image format for the backend...

Yeah, it should be qcow2 here for sure. The image conversion plugin will change it if necessary upon image activation.

mandre · 2020-02-23T11:20:31Z

pkg/tfvars/openstack/rhcos_image.go

+
+	useImageImport, err := isImageImportSupported(cloud)
+	if err != nil {
+		return err


Error checking whether glance supports image import should result is us assuming it does not rather than failing the image upload.

I check what type of error Glance returns. If this is 404 because of the lack of the Discovery API, then isImageImportSupported ignores the error and returns false, nil. The function returns only real errors, for example, when Glance service is not available. In this case it's correct to stop the execution.

The error could be anything, a 5xx because glance discovery API is broken, networking issue, ...
My point being, a failure in isImageImportSupported() is recoverable and should not stop execution: we can assume the cloud does not support glance direct image import.

Failure of glance image discovery at time T doesn't imply failure of glance image upload at time T+1. We're catching error of image upload, aren't we?

I don't really like this decision. It leads to unpredictable system behavior when, for example, due to a short network failure, the installer believes that the image import is unavailable, but it is not.
As for me, so if Glance returns an error, we should inform the user about it, and do not hide it.

There might be a higher-level concern on resilience, but I am unaware of that; and by default I am all for Mike's explicit error.

If I'm not mistaken, the rationale for using image-import is to enable the upload of images in cases where the legacy upload would fail.

Thinking of this user story (legacy method fails), the user would probably be surprised of seeing that the upload fails despite his cluster having image-upload capability.

I'd really fail here with an explicit error, and let them try again from scratch.

As an alternative, I'd consider removing the check and always uploading with image-import, and noisily falling back to the legacy upload in case of failure.

mandre · 2020-02-23T11:26:39Z

pkg/tfvars/openstack/rhcos_image.go

+		return err
+	}
+
+	if useImageImport {


Could you find a cloud with glance direct image import to test this?

I tested it on manila-env-like devstack with Glance and Ceph... This is not really cool, I know. We'll need to obtain a real OSP13 env, where we can test the solution properly.

pkg/tfvars/openstack/rhcos_image.go

pierreprinetti · 2020-02-25T09:21:57Z

pkg/tfvars/openstack/rhcos_image.go

+	}
+
+	// Next two checks are just to make sure the response data was not corrupted
+	if s.ImportMethods.Description != "Import methods available." {


I like the check just below this; so much so, that this check on "description" looks redundant. Moreover, relying on a strict check of a descriptive field looks brittle to me.

I'd remove the check on "Description", unless there's some additional reason for keeping it

Agree that there is no need to have this check. I added it because this is a constant string and it's a part of the API https://docs.openstack.org/api-ref/image/v2/?expanded=import-methods-and-values-discovery-detail#image-service-info-discovery

pierreprinetti · 2020-02-25T09:34:04Z

pkg/tfvars/openstack/rhcos_image.go

+			return true, nil
+		}
+	}
+


what about:

logrus.Debugln("Glance Direct image import plugin was not found")
?

pierreprinetti · 2020-02-25T09:35:43Z

pkg/tfvars/openstack/rhcos_image.go

+	}
+
+	if useImageImport {
+		logrus.Debugf("Using Image Import API to upload RHCOS to the image %v with ID %v", img.Name, img.ID)


nit: you could use %q instead of %v, as they're both strings

pierreprinetti · 2020-02-25T10:32:51Z

/approve

pierreprinetti · 2020-02-25T10:34:16Z

oh Gophercloud's version bump requires an installer-approver

sdodson · 2020-02-25T16:11:03Z

/approve
/cc @hardys
PTAL at the gophercloud bump, metal3 seems to be the only other consumer of that library.

openshift-ci-robot · 2020-02-25T16:14:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pierreprinetti, sdodson

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

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

Now we use Terraform to create images in Glance. This commit starts using direct Gophercloud calls for this.

pierreprinetti · 2020-02-27T15:42:16Z

/lgtm

@mandre
I am LGTMing this because as far as my understanding reaches, the patch looks fine.
Please let's circle back to the error handling discussion once you're back!

/label platform/openstack

openshift-bot · 2020-02-27T15:52:49Z

/retest

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

openshift-ci-robot · 2020-02-27T17:39:53Z

@Fedosin: All pull requests linked via external trackers have merged. Bugzilla bug 1806143 has been moved to the MODIFIED state.

In response to this:

Bug 1806143: OpenStack: start using image import when possible

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-ci-robot · 2020-02-27T18:38:39Z

@Fedosin: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-libvirt	`0d92968`	link	`/test e2e-libvirt`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

Fedosin · 2020-03-16T09:26:45Z

/cherry-pick release-4.4

openshift-cherrypick-robot · 2020-03-16T09:26:55Z

@Fedosin: #3162 failed to apply on top of branch "release-4.4":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	data/data/openstack/main.tf
M	data/data/openstack/variables-openstack.tf
Falling back to patching base and 3-way merge...
Auto-merging data/data/openstack/variables-openstack.tf
Auto-merging data/data/openstack/main.tf
CONFLICT (content): Merge conflict in data/data/openstack/main.tf
Patch failed at 0001 Upload RHCOS images directly with Gophercloud

In response to this:

/cherry-pick release-4.4

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-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 22, 2020

openshift-ci-robot requested review from jhixson74 and patrickdillon February 22, 2020 12:35

mandre reviewed Feb 23, 2020

View reviewed changes

Fedosin force-pushed the image_import branch from 9b67d7f to 54889be Compare February 23, 2020 23:21

pierreprinetti reviewed Feb 25, 2020

View reviewed changes

openshift-ci-robot requested a review from hardys February 25, 2020 16:11

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 25, 2020

Fedosin added 4 commits February 26, 2020 14:02

Upload RHCOS images directly with Gophercloud

912e7b5

Now we use Terraform to create images in Glance. This commit starts using direct Gophercloud calls for this.

Bump gophercloud

6f7471e

Add a function to check if Image Import API is supported

72e7fab

Use Image Import mechanism when possible

0d92968

Fedosin force-pushed the image_import branch from 54889be to 0d92968 Compare February 26, 2020 13:28

openshift-ci-robot assigned pierreprinetti Feb 27, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. platform/openstack labels Feb 27, 2020

openshift-merge-robot merged commit ec87e64 into openshift:master Feb 27, 2020

Fedosin deleted the image_import branch March 16, 2020 09:26

Fedosin mentioned this pull request Mar 16, 2020

[release-4.4] Bug 1813851: OpenStack: start using image import when possible #3297

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1806143: OpenStack: start using image import when possible #3162

Bug 1806143: OpenStack: start using image import when possible #3162

Fedosin commented Feb 22, 2020

openshift-ci-robot commented Feb 22, 2020

Fedosin commented Feb 22, 2020

Fedosin commented Feb 22, 2020

mandre Feb 23, 2020

Fedosin Feb 23, 2020

mandre Feb 24, 2020

mandre Feb 23, 2020

Fedosin Feb 23, 2020

mandre Feb 23, 2020

Fedosin Feb 23, 2020

mandre Feb 24, 2020

Fedosin Feb 24, 2020

pierreprinetti Feb 25, 2020 •

edited

Loading

mandre Feb 23, 2020

Fedosin Feb 23, 2020

pierreprinetti Feb 25, 2020

Fedosin Feb 26, 2020

pierreprinetti Feb 25, 2020

Fedosin Feb 26, 2020

pierreprinetti Feb 25, 2020

Fedosin Feb 26, 2020

pierreprinetti commented Feb 25, 2020

pierreprinetti commented Feb 25, 2020

sdodson commented Feb 25, 2020

openshift-ci-robot commented Feb 25, 2020

pierreprinetti commented Feb 27, 2020

openshift-bot commented Feb 27, 2020

openshift-ci-robot commented Feb 27, 2020

openshift-ci-robot commented Feb 27, 2020

Fedosin commented Mar 16, 2020

openshift-cherrypick-robot commented Mar 16, 2020

		@@ -15,12 +15,6 @@ variable "openstack_base_image_name" {
		description = "Name of the base image to use for the nodes."

Bug 1806143: OpenStack: start using image import when possible #3162

Bug 1806143: OpenStack: start using image import when possible #3162

Conversation

Fedosin commented Feb 22, 2020

openshift-ci-robot commented Feb 22, 2020

Fedosin commented Feb 22, 2020

Fedosin commented Feb 22, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierreprinetti Feb 25, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierreprinetti commented Feb 25, 2020

pierreprinetti commented Feb 25, 2020

sdodson commented Feb 25, 2020

openshift-ci-robot commented Feb 25, 2020

pierreprinetti commented Feb 27, 2020

openshift-bot commented Feb 27, 2020

openshift-ci-robot commented Feb 27, 2020

openshift-ci-robot commented Feb 27, 2020

Fedosin commented Mar 16, 2020

openshift-cherrypick-robot commented Mar 16, 2020

pierreprinetti Feb 25, 2020 •

edited

Loading