Skip to content

CNV-1396 Cloning a DataVolume using smart-cloning#25231

Merged
bergerhoffer merged 1 commit intoopenshift:masterfrom
bgaydosrh:CNV-1396
Oct 15, 2020
Merged

CNV-1396 Cloning a DataVolume using smart-cloning#25231
bergerhoffer merged 1 commit intoopenshift:masterfrom
bgaydosrh:CNV-1396

Conversation

@bgaydosrh
Copy link
Copy Markdown

@bgaydosrh bgaydosrh commented Sep 2, 2020

Tagging @awels for code review.

Label *peer review needed" and enterprise-4.6.

Test Build: https://cnv-1396--ocpdocs.netlify.app/openshift-enterprise/latest/virt/virtual_machines/virtual_disks/virt-cloning-a-datavolume-using-smart-cloning.html

Link to new PR to fix changes suggested after merging: #26676

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 2, 2020
@openshift-docs-preview-bot
Copy link
Copy Markdown

The preview will be available shortly at:

@bgaydosrh bgaydosrh force-pushed the CNV-1396 branch 6 times, most recently from ccf3647 to bf3b13e Compare September 3, 2020 16:40
@bgaydosrh bgaydosrh changed the title CNV-1396 Cloning Virtual Disks CNV-1396 Cloning DataVolumes using smart-cloning Sep 3, 2020
@bgaydosrh bgaydosrh changed the title CNV-1396 Cloning DataVolumes using smart-cloning CNV-1396 Cloning DataVolumes Sep 3, 2020
@bgaydosrh bgaydosrh changed the title CNV-1396 Cloning DataVolumes CNV-1396 Cloning a DataVolume Sep 3, 2020
@bgaydosrh bgaydosrh changed the title CNV-1396 Cloning a DataVolume CNV-1396 Cloning a DataVolume using smart-cloning Sep 3, 2020
@bgaydosrh bgaydosrh force-pushed the CNV-1396 branch 3 times, most recently from f0f5c3a to 113a8d5 Compare September 3, 2020 18:28
@awels
Copy link
Copy Markdown

awels commented Sep 4, 2020

Looks good to me.

Copy link
Copy Markdown

@kgoldbla kgoldbla left a comment

Choose a reason for hiding this comment

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

LGTM

@bgaydosrh
Copy link
Copy Markdown
Author

bgaydosrh commented Sep 9, 2020

Both Dev and QE have signed off but the feature is still not testable.Once it is and I receive confirmation of this I can move this PR to peer review and merge.

@bgaydosrh bgaydosrh force-pushed the CNV-1396 branch 2 times, most recently from 8c186bb to 40c9d12 Compare September 22, 2020 15:45
@bergerhoffer bergerhoffer added branch/enterprise-4.6 CNV Label for all CNV PRs peer-review-needed Signifies that the peer review team needs to review this PR labels Sep 22, 2020
Comment thread _topic_map.yml Outdated
Comment thread modules/virt-cloning-a-datavolume.adoc Outdated
Comment thread modules/virt-understanding-smart-cloning.adoc Outdated
Comment thread modules/virt-cloning-a-datavolume.adoc Outdated
Comment thread modules/virt-cloning-a-datavolume.adoc Outdated
Comment thread modules/virt-understanding-smart-cloning.adoc Outdated
Comment thread modules/virt-cloning-a-datavolume.adoc Outdated
@bergerhoffer bergerhoffer added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Sep 22, 2020
@bergerhoffer bergerhoffer added this to the Future Release milestone Sep 22, 2020
@openshift-ci-robot openshift-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 24, 2020
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 24, 2020
Comment thread modules/virt-understanding-smart-cloning.adoc Outdated
Comment thread modules/virt-cloning-a-datavolume.adoc Outdated
Comment thread modules/virt-cloning-a-datavolume.adoc Outdated
Copy link
Copy Markdown
Contributor

@bergerhoffer bergerhoffer left a comment

Choose a reason for hiding this comment

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

Just two more minor things, otherwise LGTM!

Comment thread modules/virt-cloning-a-datavolume.adoc Outdated
Copy link
Copy Markdown

@kgoldbla kgoldbla left a comment

Choose a reason for hiding this comment

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

Please update the doc with changes requested by mhenricks

@bergerhoffer
Copy link
Copy Markdown
Contributor

This LGTM too now. @bgaydosrh is this ready for me to merge?

@bgaydosrh
Copy link
Copy Markdown
Author

bgaydosrh commented Oct 15, 2020 via email

@bergerhoffer bergerhoffer merged commit 839d57a into openshift:master Oct 15, 2020
@bergerhoffer
Copy link
Copy Markdown
Contributor

bergerhoffer commented Oct 15, 2020

/cherrypick enterprise-4.6

@openshift-cherrypick-robot
Copy link
Copy Markdown

openshift-cherrypick-robot commented Oct 15, 2020

@bergerhoffer: new pull request created: #26502

Details

In response to this:

/cherrypick enterprise-4.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.


== Additional resources

* xref:../../../virt/virtual_machines/cloning_vms/virt-cloning-vm-disk-into-new-datavolume.html#virt-cloning-pvc-of-vm-disk-into-new-datavolume_virt-cloning-vm-disk-into-new-datavolume[Cloning the PersistentVolumeClaim of a virtual machine disk into a new DataVolume]
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.

Hey @bgaydosrh this should link to an adoc file, not to an HTML.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed.

<3> The name of the source PVC.
<4> The size of the new DataVolume. You must allocate enough space, or the
cloning operation fails. The size must be the same as or larger than the source PVC.
ifdef::blockstorage[]
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.

@bgaydosrh - what's the usage of blockstorage attribute here (and in other code samples) for? I don't see it defined in the enclosing assembly.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I lifted the module code from something written (by Andrew perhaps?) for another assembly with the blockstorage conditional attribute appropriately defined and unset. I have added the definition and un-setting of the conditional in my new assembly for consistency.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@vikram-redhat -- if you like I can confer with Andrew if this cond is needed at all. Since I copied this module doc, I assumed it was but I can investigate now, or do it after GA. Your call.

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.

@bgaydosrh can you link to the new PR to make these fixes?

Using or not using the conditional changes the code samples so this should go through a QE review.

Copy link
Copy Markdown
Author

@bgaydosrh bgaydosrh Oct 23, 2020

Choose a reason for hiding this comment

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

Link to new PR to fix changes suggested after merging: #26670

@kgoldbla can you re-review for this change in conditionals?

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

Labels

branch/enterprise-4.6 CNV Label for all CNV PRs peer-review-done Signifies that the peer review team has reviewed this PR size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants