✨ content library support proposal #1839

adam-jian-zhang · 2023-03-27T07:57:24Z

What this PR does / why we need it:

Proposal for content library support.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1838

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:


Support content library as the source for VM clone.

k8s-ci-robot · 2023-03-27T07:57:33Z

Hi @adam-jian-zhang. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

adam-jian-zhang · 2023-03-27T07:58:08Z

@srm09 please have a review.

docs/proposal/content-library-support.md

srm09

/lgtm
@yastij what do you think?

docs/proposal/content-library-support.md

srm09 · 2023-04-06T21:12:59Z

/assign @yastij
/ok-to-test

The verify markdown job is failing, could you please fix that.

adam-jian-zhang · 2023-04-13T12:47:06Z

@srm09, @yastij , @rikatz, @vrabbi I have addressed the comments, and fixed lint, please have a review.

rikatz · 2023-04-13T12:55:36Z

/lgtm
Thank you!

docs/proposal/content-library-support.md

randomvariable · 2023-07-20T17:29:14Z

/assign @vrabbi
/assign @randomvariable

docs/proposal/content-library-support.md

chrischdi

This sounds like a reasonable feature and implementation looks pretty straight forward.

Should we additionally take care via validation, that if the Template refers a library, that the CloneMode is set to fullClone?

From testing perspective: I think we could just switch over a single e2e test to clone from a local content library in the test environment, which then uses the same template as before but just from a different source.

chrischdi · 2023-07-28T08:01:13Z

docs/proposal/content-library-support.md

+  // It is optional since enabling access control for the content library is optional. It's up to content library admin to decide if
+  // it is required to enable access control.
+  // +optional
+  LibraryCredentials string `json:"library_credentials"`


Should we make this of type corev1.TypedLocalObjectReference?

This way we would be able to in future support other object types too without having to adjust the API.

Why are credentials for the library needed? A sequence diagram of interactions between VA-API and CLS that go from library item to VM creation should make this clear.

The content library will already have been subscribed to in vCenter with whatever http credentials it needs, we should only need to pass it's UUID to CreateTemplate or CreateOVF

re-reading the doc, as the overall design states that For the content library management itself users can reference vSphere documentation, I agree the credential should not be required.

Also, we should probably add that the "Management of content library items and subscriptions" is a nongoal, so it is clear that the VI admin should first subscribe/create a content library and CAPV will only use it.

randomvariable

I want to see quite a bit more detail before approving this one. Please respond to comments.

randomvariable · 2023-07-28T16:10:52Z

docs/proposal/content-library-support.md

+  // It is optional since enabling access control for the content library is optional. It's up to content library admin to decide if
+  // it is required to enable access control.
+  // +optional
+  LibraryCredentials string `json:"library_credentials"`


Why are credentials for the library needed? A sequence diagram of interactions between VA-API and CLS that go from library item to VM creation should make this clear.

The content library will already have been subscribed to in vCenter with whatever http credentials it needs, we should only need to pass it's UUID to CreateTemplate or CreateOVF

randomvariable · 2023-07-28T16:24:24Z

docs/proposal/content-library-support.md

+
+##### VSphereVM clone changes
+
+We need to enhance the [clone](https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/blob/19c5deb79e97a8cd9d546ec39db3f29231bbcb91/pkg/services/govmomi/vcenter/clone.go#L75) function to recogize the scheme we defined, and choose appropriate API to clone the VM.


I'd like some more details on this before proceeding. I think you will want to use https://github.com/vmware/govmomi/blob/main/vapi/vcenter/vcenter_ovf.go#L292-L306 and then https://github.com/vmware/govmomi/blob/252e157f27b33d5cebeecdb21212e3f3fbe44039/object/virtual_machine.go#L72

We already have this logic in VM Service mode at https://github.com/vmware-tanzu/vm-operator/blob/main/pkg/vmprovider/providers/vsphere/session/session_vm_create.go

Also some good direct reference in https://github.com/vmware/govmomi/blob/main/govc/library/deploy.go

I believe it needs to download the image content from Content Library when deploying.
We need to be careful here as it may run multiple times and create a heavy workload to vCenter if we need multiple VM instances.
I think it would be better to import the image as a separate template and do the clone as what we have today.

When creating a subscribed content library, there are two options for dealing with the content, download immediately/when needed.
I think we should advise user to use eager option(download immediately).
If user choose lazy option(when needed), I think the work round here is to deploy a test vm before creating clusters, or create a small test cluster to fetch the library items to local.

The problem with download immediately is that we may end up subscribing to CL with a lot of images that may be problematic for long term storage space.

Anyway, can we agree this is not an enhancement question but more an implementation detail?

If a content library is on-demand subscribed, an OVF template in it can be still deployed, it will take longer as it needs to download the image from the publisher first and may delay the cluster deployment, what other problems are we trying to avoid?

randomvariable · 2023-08-03T17:19:16Z

/assign @rikatz

randomvariable · 2023-08-15T13:40:00Z

/lgtm cancel

rikatz · 2023-08-21T20:40:43Z

docs/proposal/content-library-support.md

+Content library can help template management in this case, it supports subscriptions among different content libraries and provides sync capability to help distribute OVA/templates to downstream content libraries. For example, we can set up a content library on the primary vCenter that hosts the management cluster, put the OVA/templates in the content library which serves as the version of truth for the OVA/templates we intended to use. And then for other vCenters that
+hosts workload clusters, we set up a content library and subscribe to the primary content library, and sync the content library items(templates/OVAs) to it. In this way it is easy to manage templates/OVAs and have confidence in the content of the templates/OVAs.
+
+Using content library as the source of templates has another little benefit that enables using local datastore as destination for the VMs, currently we are limited to shared datastore.


I don't think this is a problem solved by content library, but mostly because of a problem on the way we used to clone our templates: #2134 as we always set the datastore as the same from the template (while VC does support cloning between different datastores).

rikatz · 2023-08-21T20:47:31Z

docs/proposal/content-library-support.md

+
+### Goals
+
+* Use ContentLibrary items as the VM image template.


Just to explore a bit more on goals and non goals, are we going to support at this time "OVA and OVF templates" and "VM templates"?

The "OVA and OVF templates" deploy should be the one simpler, where we just "Import the item" on the content library, while the 2nd one is when we deploy a VM, customize it on vSphere and clone it back as a template to CL.

It would be good to make it clear what we want to support right now :)

Also: do we want to provide an official subscribable content library for OVAs? If not, probably this should be a non goal to make it explicit.

rikatz · 2023-08-21T20:58:06Z

hey @adam-jian-zhang , just took a look into the proposal again and left some comments.

Additionally, out of the proposal but IMHO should be part of the implementation, I think we need to consider that:

This workflow should be as easy and transparent as possible to the user. This means that from an API change, I think it is fine to have just the scheme/itemname support and nothing else.
While not part of the direct implementation, I think that lowering the adoption bar of this feature would be great for users, so I would expect that there's a quick start documentation on using Content Libraries, some real next/next/finish like "Create content library, import OVA item, use it on your CAPV cluster".

I'm happy and also interested to help you move this proposal forward, let me know if you need something from me to keep this moving!

chrischdi · 2023-08-31T17:26:43Z

@rikatz maybe worth talking directly to @adam-jian-zhang to figure out when and who will continue on this one or if it makes sense to take this over :-)

k8s-ci-robot · 2023-09-21T21:52:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from randomvariable. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

codecov · 2023-09-21T21:56:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.89%. Comparing base (d37d6f9) to head (76f9abb).
Report is 640 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1839      +/-   ##
==========================================
- Coverage   60.96%   60.89%   -0.08%     
==========================================
  Files         164      164              
  Lines        9469     9469              
==========================================
- Hits         5773     5766       -7     
- Misses       3285     3290       +5     
- Partials      411      413       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

chrischdi · 2023-09-22T11:12:53Z

docs/proposal/content-library-support.md

