Several disk test improvements #824

kostyanf14 · 2025-12-17T16:19:03Z

No description provided.

gemini-code-assist · 2025-12-17T16:19:22Z

Summary of Changes

Hello @kostyanf14, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the flexibility and reliability of disk testing within the system. It empowers users to define the filesystem image format for storage driver tests via a new command-line interface option. Concurrently, it improves the robustness of disk initialization routines by relocating them to pre-test commands and incorporating error handling, which helps in preventing test failures caused by unexpected disk configurations.

Highlights

New CLI Option for Filesystem Image Format: Introduced a new command-line option, --fs-test-image-format, allowing users to specify the format (qcow2 or raw) for filesystem test images, with qcow2 as the default. This option is specifically designed for testing storage drivers.
Improved Disk Initialization for Storage Tests: Refactored the disk initialization process within vioscsi.json and viostor.json configurations. The 'Initialize Disk' step has been moved from the initial setup to pre_test_commands and now includes robust error handling (-ErrorAction SilentlyContinue; exit 0), preventing test failures due to pre-existing disk states.
Dynamic Test Image Format Handling: The storage_manager.rb now dynamically uses the specified fs_test_image_format for creating test images and generating their paths, ensuring that the correct image format is applied throughout the testing process.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces several improvements for disk-based tests. It adds a new command-line option --fs-test-image-format to allow specifying 'raw' or 'qcow2' format for filesystem test images. It also refactors the disk initialization logic for vioscsi and viostor drivers, moving it from a one-time setup step to a pre-test command to ensure a clean state for each test run.

My review has identified a couple of areas for improvement:

A potential bug in storage_manager.rb where the new image format option unintentionally affects the boot disk image format, which should likely remain fixed.
A suggestion to add input validation for the new command-line option in cli.rb to improve robustness.

gemini-code-assist · 2025-12-17T16:21:22Z

lib/setupmanagers/qemuhck/storage_manager.rb


      def create_image(path, size_gb)
