VM Image Controller, Webhook and Download/Upload Refactor for Various Backend Support #7170

WebberHuang1118 · 2024-12-16T07:46:44Z

Code Structure:

Backend interfaces:
- pkg/image/backend
Common VM Image operation package
- pkg/image/common
Backing image implementation
- pkg/image/backingimage
CDI implementation skeleton
- pkg/image/cdi

Problem:
VM Image should support various source backend

Solution:
Enhance VM Image implementation to have a clear interface for different image backend

Related Issue:
#6936

HEP:
#7273

Test plan:

Prerequisites
- Building Harvester Cluster with the master-head branch ( 0cea310be669040bc682c75edd40fd6f81051bce in my env)
- Updating VirtualMachineImage CRD
  - $ kubectl apply -f deploy/charts/harvester-crd/templates/harvesterhci.io_virtualmachineimages.yaml
- Updating harvester deployment with this PR
- Updating harvester-webhook deployment with this PR
Case 1: Creating VM image with upload
- Uploading an OS image file from local
- Checking the VM image could progress to the completed status
- Creating a VM with the VM image
Case 2: Creating VM image with download
- Downloading an OS image file from a certain URL
- Checking the VM image could progress to the completed status
- Creating a VM with the VM image
Case 3: Creating VM image with export
- Creating a volume with an existing OS VM Image
- Exporting the volume as a new image
- Checking the exported VM image could progress to the completed status
- Creating a VM with the VM image
Case 4: VM Image encryption
- Creating a secret and sc for encryption
- Creating an encrypted VM image from an existing VM image (non-encrypted)
- Checking the encrypted VM image could progress to the completed status
- Creating a VM with the VM image
Case 5: VM Image decryption
- Continuing from the above steps
- Creating a VM image by decrypting the encrypted VM image
- Checking the decrypted VM image could progress to the completed status
- Creating a VM with the VM image
Case 6: VM Image Downloading
- Triggering downloading an existing VM image
- The VM image should downloaded as a compressed file

** Additional Context **
Since the VM image implementation has the new interfaces, the original unit testing is not applicable and removed. Creating a separate ticket to track the unit testing implementation #7204

brandboat

Thanks for the great refactor!

pkg/image/backingimage/upload.go

pkg/image/common/operator.go

pkg/image/backingimage/upload.go

Yu-Jack

Nice try! There is a suggestion for folder structure:

I think our folder structure should match what it's , and backend might be a confused naming.

For example, you could move pkg/image/backend/backend.go to pkg/master/controller/image/interface.go, and rename the interface name like CSI or CSIInterface etc.

Then we could use import pkg/master/controller/image, and use image.CSIInterface to present the meaning instead of backend.Backend.

The pkg/image/backingimage/backing_image_controller.go should be in pkg/master/controller. Spreading the controller on different places might not match our current folder structure, including mutator and validator.

But, I guess that you think putting them on same folder is more easier for finding and looking , So, there is another way to organize them. Create a new folder to present our core (real business) logic. That means we could put similar feature into this folder in the future if we need.

core
└── image
    ├── backingimage
    │   ├── backing_image_controller.go
    │   ├── backingimage.go
    │   ├── download.go
    │   ├── mutate.go
    │   ├── upload.go
    │   └── validate.go
    ├── cdi
    │   ├── cdi.go
    │   ├── download.go
    │   ├── mutate.go
    │   ├── upload.go
    │   └── validate.go
    ├── common
    │   ├── mutator.go
    │   ├── operator.go
    │   └── validator.god
    └── interface.go

However, I still think backing_image_controller.go should be master/controller, and keep mutator.go and validator.go in original folder. We can wait for other people's feedbacks.

WebberHuang1118 · 2025-01-06T04:11:23Z

Thanks for the feedbacks @Yu-Jack

Nice try! There is a suggestion for folder structure:

I think our folder structure should match what it's , and backend might be a confused naming.

For example, you could move pkg/image/backend/backend.go to pkg/master/controller/image/interface.go, and rename the interface name like CSI or CSIInterface etc.

Then we could use import pkg/master/controller/image, and use image.CSIInterface to present the meaning instead of backend.Backend.

It's a good idea, I will update it.

The pkg/image/backingimage/backing_image_controller.go should be in pkg/master/controller. Spreading the controller on different places might not match our current folder structure, including mutator and validator.

Because pkg/image/backingimage/backing_image_controller.go implements the sidecar handler for LH image, which is specific to LH backingimage, Harvester may support all the image features by CDI someday. IMO, placing the sidecar handler implementation at the general path pkg/master/controller seems not suitable.

But, I guess that you think putting them on same folder is more easier for finding and looking , So, there is another way to organize them. Create a new folder to present our core (real business) logic. That means we could put similar feature into this folder in the future if we need.
core
└── image
    ├── backingimage
    │   ├── backing_image_controller.go
    │   ├── backingimage.go
    │   ├── download.go
    │   ├── mutate.go
    │   ├── upload.go
    │   └── validate.go
    ├── cdi
    │   ├── cdi.go
    │   ├── download.go
    │   ├── mutate.go
    │   ├── upload.go
    │   └── validate.go
    ├── common
    │   ├── mutator.go
    │   ├── operator.go
    │   └── validator.god
    └── interface.go

I will consider it, thanks.

However, I still think backing_image_controller.go should be master/controller, and keep mutator.go and validator.go in original folder. We can wait for other people's feedbacks.

The VM image mutator and validator are still provided by the original path pkg/webhook/resources/virtualmachineimage/mutator.go and pkg/webhook/resources/virtualmachineimage/validator.go.

pkg/image/backingimage/mutate.go and pkg/image/backingimage/validate.go aim to provide the common library for VM Image mutation and validation.

Yu-Jack · 2025-01-06T05:21:27Z

Because pkg/image/backingimage/backing_image_controller.go implements the sidecar handler for LH image, which is specific to LH backingimage, Harvester may support all the image features by CDI someday. IMO, placing the sidecar handler implementation at the general path pkg/master/controller seems not suitable.

You could organize like this. It's just an example.

controller/master/image
├── register.go
├── third-party
│   ├── backingimage
│   │   ├── backing_image_controller.go
│   │   ├── backingimage.go
│   │   ├── download.go
│   │   ├── mutate.go
│   │   ├── upload.go
│   │   └── validate.go
│   └── cdi
│       ├── cdi.go
│       ├── download.go
│       ├── mutate.go
│       ├── upload.go
│       └── validate.go
└── vm_image_controller.go

The VM image mutator and validator are still provided by the original path pkg/webhook/resources/virtualmachineimage/mutator.go and pkg/webhook/resources/virtualmachineimage/validator.go.

pkg/image/backingimage/mutate.go and pkg/image/backingimage/validate.go aim to provide the common library for VM Image mutation and validation.

Yeah, I also treat pkg/webhook/resources/virtualmachineimage/mutator.go and pkg/webhook/resources/virtualmachineimage/validator.go as a wrapper to wrap the core business logic. But, you could still do similar things like above to organize them if we're willing to do so. The point I'd like to highlight is the folder structure. Should we put all related business logic into same folder or organize by validator, mutator or controller like this? I just want to provide different perspectives to this topic.

webhook/resources/virtualmachineimage
├── mutator.go
├── third-party
│   ├── backingimage
│   │   ├── mutate.go
│   │   └── validate.go
│   └── cdi
│       ├── cdi.go
│       ├── mutate.go
│       └── validate.go
└── validator.go

controller/master/image
├── register.go
├── third-party
│   ├── backingimage
│   │   ├── backing_image_controller.go
│   │   └── backingimage.go
│   └── cdi
│       └── cdi.go
└── vm_image_controller.go

api/image
├── formatter.go
├── schema.go
└── third-party
    ├── backingimage
    │   ├── download.go
    │   └── upload.go
    └── cdi
        ├── cdi.go
        ├── download.go
        └── upload.go

WebberHuang1118 · 2025-01-06T06:52:30Z

Because pkg/image/backingimage/backing_image_controller.go implements the sidecar handler for LH image, which is specific to LH backingimage, Harvester may support all the image features by CDI someday. IMO, placing the sidecar handler implementation at the general path pkg/master/controller seems not suitable.

You could organize like this. It's just an example.
controller/master/image
├── register.go
├── third-party
│   ├── backingimage
│   │   ├── backing_image_controller.go
│   │   ├── backingimage.go
│   │   ├── download.go
│   │   ├── mutate.go
│   │   ├── upload.go
│   │   └── validate.go
│   └── cdi
│       ├── cdi.go
│       ├── download.go
│       ├── mutate.go
│       ├── upload.go
│       └── validate.go
└── vm_image_controller.go
The VM image mutator and validator are still provided by the original path pkg/webhook/resources/virtualmachineimage/mutator.go and pkg/webhook/resources/virtualmachineimage/validator.go.
pkg/image/backingimage/mutate.go and pkg/image/backingimage/validate.go aim to provide the common library for VM Image mutation and validation.

Yeah, I also treat pkg/webhook/resources/virtualmachineimage/mutator.go and pkg/webhook/resources/virtualmachineimage/validator.go as a wrapper to wrap the core business logic. But, you could still do similar things like above to organize them if we're willing to do so. The point I'd like to highlight is the folder structure. Should we put all related business logic into same folder or organize by validator, mutator or controller like this? I just want to provide different perspectives to this topic.
webhook/resources/virtualmachineimage
├── mutator.go
├── third-party
│   ├── backingimage
│   │   ├── mutate.go
│   │   └── validate.go
│   └── cdi
│       ├── cdi.go
│       ├── mutate.go
│       └── validate.go
└── validator.go

controller/master/image
├── register.go
├── third-party
│   ├── backingimage
│   │   ├── backing_image_controller.go
│   │   └── backingimage.go
│   └── cdi
│       └── cdi.go
└── vm_image_controller.go

api/image
├── formatter.go
├── schema.go
└── third-party
    ├── backingimage
    │   ├── download.go
    │   └── upload.go
    └── cdi
        ├── cdi.go
        ├── download.go
        └── upload.go

My idea is if anyone wants to add another image solution (e.q. LH v2 image), we should make the task as easy as possible, so I encapsulate the details for VM Image, and it's provided as an interface/library.

By the current directory structure:

pkg/image/
├── backend
│   └── backend.go
├── backingimage
│   ├── backing_image_controller.go
│   ├── backingimage.go
│   ├── download.go
│   ├── mutate.go
│   ├── upload.go
│   └── validate.go
├── cdi
│   ├── cdi.go
│   ├── download.go
│   ├── mutate.go
│   ├── upload.go
│   └── validate.go
└── common
    ├── mutator.go
    ├── operator.go
    └── validator.go

If there is a requirement for new image solution, ideally, it only needs to add implementations under pkg/image. Compared to the structure:

webhook/resources/virtualmachineimage
├── mutator.go
├── third-party
│   ├── backingimage
│   │   ├── mutate.go
│   │   └── validate.go
│   └── cdi
│       ├── cdi.go
│       ├── mutate.go
│       └── validate.go
└── validator.go

controller/master/image
├── register.go
├── third-party
│   ├── backingimage
│   │   ├── backing_image_controller.go
│   │   └── backingimage.go
│   └── cdi
│       └── cdi.go
└── vm_image_controller.go

api/image
├── formatter.go
├── schema.go
└── third-party
    ├── backingimage
    │   ├── download.go
    │   └── upload.go
    └── cdi
        ├── cdi.go
        ├── download.go
        └── upload.go

the new implementation will be spreaded among different places.

Should we put all related business logic into same folder or organize by validator, mutator or controller like this? I just want to provide different perspectives to this topic.

IMO, the business logic is still controlled by the original way, I mean:

Controller: pkg/controller/master/image/vm_image_controller.go
Webhook: pkg/webhook/resources/virtualmachineimage/mutator.go and pkg/webhook/resources/virtualmachineimage/validator.go
API handler: pkg/api/image/formatter.go

Source codes under pkg/image are provided for the implementation of different image solutions, and are passively invoked by the original control path.

I can understand your concern, I think we can wait for other colleagues' feedback @bk201 @brandboat @Vicente-Cheng @FrankYang0529

brandboat · 2025-01-10T08:28:44Z

I believe what Jack is trying to highlight is the importance of consistency. It seems that all modules currently follow the same pattern, with mutators and validators placed under the webhook directory, and controllers under the controller directory. Personally, I lean towards keeping things as they are, which means sticking to this pattern.

However, the drawback is clear— as you mentioned, the backing image code is scattered across multiple locations. I believe this is a separate issue and might be worth opening a new discussion or issue. Additionally, as we plan to support third-party storage in more features, I believe this problem may not be limited to VM images and could affect other areas (like vm backup) as well.

WebberHuang1118 · 2025-01-10T08:43:13Z

I believe what Jack is trying to highlight is the importance of consistency. It seems that all modules currently follow the same pattern, with mutators and validators placed under the webhook directory, and controllers under the controller directory. Personally, I lean towards keeping things as they are, which means sticking to this pattern.

However, the drawback is clear— as you mentioned, the backing image code is scattered across multiple locations. I believe this is a separate issue and might be worth opening a new discussion or issue. Additionally, as we plan to support third-party storage in more features, I believe this problem may not be limited to VM images and could affect other areas (like vm backup) as well.

Hi @brandboat,

Thanks for your feedback, however, I want to point out, that the pattern structure has not changed:

Controller: pkg/controller/master/image/vm_image_controller.go
Webhook: pkg/webhook/resources/virtualmachineimage/mutator.go and pkg/webhook/resources/virtualmachineimage/validator.go
API handler: pkg/api/image/formatter.go

the control paths for the controller, mutator, validator, and API handlers are organized as before, they just access an abstracted layer/interface/implementation from pkg/image/, which is passively invoked.

IMO, if anyone wants to add a new image solution, he doesn't need to care about the control flow for the controller, webhook, API handler, and any details about CRD VMImage (VMImage operations are encapsulated in pkg/image/common), he just needs to focus on the interfaces exported in pkg/image/. This seems simpler to me.

brandboat · 2025-01-10T08:52:55Z

the control paths for the controller, mutator, validator, and API handlers are organized as before, they just access an abstracted layer/interface/implementation from pkg/image/, which is passively invoked.

IMO, if anyone wants to add a new image solution, he doesn't need to care about the control flow for the controller, webhook, API handler, and any details about CRD VMImage, he just needs to focus on the interfaces exported in pkg/image/. This seems simpler for me.

OK, that makes sense to me. Thanks for the clarification!

pkg/image/backingimage/backing_image_controller.go

pkg/controller/master/image/vm_image_controller.go

brandboat · 2025-01-10T11:03:50Z

pkg/image/common/operator.go

+func (vmio *vmiOperator) GetName(vmi *harvesterv1.VirtualMachineImage) string {
+	return vmi.Name
+}
+
+func (vmio *vmiOperator) GetNamespace(vmi *harvesterv1.VirtualMachineImage) string {


It might be just my habit of taste, but I feel having a bunch of getters here is a bit overkill. Or do we need this for purpose?

IMO, if anyone wants to add a new image solution, he doesn't need to care about the control flow for the controller, webhook, API handler, and any details about CRD VMImage (VMImage operations are encapsulated in pkg/image/common), he just needs to focus on the interfaces exported in pkg/image/. This seems simpler to me.

As mentioned, I'd like to encapsulate the VM Image's details for any image solution (including backing image and third-party), the image implementation uses the provided libraries to operate VM Images (rather than directly manipulate the object).

That gives the integration a clear interface/concept and consistency to me. WDYT? @brandboat

My understanding is that VirtualMachineImage is similar to a DTO, and we can directly access elements like vmi.name or vmi.namespace. If there’s no additional logic in these getter methods, I’m wondering if there is much benefit in using vmio.GetName(vmi). This leaves me a bit uncertain about which approach to choose: directly invoking the DTO field (e.g., vmi.Name) or using the GetName method? I’ve noticed that some places use getters while others don’t, and I’m curious if there’s a particular reason or preference for one approach over the other.

A quick question, are there any other objects that need to implement the VMIOperator interface? Since we treat the VirtualMachineImage as a simple object to accept different parameters to connect with BackingImage and CDI, do we still need the VMIOperator interface? From my perspective, maybe we don't need to?

My understanding is that VirtualMachineImage is similar to a DTO, and we can directly access elements like vmi.name or vmi.namespace. If there’s no additional logic in these getter methods, I’m wondering if there is much benefit in using vmio.GetName(vmi). This leaves me a bit uncertain about which approach to choose: directly invoking the DTO field (e.g., vmi.Name) or using the GetName method? I’ve noticed that some places use getters while others don’t, and I’m curious if there’s a particular reason or preference for one approach over the other.

A quick question, are there any other objects that need to implement the VMIOperator interface? Since we treat the VirtualMachineImage as a simple object to accept different parameters to connect with BackingImage and CDI, do we still need the VMIOperator interface? From my perspective, maybe we don't need to?

From the original design and my opinion, for any image solution integration in pkg/image (e.q. pkg/image/backingimage and pkg/image/cdi), it should just focus on implementing interfaces exported at pkg/image/backend.

So in such a point of view, the particular image solution implementation/integration should not directly manipulate the resource VM Image. More generally speaking, it shouldn't have to care about any details in the resource VM image, instead, it only has to know the methods/interfaces (at pkg/image/common) that are provided to operate VM Images.

brandboat

I've skimmed through the code, and overall seems good to me, only a few nits left. I'll test basic VMI operations later. Thanks for the PR!

pkg/webhook/resources/virtualmachineimage/mutator.go

brandboat

LGTM, thanks!

Test below cases and all passed.

Create image through URL download
Create image through upload
Failed to create image by using invalid URL and check image retry 3 times, error message pop up correctly.
Download image from existing image

…d for various backend support Signed-off-by: Webber Huang <[email protected]>

WebberHuang1118 marked this pull request as draft December 16, 2024 07:46

WebberHuang1118 force-pushed the issue-6936 branch 5 times, most recently from 875df5f to 8f91d07 Compare December 18, 2024 09:33

WebberHuang1118 mentioned this pull request Dec 19, 2024

[ENHANCEMENT] Adding Unit Testing for VM Image #7204

Open

WebberHuang1118 marked this pull request as ready for review December 19, 2024 09:09

WebberHuang1118 requested review from FrankYang0529, Yu-Jack, bk201, Vicente-Cheng and brandboat December 19, 2024 09:10

WebberHuang1118 mentioned this pull request Dec 25, 2024

[ENHANCEMENT] VM Image Multiple Backend Support #6936

Open

brandboat reviewed Jan 3, 2025

View reviewed changes

pkg/image/backingimage/upload.go Outdated Show resolved Hide resolved

pkg/image/common/operator.go Outdated Show resolved Hide resolved

pkg/image/backingimage/upload.go Outdated Show resolved Hide resolved

WebberHuang1118 force-pushed the issue-6936 branch from 8f91d07 to 50fa656 Compare January 6, 2025 02:26

WebberHuang1118 requested a review from brandboat January 6, 2025 02:28

Yu-Jack reviewed Jan 6, 2025

View reviewed changes

brandboat reviewed Jan 10, 2025

View reviewed changes

pkg/image/backingimage/backing_image_controller.go Show resolved Hide resolved

brandboat reviewed Jan 10, 2025

View reviewed changes

WebberHuang1118 force-pushed the issue-6936 branch from 50fa656 to e84a2ec Compare January 13, 2025 02:42

brandboat reviewed Jan 13, 2025

View reviewed changes

pkg/webhook/resources/virtualmachineimage/mutator.go Outdated Show resolved Hide resolved

brandboat approved these changes Jan 14, 2025

View reviewed changes

WebberHuang1118 mentioned this pull request Jan 16, 2025

[BUG] storage class name conflicts for different namespace vmimages with a same name #5165

Open

enhancement: refactor VM Image controller, webhook and download/uploa…

b84e0e3

…d for various backend support Signed-off-by: Webber Huang <[email protected]>

WebberHuang1118 force-pushed the issue-6936 branch from e84a2ec to b84e0e3 Compare January 16, 2025 10:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VM Image Controller, Webhook and Download/Upload Refactor for Various Backend Support #7170

VM Image Controller, Webhook and Download/Upload Refactor for Various Backend Support #7170

WebberHuang1118 commented Dec 16, 2024 •

edited

Loading

brandboat left a comment

Yu-Jack left a comment •

edited

Loading

WebberHuang1118 commented Jan 6, 2025

Yu-Jack commented Jan 6, 2025 •

edited

Loading

WebberHuang1118 commented Jan 6, 2025 •

edited

Loading

brandboat commented Jan 10, 2025

WebberHuang1118 commented Jan 10, 2025 •

edited

Loading

brandboat commented Jan 10, 2025

brandboat Jan 10, 2025

WebberHuang1118 Jan 13, 2025

brandboat Jan 13, 2025

Yu-Jack Jan 13, 2025

WebberHuang1118 Jan 13, 2025 •

edited

Loading

brandboat left a comment

brandboat left a comment

VM Image Controller, Webhook and Download/Upload Refactor for Various Backend Support #7170

Are you sure you want to change the base?

VM Image Controller, Webhook and Download/Upload Refactor for Various Backend Support #7170

Conversation

WebberHuang1118 commented Dec 16, 2024 • edited Loading

brandboat left a comment

Choose a reason for hiding this comment

Yu-Jack left a comment • edited Loading

Choose a reason for hiding this comment

WebberHuang1118 commented Jan 6, 2025

Yu-Jack commented Jan 6, 2025 • edited Loading

WebberHuang1118 commented Jan 6, 2025 • edited Loading

brandboat commented Jan 10, 2025

WebberHuang1118 commented Jan 10, 2025 • edited Loading

brandboat commented Jan 10, 2025

brandboat Jan 10, 2025

Choose a reason for hiding this comment

WebberHuang1118 Jan 13, 2025

Choose a reason for hiding this comment

brandboat Jan 13, 2025

Choose a reason for hiding this comment

Yu-Jack Jan 13, 2025

Choose a reason for hiding this comment

WebberHuang1118 Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

brandboat left a comment

Choose a reason for hiding this comment

brandboat left a comment

Choose a reason for hiding this comment

WebberHuang1118 commented Dec 16, 2024 •

edited

Loading

Yu-Jack left a comment •

edited

Loading

Yu-Jack commented Jan 6, 2025 •

edited

Loading

WebberHuang1118 commented Jan 6, 2025 •

edited

Loading

WebberHuang1118 commented Jan 10, 2025 •

edited

Loading

WebberHuang1118 Jan 13, 2025 •

edited

Loading