Repository navigation
feat(instanceV2) - Add imageName and ImageNameRef to the blockDevice … - #177
Conversation
|
@Avatos Please check the PR when you have time, thanks |
3a2662d to
2297c8a
Compare
|
@lappesj Hello, maybe you can look at the my PR please. |
|
Hey @JRaver, I found an issue when blockDevice.imageName is set in initProvider and late initialization is enabled. After creation, late initialization copies the resolved image UUID into forProvider. If the instance is later deleted, Crossplane tries to recreate it using both the original image name and that UUID. Creation then fails with: I reproduced this for both cluster-scoped and namespaced resources. Could we skip late-initializing blockDevice when it’s already set in initProvider? Upjet’s ConditionalIgnoredFields looks suitable for this. A regression test covering recreation would help too. |
2297c8a to
7af7246
Compare
|
Hello, @lappesj. Thank you for catching this! The issue was a late-initialization race: after creation, upjet copied the resolved image UUID from the observed Terraform state into Fixed in two layers:
A regression test ( Please check it now, when you will have time, thanks |
7af7246 to
76e9401
Compare
|
Thanks for the update, @JRaver . I tested commit 76e9401. The original recreation test now passes for cluster-scoped and namespaced resources. I found two remaining problems. 1. The UUID exclusion also affects devices without imageNameThe new late-initialization filter excludes block_device.uuid for all block devices. For an existing UUID-based device, I tested these steps:
Late initialization restores the other device fields, but not the UUID. The next diff requires instance replacement because the desired UUID is absent. Upjet refuses this update. I reproduced this for volume and snapshot devices in both scopes. The same tests pass with the previous late-initialization code. Could you limit the UUID exclusion to the image-name case? 2. The fallback does not correct the stored UUIDThe fallback replaces the UUID in the creation data. It does not change the UUID stored in spec.forProvider. If imageName resolves to a different UUID, creation succeeds. However, the next reconciliation still requests the stored UUID. This difference requires instance replacement, which Upjet refuses. This can affect an existing resource if an image is replaced and the new image has the same name. Could you handle the stored UUID conflict and extend the regression test to check the next diff after creation? I reproduced this problem in both scopes. |
…sections Signed-off-by: Yevhen Artemenko <eartemenko@playtika.com>
76e9401 to
903b16a
Compare
lappesj
left a comment
There was a problem hiding this comment.
Reviewed and tested 903b16a. Both previously reported late-initialization/recreation issues are fixed in cluster and namespaced scope. Full Go tests, go vet, provider/generator builds, and focused race/regression tests passed. CI coverage for config/common/blockdevice will be handled separately.
|
Thanks you @lappesj for your help). Waiting for the release ) |
|
Thanks for contributing @JRaver . It's released as https://github.com/crossplane-contrib/provider-openstack/releases/tag/v0.12.0 |
PR: Add
imageNametoblockDevicefor InstanceV2Description of your changes
Adds
imageName/imageNameRef/imageNameSelectorsupport toblockDevice[]entries ofInstanceV2(both cluster and namespaced scope).Background
The upstream Terraform provider resolves
image_name→ image UUID only for the top-levelimage_namefield (ephemeral-disk boot). For boot-from-volume (block_device[].uuid,source_type: image,destination_type: volume) the provider requires a raw image UUID. This made it impossible to write portable manifests across clusters where the same image has different UUIDs.What this PR does
New field
blockDevice[].imageNameImageV2Client+imagesutils.IDFromName(Glance API), without any changes to the upstream Terraform provider.uuid; only valid forsource_type: image.ForceNew: true— changing the name is detected as drift and blocks the update (refuse to update … requires replacing it), consistent with the existinguuidbehaviour.Drift detection via
ReadContextwrapperRead, the provider looks up the current name of the boot-volume image (by UUID) in Glance and writes it back to the Terraform state asimage_name.CustomizeDiffcompares the config name with the observed name. A change produces aForceNewdiff that upjet refuses to apply, surfacing a clear error.imageNameRef/imageNameSelector— optional; resolves the name from aImageV2managed resource usingExtractParamPath("name", false). Works with both fully-managed andmanagementPolicies: [Observe]images (for the latter, setspec.forProvider.nameexplicitly).Late-initialize guard —
BlockDevice.ImageNameis added toLateInitializer.IgnoredFieldsso it is never overwritten by the observed UUID during late-init.Usage examples
Direct name (no ImageV2 object required):
With reference to an observe-only ImageV2:
I have
make submodules; make reviewable testto ensure this PR is ready for review.How has this code been tested
go test ./config/common/blockdevice/...— unit tests covering:image_namefield present,DiffSuppressFuncset onuuid)CreateContextwrapper: name resolved to UUID before upstream createuuid+imageNameconflict, wrongsource_type, image not found, multiple images with same nameReadContextwrapper: image name populated from Glance after readmake generate— verified generated types (ImageName,ImageNameRef,ImageNameSelectorinblockDevicefor both cluster and namespaced scope), resolvers, deepcopy, CRDsmake reviewable test—0 issues, all unit tests passInstanceV2withimageName: Ubuntu_2404_openstackcreated successfully;status.atProvider.blockDevice[0].imageNameshows the resolved name after first reconcileimageNameto a different image name →Synced=False, errorrefuse to update … cannot change the value of the argument "block_device.0.image_name"— server untouchedimageNameRefpointing to an observe-onlyImageV2withforProvider.nameset — resolves correctly