Skip to content

mantle/ore: gcloud: support specifying multiple image licenses#1477

Merged
openshift-merge-robot merged 2 commits intocoreos:masterfrom
dustymabe:dusty-gcp-more-licenses
May 26, 2020
Merged

mantle/ore: gcloud: support specifying multiple image licenses#1477
openshift-merge-robot merged 2 commits intocoreos:masterfrom
dustymabe:dusty-gcp-more-licenses

Conversation

@dustymabe
Copy link
Copy Markdown
Member

This will allow us to specify our normal fedora-coreos-$stream
license as well as the nested virtualization license:

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

dustymabe added 2 commits May 22, 2020 18:09
We'll use it to pass --log-level=INFO in the pipeline because the
default logging level appears to be NOTICE.
This will allow us to specify our normal fedora-coreos-$stream
license as well as the nested virtualization license:

https://compute.googleapis.com/compute/v1/projects/vm-options/global/licenses/enable-vmx
@cgwalters
Copy link
Copy Markdown
Member

Seems fine as is, but I'd lean a bit towards hardcoding the licenses, and adding --no-default-licenses or something. IMO part of the reason we have tools like ore and we're not just shelling out to underlying tools like gcloud is exactly that we want to streamline CoreOS builds. This is conceptually orthogonal to your patch - supporting adding a list of licenses pairs with --no-default-licenses.

If we hardcoded them then we'd share between FCOS+RHCOS automatically versus needing to encode them in the pipeline.

@dustymabe
Copy link
Copy Markdown
Member Author

If we hardcoded them then we'd share between FCOS+RHCOS automatically versus needing to encode them in the pipeline.

I think I'm fine with that approach if we can agree that we want the nested virt change for RHCOS too. Right now the change would only apply to FCOS.

@dustymabe
Copy link
Copy Markdown
Member Author

If we hardcoded them then we'd share between FCOS+RHCOS automatically versus needing to encode them in the pipeline.

I think I'm fine with that approach if we can agree that we want the nested virt change for RHCOS too. Right now the change would only apply to FCOS.

I figured it might take some time to get some agreement on the RHCOS side so I went with this for now.

@jlebon
Copy link
Copy Markdown
Member

jlebon commented May 25, 2020

Hmm, why wouldn't we want to do this for RHCOS too? I might be missing context here.

@cgwalters
Copy link
Copy Markdown
Member

Hmm, why wouldn't we want to do this for RHCOS too? I might be missing context here.

I think we do - but so far this "it's ok to add nested virt always" bit is something that happened from a live discussion between Dusty and GCP engineers and isn't written down formally right?

Ideally this is one of those changes that we lead w/FCOS (+OKD) and let percolate down to RHCOS (+OCP) once we've tried it for a while.

If we get that bit written down formally, then 👍 from me to just ship it always.

@cgwalters
Copy link
Copy Markdown
Member

/test sanity
/approve
/lgtm

@ashcrow
Copy link
Copy Markdown
Member

ashcrow commented May 26, 2020

/approve

@ashcrow
Copy link
Copy Markdown
Member

ashcrow commented May 26, 2020

/lgtm

@ashcrow
Copy link
Copy Markdown
Member

ashcrow commented May 26, 2020

/shrug

@ashcrow
Copy link
Copy Markdown
Member

ashcrow commented May 26, 2020

/approve

@cgwalters
Copy link
Copy Markdown
Member

/lgtm
/test sanity

@openshift-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashcrow, cgwalters, dustymabe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [ashcrow,cgwalters,dustymabe]

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

@openshift-merge-robot openshift-merge-robot merged commit 560c2dc into coreos:master May 26, 2020
@zmarano
Copy link
Copy Markdown

zmarano commented May 26, 2020

From the GCE point of view, enabling nested virt on an image by default (currently via the license resource addition) is perfectly fine for your use case. It will not change behaviors for users except to expose /dev/kvm (its synonymous to turning on the Intel-VTx setting on a physical machine in the firmware). That said, there isn't any guarantee that a given guest VM within the nested virt host VM will work or that the user is using a machine type that supports it. The matrix of possibilities there is enormous. It is therefore similar to saying your OS supports KVM on the hardware it is running on and by default allowing your users to use KVM on GCE VM's. I know this is a bit wishy-washy but let me know if I can clarify any of these points.

This is the doc on this topic and it is largely saying the same things in a different way.

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.

7 participants