+* Parse the field as a URI. If just a "Host" is present, it means user is requesting an item name, so it should be queried by item name
+* If both host and item are present, then host is the content library name (or uuid) and path is the item name


Do I get it right that it would be possible to use it via (noob content library question):

not specify the library name or id: library://ubuntu-22.04-k8s-1.28.0

specify the library name or id: library://foo/ubuntu-22.04-k8s-1.28.0

Adding direct examples here would make it a better read :-)

yes you do! but if there are two items named ubuntu-22.04-k8s-1.28.0 on different libraries it should be an error, as CAPV should not select for the user one of two IMHO

Same for 2 content libraries with same names on VC.

Let me add some examples here!

That sounds good, thx

Sounds good, we need a way to unique identify the library item user want, the proposed scheme library://library_name_or_id/itemname should be able to address that.
library_id is what we actually needed here to resolve ambiguity here, allowing user to use library_name instead or even omitting the library_id/library_name when the item name is unique is just for enhancing user experience.

Personally I think the full path is better( although it is harder to use), as it can avoid us problem down the road. I remember we have several similar issues for templates, as we allow user to just specify the template name not the full path of the template, and use tkr-resolver to update it to the full path later, which may break some time.

I've discussed with @adam-jian-zhang and I'm also adding to the status field of vspherevm what was the "resolved" library id / item name

I was even thinking about adding the item ID, but I think in this case is too much, as we are not allowing users to add the item ID anyway, wdyt?

Question: what would the status field get used for? If it is just for visualization: I think having a log line would also be okay.

good question, IIRC vspherevm is not mutable after cloned right? So it would just be a "I'm based on this image" field, which can be a log line yes (unless I'm forgetting some other condition)

If we don't actually use the field I slightly tend to only log it yes 👍

I assume the image could get moved in the content library, so the status field may get outdated.

In Content Library, the name can change, but the ID of a library or library item does not, so showing the resolved ID in the status can be useful in certain scenarios.

chrischdi · 2023-09-22T11:17:39Z

docs/proposal/content-library-support.md

+        // ERROR deploying the content library item
+    }
+
+	finder := find.NewFinder(vim25Client)


I assume DeployLibraryItem is not instantly ready and would result in a task on vsphere? so finding the object is after the task did run right? And the logic below here should then be the same as for the normal clone?

So, it appears to not return a task, I'm checking here

It does not show the task in the deploy result though it does create a vCenter task, you can find it by query vCenter task API, and may check how VM operator handle it.

chrischdi · 2023-09-22T11:19:19Z

docs/proposal/content-library-support.md

+
+func deployContentLibraryAsVM(itemID string, additionalargs...) (*object.VirtualMachine, error) {
+	m := vcenter.NewManager(restclient)
+	ref, err := m.DeployLibraryItem(ctx, itemID string, vcenter.Deploy{


This function call is basically the equivalent to calling tpl.Clone right? (

cluster-api-provider-vsphere/pkg/services/govmomi/vcenter/clone.go

Line 75 in 19c5deb

tpl, err := template.FindTemplate(ctx, ctx.VSphereVM.Spec.Template)

)

correct :) And the customization will need to happen differently, as we know what exists on a template, but the OVF deployment is different (I need to figure out actually if there's a way to get the spec and customize accordingly, I didn't wanted to go that down on the spec right now).

So, for template we call lastly tpl.Clone that generates a new task.

And for library we call DeployLibraryItem that does that directly (I'm checking if there's a way to generate a task as well)

rikatz · 2023-09-25T14:14:21Z

Question: should we add a featureflag for it?

chrischdi · 2023-09-25T14:25:08Z

Question: should we add a featureflag for it?

Good question: I tend to say no, because you have to explicitly use the library:// prefix to make use of it. There's no magical "gets used".

When adding a feature gate for it I think we would also have to verify it on creation (if the feature gate is disabled) that no library:// prefix gets set.

rikatz · 2023-11-20T22:12:14Z

@chrischdi 👋

I'm coming back to this (after almost 2 months of hiatus, sorry!)

Can you take a new look just to see if I'm missing something?

I think the most important piece right now is that the "cloning" from a Content Library should happen on a different workflow, as it does not use a template/object reference but instead https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/pull/1839/files#diff-05d6df7ed3848d35aa732cfdeb480d8a394f42809bc3489bc87a9bcdcc0fcc6dR219 and also as you pointed, it is "sync" (does not return a task).

Thanks!

chrischdi · 2023-11-21T08:24:08Z

Hi @rikatz , I think this looks good to continue.

The only question coming up for me after reading all again is this part here:

https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/pull/1839/files#r1335335234

About "do we have to pre download it in some way" but I'd let it up to you if there is something to think about here. It would be more obvious to me when we have code :-)

ericvmw · 2023-11-22T02:39:25Z

docs/proposal/content-library-support.md

+  // the virtual machine. It can be a vSphere template, or a content library item. For example:
+  // vsphere template: [vsphere://location of the vsphere template], vsphere:// can be omitted for backward compatibility
+  // content library: [library://library_name_or_id/content_library_item_name]
+  // note that this has impact on cloneMode field, since it is only possible to do full clone from content library items,


The full clone holds true for OVF templates in Content Library, Content Library has the checkin/checkout feature for VMTX templates (supported within single VC) that can do linked clone ([checkin checkout])(https://developer.vmware.com/apis/vsphere-automation/latest/vcenter/api/vcenter/vm-template/library-items/template_library_item/check-outsactioncheck-out/post/), if this VirtualMachineCloneSpec may support VMTX template from Content Library, better to mention this fullclone is for OVF template only.

ericvmw · 2023-11-22T02:47:02Z

docs/proposal/content-library-support.md

+* Parse the field as a URI. If just a "Host" is present, it means user is requesting an item name, so it should be queried by item name
+* If both host and item are present, then host is the content library name (or uuid) and path is the item name


In Content Library, the name can change, but the ID of a library or library item does not, so showing the resolved ID in the status can be useful in certain scenarios.

ericvmw · 2023-11-22T02:56:58Z

docs/proposal/content-library-support.md

+
+##### VSphereVM clone changes
+
+We need to enhance the [clone](https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/blob/19c5deb79e97a8cd9d546ec39db3f29231bbcb91/pkg/services/govmomi/vcenter/clone.go#L75) function to recogize the scheme we defined, and choose appropriate API to clone the VM.


If a content library is on-demand subscribed, an OVF template in it can be still deployed, it will take longer as it needs to download the image from the publisher first and may delay the cluster deployment, what other problems are we trying to avoid?

ericvmw · 2023-11-22T03:03:17Z

docs/proposal/content-library-support.md

+        // ERROR deploying the content library item
+    }
+
+	finder := find.NewFinder(vim25Client)


It does not show the task in the deploy result though it does create a vCenter task, you can find it by query vCenter task API, and may check how VM operator handle it.

k8s-ci-robot · 2024-01-26T18:23:09Z

@adam-jian-zhang: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-vsphere-e2e-main	`30e3b1d`	link	true	`/test pull-cluster-api-provider-vsphere-e2e-main`
pull-cluster-api-provider-vsphere-e2e-blocking-main	`76f9abb`	link	true	`/test pull-cluster-api-provider-vsphere-e2e-blocking-main`

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.

sbueringer · 2024-04-09T12:11:09Z

/close

Let's revive if there is interest in this again

k8s-ci-robot · 2024-04-09T12:11:14Z

@sbueringer: Closed this PR.

In response to this:

/close

Let's revive if there is interest in this again

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.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 27, 2023

k8s-ci-robot requested review from MaxRink and vrabbi March 27, 2023 07:57

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 27, 2023

adam-jian-zhang force-pushed the content-library branch from 020b460 to 21630bd Compare March 27, 2023 14:40

vrabbi reviewed Mar 27, 2023

View reviewed changes

docs/proposal/content-library-support.md Outdated Show resolved Hide resolved

adam-jian-zhang force-pushed the content-library branch from 21630bd to 47a2a18 Compare March 28, 2023 02:35

srm09 reviewed Apr 6, 2023

View reviewed changes

docs/proposal/content-library-support.md Outdated Show resolved Hide resolved

k8s-ci-robot assigned srm09 Apr 6, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 6, 2023

rikatz reviewed Apr 6, 2023

View reviewed changes

docs/proposal/content-library-support.md Outdated Show resolved Hide resolved

rikatz reviewed Apr 6, 2023

View reviewed changes

docs/proposal/content-library-support.md Outdated Show resolved Hide resolved

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Apr 6, 2023

k8s-ci-robot assigned yastij Apr 6, 2023

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 6, 2023

adam-jian-zhang force-pushed the content-library branch from 47a2a18 to 30e3b1d Compare April 13, 2023 12:41

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 13, 2023

adam-jian-zhang requested review from rikatz, vrabbi and srm09 April 13, 2023 12:45

k8s-ci-robot assigned rikatz Apr 13, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 13, 2023

zhanggbj reviewed Jun 28, 2023

View reviewed changes

docs/proposal/content-library-support.md Show resolved Hide resolved

k8s-ci-robot assigned randomvariable and vrabbi Jul 20, 2023

chrischdi reviewed Jul 28, 2023

View reviewed changes

docs/proposal/content-library-support.md Show resolved Hide resolved

chrischdi reviewed Jul 28, 2023

View reviewed changes

randomvariable requested changes Jul 28, 2023

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 15, 2023

k8s-ci-robot requested review from randomvariable and yastij August 15, 2023 13:40

rikatz reviewed Aug 21, 2023

View reviewed changes

adam-jian-zhang and others added 2 commits September 21, 2023 18:51

content library support proposal

ef91318

Add implementation details on content library feature

76f9abb

rikatz force-pushed the content-library branch from 30e3b1d to 76f9abb Compare September 21, 2023 21:52

chrischdi reviewed Sep 22, 2023

View reviewed changes

ericvmw reviewed Nov 22, 2023

View reviewed changes

k8s-ci-robot closed this Apr 9, 2024


		##### VSphereVM clone changes

		We need to enhance the [clone](https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/blob/19c5deb79e97a8cd9d546ec39db3f29231bbcb91/pkg/services/govmomi/vcenter/clone.go#L75) function to recogize the scheme we defined, and choose appropriate API to clone the VM.


		### Goals

		* Use ContentLibrary items as the VM image template.

		* Parse the field as a URI. If just a "Host" is present, it means user is requesting an item name, so it should be queried by item name
		* If both host and item are present, then host is the content library name (or uuid) and path is the item name

✨ content library support proposal #1839

✨ content library support proposal #1839

Conversation

adam-jian-zhang commented Mar 27, 2023

k8s-ci-robot commented Mar 27, 2023

adam-jian-zhang commented Mar 27, 2023

srm09 left a comment

Choose a reason for hiding this comment

srm09 commented Apr 6, 2023

adam-jian-zhang commented Apr 13, 2023

rikatz commented Apr 13, 2023

randomvariable commented Jul 20, 2023

chrischdi left a 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

randomvariable left a 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

randomvariable commented Aug 3, 2023

randomvariable commented Aug 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rikatz commented Aug 21, 2023

chrischdi commented Aug 31, 2023

k8s-ci-robot commented Sep 21, 2023

codecov bot commented Sep 21, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-jian-zhang Sep 25, 2023 • 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

chrischdi Sep 22, 2023 • 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

rikatz commented Sep 25, 2023

chrischdi commented Sep 25, 2023

rikatz commented Nov 20, 2023

chrischdi commented Nov 21, 2023

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

k8s-ci-robot commented Jan 26, 2024

sbueringer commented Apr 9, 2024

k8s-ci-robot commented Apr 9, 2024

codecov bot commented Sep 21, 2023 •

edited

Loading

adam-jian-zhang Sep 25, 2023 •

edited

Loading

chrischdi Sep 22, 2023 •

edited

Loading