Skip to content

fix: update volume metadata when skipping V2V conversion#218

Open
miguelntt wants to merge 5 commits intoos-migrate:mainfrom
miguelntt:fix/skip-conversion-volume-metadata
Open

fix: update volume metadata when skipping V2V conversion#218
miguelntt wants to merge 5 commits intoos-migrate:mainfrom
miguelntt:fix/skip-conversion-volume-metadata

Conversation

@miguelntt
Copy link
Copy Markdown

Summary

When skip_conversion: true is set (useful for appliances or OSes not recognized by virt-v2v), the disk copy via nbdcopy completes successfully but the Cinder volume metadata was never updated to converted=true. Ansible reads this metadata to decide whether to create the instance in OpenStack, so it was silently skipping instance creation after an otherwise successful migration.

Root cause

In migrate.go, the else branch (when runV2V=false) only logged a message but never called UpdateVolumeMetadata. The if runV2V branch correctly sets osm=true and converted=true after conversion, but the skip path had no equivalent.

Fix

Added the same metadata update call that exists in the runV2V=true path:

} else {
    logger.Log.Infof("Skipping V2V conversion...")
    volMetadata = map[string]string{
        "osm":       "true",
        "converted": "true",
    }
    err = osm_os.UpdateVolumeMetadata(c.OSClient, volume.ID, volMetadata)
    if err != nil {
        logger.Log.Infof("Failed to set volume metadata: %v, ignoring ...", err)
    }
}

Both keys are required: osm=true because UpdateVolumeMetadata calls volumes.Update which replaces all metadata (not a merge), and converted=true to unblock the Ansible instance creation step.

Usage

No new parameters needed. The existing variable already works correctly with this fix:

import_workloads_skip_conversion: true

Files changed

  • plugins/modules/src/migrate/migrate.go — 8 lines added in the else branch
  • CHANGELOG.md — version entry for v2.2.2

Copy link
Copy Markdown
Contributor

@fdiazbra fdiazbra left a comment

Choose a reason for hiding this comment

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

Great catch! The fix is correct — without updating the volume metadata, Ansible can't proceed with instance creation.

The code change looks good to merge. However, please remove the changes to CHANGELOG.md. We group multiple PRs together and update the CHANGELOG when we create the release tag. This helps us maintain consistency and avoid merge conflicts.

Otherwise LGTM!

When skip_conversion is true (useful for appliances or OSes not
recognized by virt-v2v), the disk copy was completing correctly but
the Cinder volume metadata was never updated to converted=true.
Ansible reads this metadata to decide whether to create the instance,
so it was silently skipping instance creation after a successful copy.
This adds the same metadata update that exists in the normal V2V path.
@miguelntt miguelntt force-pushed the fix/skip-conversion-volume-metadata branch from a7cfda5 to 38b1348 Compare February 26, 2026 05:28
Copy link
Copy Markdown
Contributor

@fdiazbra fdiazbra left a comment

Choose a reason for hiding this comment

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

LGTM

logger.Log.Infof("Skipping V2V conversion...")
volMetadata = map[string]string{
"osm": "true",
"converted": "true",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for this PR, and you are correct, when we want to skip v2v we should mention it in the metadata, but I dont like the idea to set "converted" to true, because it's not what happened.

Maybe we can create a new flag, in order to specify that the volume is migrated and ready to be used by OpenStack, something like:
"osm_state": "ready" or "migrated"

So we need a checks also line 158, like this one:
if state == "ready" {
logger.Log.Infof("Volume already migrated and ready to be used by OpenStack, skipping...")
return volume.ID, nil
}

@miguelntt
Copy link
Copy Markdown
Author

Thanks for the suggestion! Updated to use osm_state: ready instead of converted: true, and added the check at line 158.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants