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-1388 | Add dynamic way to specify the virt-customize #1017

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

mnecas
Copy link
Member

@mnecas mnecas commented Sep 4, 2024

Issue:

Right now all the run/firstboot scripts are located inside the conversion pod. To get new scripts to the users we need some alternative way to allow users to specify the scripts themselves. This will speed up the migration process as the users won't need to wait for the build and release of the patch. Additionally, the users themselves can tweak the scripts depending on their needs.

Design:

We have two options use the config maps, mount it to the container and read the mounted directory depending on their name we do virt-customize with them. An alternative solution would be to create CRD with all configurations such as action, path etc. I have chosen first as this is needed as soon as possible and configmaps are faster to implement.

User flow:

  1. Create configmap:
    The user needs to create a config map inside the namespace where they want to migrate the VM. The config map needs to be named forklift-virt-customize.
  2. Populate configmap key and value:
    Inside the configmap user can specify data with key/value definition. The value is the script itself which they want to run. The key needs to follow regex ([0-9]+_win_firstboot(([\w\-]*).ps1))$ or ^([0-9]+_linux_(run|firstboot)(([\w\-]*).sh))$ depending on where the customer wants the script to run.
    For example 00_win_firstboot_test.ps1 will specify that the data should be interpreted as PowerShell script which should start at boot. Alternatively, the Linux option has not only the firstboot but also the run option. This will be applied on the VM after virt-v2v conversion, but before the VM is started.
    Note: The number in the beginning of the key sets the order.

Conversion pod flow:

  1. The forklift-controller checks if there is a config map inside the namespace. If the config map is present it is automatically mounted to the conversion pod at /mnt/dynamic_scripts.
  2. The conversion pod checks if the /mnt/dynamic_scripts directory is present and if it is, it checks the files and their regexes.
  3. Depending on the VM operating system it will use different regex. From the filename, the conversion pod determines the action required on the virt-customize step.

Additional changes:

  • I have moved the static variables into a single file as we are using more and more and it's hard to keep track.
  • New batch file which will run all PowerShell scripts inside the windows first boot scripts dir.
  • The users can additionally also edit the virt customize config map name in the controller using the variable in controller virt_customize_configmap_name

@mnecas mnecas requested a review from yaacov as a code owner September 4, 2024 17:13
@mnecas mnecas force-pushed the MTV-1388 branch 7 times, most recently from 47b461a to 84c9642 Compare September 6, 2024 07:41
Copy link

codecov bot commented Sep 6, 2024

Codecov Report

Attention: Patch coverage is 12.50000% with 112 lines in your changes missing coverage. Please review.

Project coverage is 16.41%. Comparing base (26406f2) to head (84c9642).

Files with missing lines Patch % Lines
pkg/controller/plan/kubevirt.go 0.00% 41 Missing ⚠️
virt-v2v/cold/customize-win.go 0.00% 35 Missing ⚠️
virt-v2v/cold/customize-image.go 37.50% 20 Missing ⚠️
virt-v2v/cold/customize-rhel.go 21.05% 12 Missing and 3 partials ⚠️
virt-v2v/cold/entrypoint.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1017      +/-   ##
==========================================
- Coverage   16.43%   16.41%   -0.03%     
==========================================
  Files         107      108       +1     
  Lines       19642    19723      +81     
==========================================
+ Hits         3229     3238       +9     
- Misses      16126    16195      +69     
- Partials      287      290       +3     
Flag Coverage Δ
unittests 16.41% <12.50%> (-0.03%) ⬇️

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.

@mnecas
Copy link
Member Author

mnecas commented Sep 17, 2024

Tested config map:
image

Result in the VM:
image

@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2024

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

Codecov Report

Attention: Patch coverage is 15.23810% with 89 lines in your changes missing coverage. Please review.

Project coverage is 16.25%. Comparing base (c8a80f1) to head (b82936b).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/controller/plan/kubevirt.go 0.00% 34 Missing ⚠️
virt-v2v/pkg/customize/windows.go 0.00% 24 Missing ⚠️
virt-v2v/pkg/customize/rhel.go 19.04% 14 Missing and 3 partials ⚠️
virt-v2v/pkg/customize/utils.go 48.00% 13 Missing ⚠️
virt-v2v/pkg/customize/image.go 0.00% 1 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1017      +/-   ##
==========================================
- Coverage   16.34%   16.25%   -0.10%     
==========================================
  Files         110      111       +1     
  Lines       19645    19750     +105     
==========================================
- Hits         3211     3210       -1     
- Misses      16154    16259     +105     
- Partials      280      281       +1     
Flag Coverage Δ
unittests 16.25% <15.23%> (-0.10%) ⬇️

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.

@mnecas mnecas force-pushed the MTV-1388 branch 2 times, most recently from a963aa1 to da23443 Compare September 17, 2024 15:34
@mnecas mnecas force-pushed the MTV-1388 branch 2 times, most recently from 3ed9a8e to b178e95 Compare September 18, 2024 07:14
Issue:
Right now all the run/firstboot scripts are located inside the
conversion pod. To get new scripts to the users we need some alternative
way to allow users to specify the scripts themselves. This will speed
up the migration process as the users won't need to wait for the build
and release of the patch. Additionally, the users themselves can tweak
the scripts depending on their needs.

Design:
We have two options use the config maps, mount it to the
container and read the mounted directory depending on their name we
do virt-customize with them. An alternative solution would be to create CRD
with all configurations such as `action`, `path` etc. I have chosen first
as this is needed as soon as possible.

User flow:
1. The user needs to create a config map inside the namespace where they want to
   migrate the VM. The config map needs to be named `mtv-virt-customize`.
2. Inside the configmap user can specify data with key/value definition.
   The value is script itself which they want to run.
   The key needs to follow regex `^([0-9]+_win_firstboot(([\w\-]*).ps1))$` or
   `^([0-9]+_linux_(run|firstboot)(([\w\-]*).sh))$` depending on where the customer
   wants the script to run.
   For example `00_win_firstboot_test.ps1` will specify that the data
   should be interpreted as PowerShell script which should start at boot.
   Alternatively, the Linux option has not only the `firstboot` but also
   the `run` option. This will be applied on the VM after virt-v2v conversion,
   but before the VM is started.
   Note: The number in the begining of the key sets the order.

Conversion pod flow:
1. The `forklift-controller` checks if there is a config map inside the
   namespace. If the config map is present it is automatically mounted to
   the conversion pod at `/mnt/dynamic_scripts`.
2. The conversion pod checks if the `/mnt/dynamic_scripts` directory is
   present and if it is, it checks the files and their regexes.
   Depending on the VM operating system it will use different. From the
   filename the conversion pod determines the action required on the
   `virt-customize` step.

Additional changes:
- I have moved the static variables into a single file as we are using
  more and more and it's hard to keep track.
- New batch file which will run all PowerShell scripts inside the
  windows first boot scripts dir.

Signed-off-by: Martin Necas <[email protected]>
Copy link

Copy link
Member

@yaacov yaacov left a comment

Choose a reason for hiding this comment

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

LGTM

@mnecas mnecas merged commit 650d73d into kubev2v:main Sep 18, 2024
13 checks passed
@mnecas
Copy link
Member Author

mnecas commented Oct 14, 2024

Hi @jsakil14,
the scripts to enable the drivers are already implemented in the latest forklift
please rebuild the virt-v2v+controller image and apply it to your operator.
https://github.com/kubev2v/forklift/blob/main/virt-v2v/pkg/customize/scripts/windows/9999-restore_config.ps1
I need to do a forklift release, I'll try to make 2.7.2.

The config looks correct which version of forklift are you using?

@mnecas
Copy link
Member Author

mnecas commented Oct 14, 2024

Don't rebuild anything, but use the latest images from https://quay.io/organization/kubev2v/

@mnecas
Copy link
Member Author

mnecas commented Oct 14, 2024

you can replace the image in the CSV and the operator will do the rest
ohh customize to remote cluster, it should be done on that cluster and in namepsace where you are migrating to... that said I have not tested it to remote clusters so might not be working correctly

@mnecas
Copy link
Member Author

mnecas commented Oct 14, 2024

you are correct only in the admin cluster, from there we create the pods on the remote clusters, but the configurations and images are specified in the master

@jsakil14
Copy link

@mnecas question reg-configmap - Can I have both linux & windows scripts in a single CM? will the v2v pick up the right one based on the OS flavour?

@jsakil14
Copy link

@mnecas question reg-configmap - Can I have both linux & windows scripts in a single CM? will the v2v pick up the right one based on the OS flavour?

BUMP! @mnecas

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.

4 participants