-        run_cmd(*%W[#{@qemu_img_bin} create -f #{IMAGE_FORMAT} #{path} #{size_gb}G])
+        run_cmd(*%W[#{@qemu_img_bin} create -f #{@fs_test_image_format} #{path} #{size_gb}G])


The change in create_image to use @fs_test_image_format has an unintended side effect: it also changes the format of the boot image, since create_boot_image calls this method. The --fs-test-image-format option is intended only for filesystem test images.

The boot image format should likely remain fixed as 'qcow2' to ensure features like snapshots work correctly, as other parts of the code assume this format.

To fix this, you can determine the correct format inside create_image based on which image path is being used.

format = (path == @test_image_path) ? @fs_test_image_format : IMAGE_FORMAT run_cmd(*%W[#{@qemu_img_bin} create -f #{format} #{path} #{size_gb}G])

@kostyanf14 Do we care?

gemini-code-assist · 2025-12-17T16:21:22Z

lib/cli.rb

+        parser.on('--fs-test-image-format <fs_test_image_format>', String,
+                  'Filesystem test image format (qcow2/raw). Default is qcow2.',
+                  'Has effect only when testing storage drivers.',
+                  &method(:fs_test_image_format=))


To improve robustness, it's a good practice to validate user input. The --fs-test-image-format option is expected to be either 'qcow2' or 'raw'. You can use OptionParser to enforce this constraint, which will automatically provide a helpful error message for invalid values.

parser.on('--fs-test-image-format <fs_test_image_format>', %w[qcow2 raw], 'Filesystem test image format (qcow2/raw). Default is qcow2.', 'Has effect only when testing storage drivers.', &method(:fs_test_image_format=))

Copilot

Pull request overview

This PR introduces improvements to disk testing functionality by adding support for configurable filesystem test image formats and reorganizing disk initialization commands for better reliability.

Key changes include:

Adds a new --fs-test-image-format CLI option to allow users to specify the disk image format (qcow2/raw) for storage driver testing
Moves the "Initialize Disk" PowerShell command from post_start_commands to pre_test_commands in driver configurations, with improved error handling using -ErrorAction SilentlyContinue; exit 0

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
lib/cli.rb	Adds new command-line option `--fs-test-image-format` to TestOptions for specifying filesystem test image format
lib/setupmanagers/qemuhck/qemuhck.rb	Passes the new `fs_test_image_format` option through to client VM configuration
lib/setupmanagers/qemuhck/storage_manager.rb	Implements configurable test image format with fallback to default IMAGE_FORMAT constant; updates image path and creation commands
lib/engines/hcktest/drivers/viostor.json	Moves Initialize-Disk from post_start_commands to pre_test_commands with enhanced error handling
lib/engines/hcktest/drivers/vioscsi.json	Moves Initialize-Disk from post_start_commands to pre_test_commands with enhanced error handling

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-17T16:22:45Z

lib/setupmanagers/qemuhck/storage_manager.rb

      def create_image(path, size_gb)
-        run_cmd(*%W[#{@qemu_img_bin} create -f #{IMAGE_FORMAT} #{path} #{size_gb}G])
+        run_cmd(*%W[#{@qemu_img_bin} create -f #{@fs_test_image_format} #{path} #{size_gb}G])
      end


The create_image method now uses @fs_test_image_format for all image creation, but this is also called by create_boot_image (line 52). Boot images should always use the default IMAGE_FORMAT constant ('qcow2'), not the configurable @fs_test_image_format which is intended only for test images. This could cause boot image creation to fail or create incompatible boot images when a non-default format like 'raw' is specified.

Consider adding a format parameter to create_image to distinguish between boot and test image creation, or create separate methods for each use case.

YanVugenfirer

Do we care of the boot image format will change as well, or we want to switch data disk format on existing setups too?

YanVugenfirer · 2025-12-17T16:59:30Z

lib/setupmanagers/qemuhck/storage_manager.rb


      def create_image(path, size_gb)
-        run_cmd(*%W[#{@qemu_img_bin} create -f #{IMAGE_FORMAT} #{path} #{size_gb}G])
+        run_cmd(*%W[#{@qemu_img_bin} create -f #{@fs_test_image_format} #{path} #{size_gb}G])


@kostyanf14 Do we care?

Use QCOW2 as a default. Signed-off-by: Kostiantyn Kostiuk <[email protected]>

We catch issue that HLK test failed and make disk unitialized. Try to initialized disk before each tests to make sure that we have proper setup for testing. Signed-off-by: Kostiantyn Kostiuk <[email protected]>

kostyanf14 · 2025-12-24T15:48:51Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces several improvements for disk-based tests. A new command-line option, --fs-test-image-format, is added to allow specifying the disk image format (qcow2 or raw) for storage tests. The implementation correctly propagates this option down to the storage manager. Additionally, the disk initialization logic for vioscsi and viostor drivers is moved to a pre_test_command and made idempotent, which enhances test reliability by ensuring a clean disk state before each test. The changes are logical and well-executed. I have one suggestion to add input validation for the new command-line option to make it more robust.

gemini-code-assist · 2025-12-24T15:50:20Z

lib/cli.rb

+        parser.on('--fs-test-image-format <fs_test_image_format>', String,
+                  'Filesystem test image format (qcow2/raw). Default is qcow2.',
+                  'Has effect only when testing storage drivers.',
+                  &method(:fs_test_image_format=))


The new --fs-test-image-format option accepts 'qcow2' or 'raw' as valid formats. To improve robustness, it's a good practice to validate this input at the command-line parsing stage. This will provide immediate and clear feedback to the user if they supply an unsupported format, rather than letting it fail later during image creation.

OptionParser supports this directly by allowing you to pass an array of valid strings.

parser.on('--fs-test-image-format <fs_test_image_format>', %w[qcow2 raw], 'Filesystem test image format (qcow2/raw). Default is qcow2.', 'Has effect only when testing storage drivers.', &method(:fs_test_image_format=))

kostyanf14 requested review from YanVugenfirer and Copilot December 17, 2025 16:19

Copilot started reviewing on behalf of kostyanf14 December 17, 2025 16:19 View session

gemini-code-assist bot reviewed Dec 17, 2025

View reviewed changes

Copilot AI reviewed Dec 17, 2025

View reviewed changes

YanVugenfirer reviewed Dec 17, 2025

View reviewed changes

kostyanf14 added 2 commits December 24, 2025 17:48

Add ability to specifiy test image format

228c294

Use QCOW2 as a default. Signed-off-by: Kostiantyn Kostiuk <[email protected]>

Make sure that disk is initialized before each storage tests

488af82

We catch issue that HLK test failed and make disk unitialized. Try to initialized disk before each tests to make sure that we have proper setup for testing. Signed-off-by: Kostiantyn Kostiuk <[email protected]>

kostyanf14 force-pushed the fs_test_img branch from 60bb628 to 488af82 Compare December 24, 2025 15:48

gemini-code-assist bot reviewed Dec 24, 2025

View reviewed changes

Several disk test improvements #824

Are you sure you want to change the base?

Several disk test improvements #824

Uh oh!

Conversation

kostyanf14 commented Dec 17, 2025

Uh oh!

gemini-code-assist bot commented Dec 17, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

YanVugenfirer Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

YanVugenfirer left a comment

Choose a reason for hiding this comment

Uh oh!

YanVugenfirer Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

kostyanf14 commented Dec 24, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Dec 24, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants