Add nested support for GCP #3430

amorenoz · 2020-04-08T18:33:26Z

This is based on the a patch from @cgwalters. I just applied his patch and tweaked a bit the terraform variables file to avoid some errors.
I am not familiar with the project, so I cannot judge whether this implementation is acceptable or not. I can only testify that it does work.
So, this PR should be understood as me volunteering to rework this patch based on the feedback I receive.

Fixes #2649

amorenoz · 2020-04-08T18:34:14Z

cc @abhinavdahiya @mrunalp PTAL

openshift-ci-robot · 2020-04-08T18:34:55Z

Hi @amorenoz. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

abhinavdahiya · 2020-04-08T18:47:30Z

pkg/asset/cluster/tfvars.go

+		// See https://github.com/coreos/coreos-assembler/blob/master/doc/openshift-gcp-nested-virt.md
+		// and https://cloud.google.com/compute/docs/instances/enable-nested-virtualization-vm-instances
+		licenses := []string{}
+		if _, nestedvirt := os.LookupEnv("OPENSHIFT_INSTALL_OS_IMAGE_GCP_ENABLE_NESTED_VIRT"); nestedvirt {
+			licenses = append(licenses, "https://www.googleapis.com/compute/v1/projects/vm-options/global/licenses/enable-vmx")
+		}
+


using env to configure the list of licenses is not great!

Any alternative suggestions?

#3430 (comment)

cannot answer or suggest without details on ^^

would a dedicated field in the the customizable install-config.yaml be an acceptable approach?

abhinavdahiya · 2020-04-08T18:53:52Z

currently the installer creates the image, but in case it is forced to use an already exiting image, how would installer enforce this on an image that isn't enabled.

should users be allowed to turn it off, any reason some users might not like this default.

are there other URLs for licences that people would want to use. if yes, how would they use those, and if we allow users to specify a different list, how does that fit in with pre-existing images.

cgwalters · 2020-04-08T19:15:17Z

currently the installer creates the image, but in case it is forced to use an already exiting image, how would installer enforce this on an image that isn't enabled.

If someone is using the image override environment variable, they can figure this out on their own.

should users be allowed to turn it off, any reason some users might not like this default.

This is off by default.

are there other URLs for licences that people would want to use.

Not that I know of. When sketched out this patch I looked at that briefly and couldn't find any other "licenses" - it seems like they added something generic but nothing besides nested virt has used it yet.

But, in the (IMO unlikely) case that one appears and we want to use it...we could accept another environment variable.

I think it's OK too if we choose to break this interface later, as long as we give sufficient notice for users to adapt. Ideally, the only user is the openshift/release git repository at least to start.

But I dunno, we could call it OPENSHIFT_INSTALL_GCP_LICENSES and have it take comma/whitespace -separated values or so too.

if yes, how would they use those, and if we allow users to specify a different list, how does that fit in with pre-existing images.

If we get to the point where we have pre-existing images for RHCOS (which certainly may happen) then one of two things would need to happen: We change this code to copy it via Terraform, or we punt and ask users to do it.

The reason the installer needs to do it is really precisely because RHCOS is pinned in the installer and we don't have any public interface like openshift/enhancements#201 to do it...and I promise that will be implemented at some point but the benefit of supporting CNV and others using nested virt for CI in the meantime is substantial.

abhinavdahiya · 2020-04-09T03:07:42Z

I promise that will be implemented at some point but the benefit of supporting CNV and others using nested virt for CI in the meantime is substantial.

If this is being added for CI,

This doesn't have to be configured by installer during install-time. not every thing needs to be.
The CI jobs creates a nested virt image.
The CI job then creates new compute machinesets using the new nested virt image.

machinesets should allow the CI job to easily do this. and the new steps registry for CI jobs should make it really easy.

The original issue that requested to RFE isn't something a lot of the users are expecting to see, no a lot of traction, nor are we seeing this through JIRA RFEs. So adding ENV based configuraton is something i would like to avoid, and install-config based has difficult maintainability concerns later on when we mature our OS image handling..

amorenoz · 2020-04-14T08:07:32Z

I promise that will be implemented at some point but the benefit of supporting CNV and others using nested virt for CI in the meantime is substantial.

If this is being added for CI,
* This doesn't have to be configured by installer during install-time. not every thing needs to be.

* The CI jobs creates a nested virt image.

* The CI job then creates new compute machinesets using the new nested virt image.

This not for CI alone. We (i.e: the virt team trying to make Kata Containers work on OpenShift) would like to deliver a tech-preview version on GCP. So this would enable potential users to try nested GCP + Kata.

abhinavdahiya · 2020-04-14T17:58:36Z

#3430 (comment)

since we are expecting to bring it to users.. I think we need to,

drive this using the install-config
request for a list of license URLs
make sure we document that using these URLs will force the installer to copy the source_image before using them for cluster. currently this doesn't matter but it could matter soon therefore the field should have this documented.
also that setting this field with OS_OVERRIDE is not allowed.

amorenoz · 2020-04-14T18:21:32Z

OK @abhinavdahiya , I'll give it a spin

amorenoz · 2020-04-17T15:06:05Z

@abhinavdahiya PTAL

abhinavdahiya · 2020-04-17T15:35:31Z

pkg/asset/cluster/tfvars.go

+		licenses := []string{}
+		for _, license := range installConfig.Config.GCP.Licenses {
+			licenses = append(licenses, license)
+		}
+


why the copy? isn't it already a string slice?

You're right.

abhinavdahiya · 2020-04-17T15:38:30Z

pkg/types/gcp/platform.go

@@ -29,4 +29,9 @@ type Platform struct {
 	// The value should be the name of the subnet.
 	// +optional
 	ComputeSubnet string `json:"computeSubnet,omitempty"`
+
+	// Licenses is a list of licenses to apply to the compute images
+	// The value should a list of strings representing the license keys


add the requirement that these are URLs (https) only

add documentation that this field cannot be used to any kind of OS image override
^^ also update the docs to say that.. we don't want to enumerate the it's only the env, but generically all workflows like in future the clusterOSImage fields.

also add documentation that when set, this will cause the installer to copy the image into user's project.

looking at the docs https://cloud.google.com/compute/docs/reference/rest/v1/images/insert

licenses[] | string Any applicable license URI. Authorization requires the following IAM permission on the specified resource licenses: compute.licenseCodes.use

it seems like the user creating the image with a license needs to have certain permissions to use it. we should add that to docs/

there is also an API to get the license.. https://cloud.google.com/compute/docs/reference/rest/v1/licenses/get
we should add a validation to pkg/asset/installconfig/gcp/validations.go so that we can validate, user(here the installer with the creds given to it) can GET the license and the users are not setting invalid licenses..

Giving it a try, I think it might not be possible to validate the nested license through the compute.license API. Such API seems to be intended to inspect project-specific resources:
GET https://compute.googleapis.com/compute/v1/projects/{project}/global/licenses/{resourceId}

However, the nested license seems to live in a special internal "project" called "vm-options". That does not seem accessible through the API

https://www.googleapis.com/compute/v1/projects/vm-options/global/licenses/enable-vmx

Trying to access that resource from the API yields a permission error. And trying to grant permissions over this "vm-options" project also fails.
I have talked with GCP's tech support and they have confirmed this.

Should we still try to keep the option generic (string-based license URLs) or should we add a "enable-nested" knob?

@abhinavdahiya ^^

it looks like we can't validate if license is valid, so let's drop that specific validation.

pkg/types/gcp/validation/platform.go

abhinavdahiya · 2020-04-17T15:40:01Z

docs/user/gcp/customization.md

@@ -8,6 +8,7 @@ Beyond the [platform-agnostic `install-config.yaml` properties](../customization
 * `controlPlaneSubnet` (optional string): The name of an existing GCP subnet which should be used by the cluster control plane.
 * `computeSubnet` (optional string): The name of an existing GCP subnet which should be used by the cluster nodes.
 * `defaultMachinePlatform` (optional object): Default [GCP-specific machine pool properties](#machine-pools) which apply to [machine pools](../customization.md#machine-pools) that do not define their own GCP-specific properties.
+* `licenses` (optional list of strings): A list of license URLs that should be applied to the compute images. The use of this property in combination with OPENSHIFT_INSTALL_OS_IMAGE_OVERRIDE is forbidden. Also, note that use of these URLs will force the installer to copy the source image before being used.


this needs to include docs from gcp that show where to get these licenses..

abhinavdahiya · 2020-04-17T18:22:19Z

pkg/types/gcp/validation/platform.go

+	}
+
+	for i, license := range p.Licenses {
+		if !strings.HasPrefix(license, "https://") {


nit: use

installer/pkg/validate/validate.go

Lines 167 to 178 in 6decc16

// URIWithProtocol validates that the URI specifies a certain

// protocol scheme (e.g. "https")

func URIWithProtocol(uri string, protocol string) error {

parsed, err := url.Parse(uri)

if err != nil {

return err

}

if parsed.Scheme != protocol {

return fmt.Errorf("must use %s protocol", protocol)

}

return nil

}

abhinavdahiya · 2020-04-23T16:49:15Z

@amorenoz i think https://github.com/openshift/installer/pull/3430/files#r410395101 is the only remaining comment from me.

after that we can merge this as soon as we open for 4.6 development.

abhinavdahiya · 2020-04-23T16:49:30Z

/ok-to-test
/test e2e-gcp

Add an optional parameter in the GCP install-config that contains a list of license URLs to be added to the compute image Credits: Based on the work by Colin Walters Signed-off-by: Adrian Moreno <amorenoz@redhat.com>

abhinavdahiya

/lgtm

openshift-bot · 2020-05-08T23:35:43Z

/retest

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

cgwalters · 2020-05-08T23:40:23Z

Thanks a lot for driving this to completion! It will help us use OpenShift CI more for RHCOS too!

openshift-bot · 2020-05-09T00:27:47Z

/retest

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

openshift-bot · 2020-05-09T00:53:40Z

/retest

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

openshift-bot · 2020-05-09T01:19:39Z

/retest

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

openshift-bot · 2020-05-09T01:58:39Z

/retest

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

openshift-bot · 2020-05-09T02:11:43Z

/retest

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

openshift-bot · 2020-05-09T02:24:39Z

/retest

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

openshift-bot · 2020-05-09T03:05:05Z

/retest

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

cgwalters · 2020-05-09T13:23:40Z

Just to xref, this will obsolete coreos/coreos-assembler#748

dustymabe · 2020-05-22T22:23:51Z

On a semi related note in FCOS land we are going to try to switch to enabling nested virtualization by default on our images. Our representatives from GCP have indicated there shouldn't be any drawbacks for doing so: coreos/fedora-coreos-pipeline#242

If we switch to using already created images in GCP for openshift-installer we maybe should consider doing the same thing (switching to nested virt by default) for RHCOS.

cgwalters · 2020-05-26T20:38:48Z

Just to xref, in the future based on
coreos/fedora-coreos-pipeline#242 (comment)
we are likely to switch RHCOS+openshift-install to boot from a pre-created image with nested virt enabled by default.

dustymabe · 2020-05-26T21:05:27Z

and more details in coreos/fedora-coreos-pipeline#242 (comment)

openshift-bot · 2020-06-03T06:59:19Z

/retest

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

openshift-bot · 2020-06-03T07:12:43Z

/retest

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

openshift-bot · 2020-06-03T08:43:25Z

/retest

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

openshift-bot · 2020-06-03T08:56:01Z

/retest

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

openshift-bot · 2020-06-03T12:11:08Z

/retest

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

openshift-bot · 2020-06-03T12:50:05Z

/retest

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

openshift-bot · 2020-06-03T19:59:03Z

/retest

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

openshift-bot · 2020-06-03T20:25:14Z

/retest

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

openshift-bot · 2020-06-03T21:04:00Z

/retest

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

openshift-bot · 2020-06-03T22:09:12Z

/retest

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

openshift-bot · 2020-06-04T03:47:00Z

/retest

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

fabiand · 2020-06-04T09:06:16Z

Wohoo.

IS it already clear in which release this feature will land?

fabiand · 2020-06-04T11:00:20Z

Please ping me at the next conf for a cold beverage.

amorenoz · 2020-06-04T11:03:14Z

I think this should land on 4.6

openshift-ci-robot requested review from jhixson74 and jstuever April 8, 2020 18:33

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 8, 2020

abhinavdahiya reviewed Apr 8, 2020

View reviewed changes

amorenoz force-pushed the wip/gcp_nested branch from 9530949 to bc908e4 Compare April 17, 2020 13:27

abhinavdahiya reviewed Apr 17, 2020

View reviewed changes

pkg/types/gcp/validation/platform.go Show resolved Hide resolved

abhinavdahiya reviewed Apr 17, 2020

View reviewed changes

amorenoz force-pushed the wip/gcp_nested branch 3 times, most recently from 052f09b to 6b6565d Compare April 17, 2020 17:24

abhinavdahiya reviewed Apr 17, 2020

View reviewed changes

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 23, 2020

GCP: add support for image licenses

7827bab

Add an optional parameter in the GCP install-config that contains a list of license URLs to be added to the compute image Credits: Based on the work by Colin Walters Signed-off-by: Adrian Moreno <amorenoz@redhat.com>

amorenoz force-pushed the wip/gcp_nested branch from 6b6565d to 7827bab Compare April 24, 2020 08:26

abhinavdahiya reviewed Apr 24, 2020

View reviewed changes

openshift-ci-robot assigned abhinavdahiya Apr 24, 2020

openshift-merge-robot merged commit 64f1a13 into openshift:master Jun 4, 2020

This was referenced Feb 25, 2021

Add and use stream metadata for RHCOS, add openshift-install coreos print-stream-json #4582

Merged

gcp: Remove support for Licenses, only use public image #4696

Closed

	// URIWithProtocol validates that the URI specifies a certain
	// protocol scheme (e.g. "https")
	func URIWithProtocol(uri string, protocol string) error {
	parsed, err := url.Parse(uri)
	if err != nil {
	return err
	}
	if parsed.Scheme != protocol {
	return fmt.Errorf("must use %s protocol", protocol)
	}
	return nil
	}

Add nested support for GCP #3430

Add nested support for GCP #3430

Conversation

amorenoz commented Apr 8, 2020

amorenoz commented Apr 8, 2020

openshift-ci-robot commented Apr 8, 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

abhinavdahiya commented Apr 8, 2020

cgwalters commented Apr 8, 2020 • edited Loading

abhinavdahiya commented Apr 9, 2020

amorenoz commented Apr 14, 2020

abhinavdahiya commented Apr 14, 2020

amorenoz commented Apr 14, 2020

amorenoz commented Apr 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinavdahiya Apr 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amorenoz Apr 20, 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

abhinavdahiya commented Apr 23, 2020

abhinavdahiya commented Apr 23, 2020

abhinavdahiya left a comment

Choose a reason for hiding this comment

openshift-bot commented May 8, 2020

cgwalters commented May 8, 2020

openshift-bot commented May 9, 2020

openshift-bot commented May 9, 2020

openshift-bot commented May 9, 2020

openshift-bot commented May 9, 2020

openshift-bot commented May 9, 2020

openshift-bot commented May 9, 2020

openshift-bot commented May 9, 2020

cgwalters commented May 9, 2020

dustymabe commented May 22, 2020

cgwalters commented May 26, 2020

dustymabe commented May 26, 2020

openshift-bot commented Jun 3, 2020

openshift-bot commented Jun 3, 2020

openshift-bot commented Jun 3, 2020

openshift-bot commented Jun 3, 2020

openshift-bot commented Jun 3, 2020

openshift-bot commented Jun 3, 2020

openshift-bot commented Jun 3, 2020

openshift-bot commented Jun 3, 2020

openshift-bot commented Jun 3, 2020

openshift-bot commented Jun 3, 2020

openshift-bot commented Jun 4, 2020

fabiand commented Jun 4, 2020

fabiand commented Jun 4, 2020

amorenoz commented Jun 4, 2020

cgwalters commented Apr 8, 2020 •

edited

Loading

abhinavdahiya Apr 17, 2020 •

edited

Loading

amorenoz Apr 20, 2020 •

edited

Loading