Skip to content
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.

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

CP-46179 backport for deterministic backup VDI #5552

Merged
merged 4 commits into from
Apr 11, 2024

Conversation

lindig
Copy link
Contributor

@lindig lindig commented Apr 9, 2024

We want to use a predictable VDI for metadata backup. This is the backport of that feature to Yangtze.

  • The existing shell scripts differ slightly between the branches and I've kept these changes.
  • The UUIDX module does not exist in Yangtze; I created a minimal implementation for this backport to get UUIDv5.

@lindig lindig requested review from robhoes and psafont April 9, 2024 13:34

let namespace =
let ns = "e93e0639-2bdb-4a59-8b46-352b3f408c19" in
Uuidm.(of_string ns |> Option.get)
Copy link
Member

Choose a reason for hiding this comment

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

The Uuidx library is available as Uuid in yangtze. Patching it seems more involved than doing it here, so I think this change makes more sense than backporting it to https://github.com/xapi-project/xen-api-libs-transitional/blob/2.25-lcm/uuid/uuid.ml

@@ -48,6 +48,29 @@ function usage {
exit 1
}

function uuid5 {
Copy link
Member

Choose a reason for hiding this comment

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

missing hash of original commit in the commit message: 084c6a9

@@ -74,7 +74,7 @@ function validate_vdi_uuid {
function test_sr {
sr_uuid_found=$(${XE} sr-list uuid="$1" --minimal)
if [ "${sr_uuid_found}" != "$1" ]; then
echo Invalid SR UUID specified: $1
echo "Invalid SR UUID specified: $1"
Copy link
Member

Choose a reason for hiding this comment

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

missing hash of original commit in the commit message: f88fa12

@@ -39,6 +39,7 @@ function usage {
echo " -k: Number of older backups to preserve (default: ${history_kept})"
echo " -n: Just try to find a backup VDI and stop the script after that"
echo " -f Force backup even when less than 10% free capacity is left on the backup VDI"
echo " -y: Assume non-interactive mode and yes to all questions"
Copy link
Member

Choose a reason for hiding this comment

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

missing hash of original commit in the commit message: 4fe8ffa

@psafont
Copy link
Member

psafont commented Apr 9, 2024

Backports look faithful to me, I've only got the small nit of the missing original hashes in the commit messages.

Can this be merged as is, or does it depend on some xsconsole changes?

@lindig
Copy link
Contributor Author

lindig commented Apr 9, 2024

This can be merged but should not be released as a hot fix because it might depend on xsconsole changes. Which creates the additional problem of creating an ordering problem. But I think we should at least get the back port into our repo somehow.

Backport of bbc5a1f

When creating a backup VDI on an SR we want to derive the VDI's UUID
from the SR such that we know how to find it later on the SR.

Implement a hash that derives the VDI UUID and pass this UUID to SM to
be used.

Currently the creation of this VDI in Xapi is detected based on its
name-label: "Pool Metadata Backup". The SM stack usually creates a new
random UUID but will use a given UUID passed as "vdi_uuid" for its
"vdi_create" command. The difficulty in the implementation is that
"vdi_uuid" is used in other commands as well and we have to make sure to
set it only in the intended context. It is not straight forward to pass
the UUID from Xapi_vdi.create down to Sm.

Signed-off-by: Christian Lindig <[email protected]>
@lindig lindig force-pushed the private/christianlin/1.249-lcm branch from 7ab7414 to 7d409fd Compare April 10, 2024 13:41
@lindig
Copy link
Contributor Author

lindig commented Apr 10, 2024

I've updated the commit messages to include the hashes of the original commits.

@psafont
Copy link
Member

psafont commented Apr 10, 2024

The second commit in the series (69f1fd2) doesn't point to the correct commit hash, it should point to 65f152d

Christian Lindig added 3 commits April 10, 2024 15:30
Backport of 65f152d

We introduce deterministic UUID for backup VDIs to avoid searching for
VDIs. Add this capability to the scripts that create and restore
backups. In case the VDI with the expected UUID does not exist, let the
user confirm probing for it.

Signed-off-by: Christian Lindig <[email protected]>
Backport of c2f7456

Signed-off-by: Christian Lindig <[email protected]>
Backport of 7f1d315

We have added an interactive prompt in case the backup VDI was created
by an earlier version and does not match the expected UUID. To
facilitate scripting, add a -y (yes) flag that assumes a confirmation.

Signed-off-by: Christian Lindig <[email protected]>
@lindig lindig force-pushed the private/christianlin/1.249-lcm branch from 7d409fd to f0eb272 Compare April 10, 2024 14:30
@lindig lindig requested a review from edwintorok April 10, 2024 15:49
Copy link
Contributor

@Vincent-lau Vincent-lau left a comment

Choose a reason for hiding this comment

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

Minor questions, but overall looks good

@@ -113,7 +113,23 @@ let make_call ?driver_params ?sr_sm_config ?vdi_sm_config ?vdi_type
may (fun self -> Db.VDI.get_location ~__context ~self) vdi_ref
in
let vdi_uuid =
may (fun self -> Db.VDI.get_uuid ~__context ~self) vdi_ref
let __FUNCTION__ = "Sm_exec.make_call" in
Copy link
Contributor

Choose a reason for hiding this comment

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

why redefining __FUNCTION__?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not defined in the compiler version on Yangtze. So now I don't have to change the code in the back port.

@@ -688,16 +688,31 @@ module SMAPIv1 = struct
vdi_info_from_db ~__context (Db.VDI.get_by_uuid ~__context ~uuid)

Copy link
Contributor

Choose a reason for hiding this comment

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

this storage_access.ml file corresponds to storage_smapiv1.ml in master, no smapiv3 back then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

@lindig lindig merged commit 31e1153 into xapi-project:1.249-lcm Apr 11, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants