Skip to content

Adding supported image formats in sm-list (XCP-ng 8.3) #10

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 4 commits into
base: v25.6.0-8.3
Choose a base branch
from
Draft

Adding supported image formats in sm-list (XCP-ng 8.3) #10

wants to merge 4 commits into from

Conversation

gthvn1
Copy link

@gthvn1 gthvn1 commented May 13, 2025

This patch adds the field supported-image-format to SM object of the XAPI database. As it modifies the database we cannot have the same patch for our 8.3 and upstream because version of XAPI database or not the same. So upstream modifications conflicts with our modification. And the branch that follows the upstream one cannot be merged without conflicts.
So for testing and start review I created this patch that can be applied on our XCP-ng 8.3 branch so it is easy to test. Once reviewed I will rebase it upstream and see how we can do a PR.

@gthvn1 gthvn1 requested a review from last-genius May 13, 2025 11:44
@gthvn1 gthvn1 self-assigned this May 13, 2025
@gthvn1 gthvn1 changed the title Adding supported image formats in sm-list Adding supported image formats in sm-list (XCP-ng 8.3) May 13, 2025
last-genius
last-genius approved these changes May 13, 2025
View reviewed changes
Copy link

@last-genius last-genius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we confident about this? Or do we want to create a separate QCOW branch with this and other associated patches?

@coveralls
Copy link

coveralls commented May 13, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 15923352984

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 78.248%
Totals Coverage Status
Change from base Build 15895299829: 0.0%
Covered Lines: 3457
Relevant Lines: 4418
💛 - Coveralls

@gthvn1
Copy link
Author

gthvn1 commented May 13, 2025

I think that it should be a default feature for XCP-ng as XO will display this information. And it will probably be used to allow the selection of a file format when the user creates a VDI. So I think that we don't need a specific branch for this. I mean we need to provide this feature.

@gthvn1 gthvn1 mentioned this pull request May 21, 2025
Closed
@gthvn1 gthvn1 force-pushed the gtn-supported-image-formats-8.3 branch 8 times, most recently from fca1aa5 to afc2368 Compare June 2, 2025 15:46
@gthvn1 gthvn1 force-pushed the gtn-supported-image-formats-8.3 branch 5 times, most recently from 9f45b71 to 526ccd6 Compare June 13, 2025 11:09
@gthvn1 gthvn1 force-pushed the gtn-supported-image-formats-8.3 branch 4 times, most recently from aa7e2dd to b83f906 Compare June 23, 2025 13:59
gthvn1
gthvn1 commented Jun 23, 2025
View reviewed changes
gthvn1
gthvn1 commented Jun 23, 2025
View reviewed changes
ocaml/xapi/xapi_vm_migrate.ml
raise
Api_errors.(
Server_error
( vdi_incompatible_type
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we return incompatible type that makes sense I think but we can also create a new kind of error.

@gthvn1 gthvn1 requested a review from last-genius June 23, 2025 15:44
last-genius
last-genius approved these changes Jun 24, 2025
View reviewed changes
Copy link

@last-genius last-genius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from the comments, looks good to me overall. Would also be great if you could split the second commit into two:

  1. vdi_pool_migrate: Add the destination_format parameter
  2. vm.migrate_send: Add the vdi_format_map parameter

These are independent pieces of work and it would be great to be able to revert them separately, if needed.

The first commit title could also be changed to something like:

datamodel: Add supported_image_formats field to the SM object

Since the fact that the new field shows up in sm-list is really a consequence of this

@gthvn1
Copy link
Author

gthvn1 commented Jun 25, 2025

Aside from the comments, looks good to me overall. Would also be great if you could split the second commit into two:

1. `vdi_pool_migrate: Add the destination_format parameter`

2. `vm.migrate_send: Add the vdi_format_map  parameter`

In fact I think I will split it in the other way because vdi.pool_migrate uses vm.migrate_send. So I think it makes more sense to start with vm migration and then vdi pool migrate.

@gthvn1
Copy link
Author

gthvn1 commented Jun 25, 2025 •
edited
Loading

Note: I set this PR back to draft because the goal, once we agree that it looks correct, is to rebase it upstream and propose it upstream. Here it is easier for testing and review to be based on XCP-ng 8.3 but the goal is to push it upstream at the end.

@gthvn1 gthvn1 marked this pull request as draft June 25, 2025 08:23
@gthvn1 gthvn1 force-pushed the gtn-supported-image-formats-8.3 branch 5 times, most recently from 1edd1a2 to 167884d Compare June 26, 2025 07:37
@gthvn1 gthvn1 requested a review from last-genius June 26, 2025 13:18
@gthvn1 gthvn1 force-pushed the gtn-supported-image-formats-8.3 branch from 485a767 to bb34238 Compare June 26, 2025 13:28
last-genius
last-genius approved these changes Jun 26, 2025
View reviewed changes
Copy link

@last-genius last-genius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Could probably split the schema hash and datamodel version bump into a separate commit since they do not really only belong to the first one

@last-genius
Copy link

And let's see what the upstream says :)

gthvn1 added 4 commits June 27, 2025 11:39
@gthvn1
Datamodel: add supported_image_format field to SM object
a5320ac 
When running `xe sm-list params=all` you will now have the info of
supported image formats if the SM plugin specified it in its DRIVER_INFO.
The field is called `supported-image-formats`. If the plugin doesn't
provide the info the field will be empty.

This patch modifies the datamodel and add a new field to store this
information into the SM object.

Signed-off-by: Guillaume <guillaume.thouvenin@vates.tech>
@gthvn1
Allow selection of image format during VM migration
08918f1 
This patch allows specifying the destination format for individual VDIs
mapped to a destination SR. It adds a new parameter to `VM.migrate_send`
and `VM.assert_can_migrate` API. It also adds a new parameter to XE CLI.
The format to specify the image format is `image-format:<source VDI
UUID>=<destination image format>`. If the given image format cannot be
validated, an error is returned.

Signed-off-by: Guillaume <guillaume.thouvenin@vates.tech>
@gthvn1
Allow selection of image format during VDI pool migration
9b1eec1 
This patch add a new parameter to `VDI.pool-migrate`. This new parameter
allows to provide a string that is the destination format. This string is used
to check whether the destination SR supports the expected format. If the check
fails or cannot be performed due to missing information on the destination SR,
an error is returned.

Signed-off-by: Guillaume <guillaume.thouvenin@vates.tech>
@gthvn1
Bumping database schema version
3a0e8fd 
A new field supported_image_format has been added to SM object and a new field
has been added to:
  - VM.migrate_send
  - VM.assert_can_migrate
  - VDI.pool_migrate

Signed-off-by: Guillaume <guillaume.thouvenin@vates.tech>
@gthvn1 gthvn1 force-pushed the gtn-supported-image-formats-8.3 branch from 4b03dc3 to 3a0e8fd Compare June 27, 2025 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@last-genius last-genius last-genius approved these changes

Assignees

@gthvn1 gthvn1

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gthvn1 @coveralls @last-genius