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

MTV 1595 | PVC name template #1310

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

yaacov
Copy link
Member

@yaacov yaacov commented Jan 20, 2025

Ref: https://issues.redhat.com/browse/MTV-1595

Issue:
Users operating automation want the capability to know the disk names in the OS in the yamls for the PVC's so they can easily know which PVC is liked to specific volume in a VM.

Fix:
Adding pvcNameTemplate field to the Plan CR and to each VM in the VM list of the plan.

PVCNameTemplate is a template for generating PVC names for VM disks.
The VM specific template will override the template set in the plan.

It follows Go template syntax and has access to the following variables:
 - .VmName: name of the VM
 - .PlanName: name of the migration plan
 - .DiskIndex: initial volume index of the disk
 - .RootDiskIndex: index of the root disk
  
Examples:
   "{{.VmName}}-disk-{{.DiskIndex}}"
   "{{if eq .DiskIndex .RootDiskIndex}}root{{else}}data{{end}}-{{.DiskIndex}}"

In the plan YAML:

spec:
   ...
   pvcNameTemplate:   "{{.VmName}}-disk-{{.DiskIndex}}"

   vms:
    - name: some-vm
      pvcNameTemplate:   "{{.VmName}}-disk-{{.DiskIndex}}"

Screenshots:
pvcNameTemplate fields:
pvc-template-yaml

Running plan with custom PVC name:
pvc-template

@yaacov yaacov force-pushed the pvc-name-template branch 5 times, most recently from c617aeb to 9f8ab09 Compare January 27, 2025 08:55
@@ -512,6 +512,20 @@ spec:
- progress
type: object
type: array
pvcNameTemplate:
description: |-
Copy link
Member Author

Choose a reason for hiding this comment

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

@RichardHoch hi, can you have a look at the descriptions ?

@yaacov yaacov force-pushed the pvc-name-template branch 2 times, most recently from 391c462 to e4d4bcd Compare January 27, 2025 10:47
@yaacov yaacov marked this pull request as ready for review January 28, 2025 07:25
@yaacov yaacov requested a review from mnecas as a code owner January 28, 2025 07:25
@yaacov yaacov force-pushed the pvc-name-template branch from e4d4bcd to 9ba6ce2 Compare January 28, 2025 07:27
@yaacov yaacov force-pushed the pvc-name-template branch from 9ba6ce2 to 41902ab Compare January 28, 2025 09:45
@yaacov yaacov force-pushed the pvc-name-template branch 2 times, most recently from 54d6ca8 to 0bef440 Compare January 28, 2025 12:49
@codecov-commenter
Copy link

codecov-commenter commented Jan 28, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 40.65041% with 73 lines in your changes missing coverage. Please review.

Project coverage is 15.61%. Comparing base (f1fe5d0) to head (e0baa22).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
pkg/controller/plan/adapter/vsphere/builder.go 0.00% 55 Missing ⚠️
pkg/controller/plan/validation.go 73.52% 18 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1310      +/-   ##
==========================================
+ Coverage   15.45%   15.61%   +0.16%     
==========================================
  Files         112      112              
  Lines       23377    23511     +134     
==========================================
+ Hits         3613     3672      +59     
- Misses      19479    19551      +72     
- Partials      285      288       +3     
Flag Coverage Δ
unittests 15.61% <40.65%> (+0.16%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yaacov
Copy link
Member Author

yaacov commented Jan 28, 2025

@mnecas your suggestions worked, build pass now 🎉

@yaacov yaacov force-pushed the pvc-name-template branch 2 times, most recently from 28205d2 to dd866ed Compare January 28, 2025 13:15
@@ -27,6 +27,10 @@ const (
// Set the source PVC of the conversion, used on the DV for filtering
AnnConversionSourcePVC = "forklift.konveyor.io/conversionSourcePVC"

// Annotation that provides a template for customizing persistent volume claim names
// during the migration process. The template can reference VM and Plan fields.
AnnForkliftPVCNameTemplate = "forklift.konveyor.io/pvc-name-template"
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need the annotation? Can't we get the information directly form CR?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we can ... woo, will change and ping here ...

Copy link
Member Author

Choose a reason for hiding this comment

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

updated the code

@yaacov yaacov force-pushed the pvc-name-template branch 4 times, most recently from c5d6602 to 2d42f6a Compare January 30, 2025 10:09
@yaacov yaacov force-pushed the pvc-name-template branch from 2d42f6a to d72a95f Compare January 30, 2025 10:29
@yaacov yaacov force-pushed the pvc-name-template branch from d72a95f to 0d0ad2e Compare January 30, 2025 10:31
@yaacov yaacov changed the title MTV 1595: PVC name template MTV 1595 | PVC name template Feb 2, 2025
@yaacov yaacov force-pushed the pvc-name-template branch from 2265d73 to d768d5a Compare February 2, 2025 12:54
@yaacov
Copy link
Member Author

yaacov commented Feb 2, 2025

@mnecas hi, I refactored the PR to play nice with #1327
all new changes are in a new commit

@yaacov yaacov force-pushed the pvc-name-template branch from d768d5a to e0baa22 Compare February 2, 2025 19:40
Copy link

sonarqubecloud bot commented Feb 2, 2025

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