test(robot): support v2 volume for backing image related test cases #2345
Conversation
Signed-off-by: Yang Chiu <yang.chiu@suse.com>
WalkthroughThe changes update the parameter names in several Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test Script
participant K as Backing Image Keywords
participant BI as Backing Image Client
participant API as Longhorn API
T->>K: create_backing_image(name, url, expectedChecksum, dataEngine, minNumberOfCopies)
K->>BI: create(name, sourceType, url, expectedChecksum, dataEngine, minNumberOfCopies)
BI->>API: get_longhorn_client().create_backing_image(...)
API-->>BI: Return backing image details
BI-->>K: Confirmation response
K-->>T: Backing image created
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
e2e/libs/keywords/backing_image_keywords.py (1)
12-43: Consider updating parameter names in other methods for consistencyMethods like
all_disk_file_status_are_ready,clean_up_backing_image_from_a_random_disk, anddelete_backing_imagestill usebi_nameas parameter name, while thecreate_backing_imagemethod now usesname. This inconsistency could be confusing and might lead to maintenance issues in the future.- def all_disk_file_status_are_ready(self, bi_name): - self.backing_image.all_disk_file_status_are_ready(bi_name) + def all_disk_file_status_are_ready(self, name): + self.backing_image.all_disk_file_status_are_ready(name) - def clean_up_backing_image_from_a_random_disk(self, bi_name): - self.backing_image.clean_up_backing_image_from_a_random_disk(bi_name) + def clean_up_backing_image_from_a_random_disk(self, name): + self.backing_image.clean_up_backing_image_from_a_random_disk(name) - def delete_backing_image(self, bi_name): - self.backing_image.delete(bi_name) + def delete_backing_image(self, name): + self.backing_image.delete(name)Similar changes would be needed in the
Baseclass and its other implementations. Consider creating a follow-up PR to address these inconsistencies.e2e/libs/backing_image/rest.py (2)
30-32: Unused loop control variableThe loop variable
iis defined but not used within the loop body.- for i in range(self.retry_count): + for _i in range(self.retry_count):🧰 Tools
🪛 Ruff (0.8.2)
30-30: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
50-51: Inconsistent parameter naming convention in remaining methodsWhile the
createmethod has been updated to use camelCase parameter names, other methods in this file still use snake_case (e.g.,bi_nameinget,delete, etc.). It would be more consistent to update all methods to use the same naming convention.- def get(self, bi_name): - return get_longhorn_client().by_id_backing_image(bi_name) + def get(self, name): + return get_longhorn_client().by_id_backing_image(name)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e/libs/backing_image/backing_image.py(1 hunks)e2e/libs/backing_image/base.py(1 hunks)e2e/libs/backing_image/crd.py(1 hunks)e2e/libs/backing_image/rest.py(2 hunks)e2e/libs/keywords/backing_image_keywords.py(1 hunks)e2e/tests/negative/component_resilience.robot(3 hunks)e2e/tests/regression/test_backing_image.robot(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
e2e/libs/backing_image/rest.py
30-30: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build images
- GitHub Check: Summary
🔇 Additional comments (15)
e2e/libs/backing_image/crd.py (1)
17-17:❓ Verification inconclusive
Method signature updated to use camelCase naming
The parameter names have been changed from snake_case to camelCase and
bi_namehas been simplified toname. This change aligns with the PR objective to support v2 volume for backing image test cases.
🏁 Script executed:
#!/bin/bash # Check if parameter renaming is consistent across all files in the backing_image module echo "Checking for any remaining instances of the old parameter names..." rg -g "e2e/libs/backing_image/*.py" "def create\(.*bi_name|source_type|expected_checksum|data_engine|min_copies"Length of output: 257
CamelCase Parameter Update Verification
The
createmethod ine2e/libs/backing_image/crd.pynow correctly uses camelCase naming. The old parameter names (e.g.bi_name,source_type,expected_checksum,data_engine, andmin_copies) no longer appear according to the automated check. Please manually verify that no residual references to these snake_case names exist in the backing image module.
- File: e2e/libs/backing_image/crd.py (line 17)
- Change: Method signature updated from using snake_case (and
bi_name) to camelCase (andname) to align with the PR objective for supporting v2 volume backing image tests.e2e/libs/backing_image/backing_image.py (1)
16-18: Method signature and implementation updated to use camelCase namingThe method has been properly updated to use camelCase for both parameter names and internal variables, maintaining consistency with the interface changes in the
Baseclass. The parameterbi_namehas been simplified toname, and the variablesource_typehas been renamed tosourceTypeinside the method body.e2e/libs/backing_image/base.py (1)
11-11: Abstract method signature updated to camelCase conventionThe
createmethod's parameters in the abstract base class have been updated to use camelCase naming, which sets the standard interface for all derived classes. This is correctly aligned with the PR objective to support v2 volume for backing image test cases.e2e/libs/keywords/backing_image_keywords.py (1)
9-10:❓ Verification inconclusive
Method updated for consistency with backing_image module changes
The
create_backing_imagemethod has been properly updated to reflect the parameter changes in the backing image module. Default values for the new parameter names have been preserved, maintaining backward compatibility.
🏁 Script executed:
#!/bin/bash # Check for any missed parameter renaming in method calls echo "Checking if all calling code has been updated..." rg -g "e2e/tests/**/*.robot" "create_backing_image.*min_copies|data_engine"Length of output: 184
Verified update – manual check required
The changes to the
create_backing_imagemethod ine2e/libs/keywords/backing_image_keywords.pyappear consistent with the updated parameter ordering in the backing image module. The code now passes the parameters as follows:def create_backing_image(self, name, url, expectedChecksum="", dataEngine="v1", minNumberOfCopies=1): self.backing_image.create(name, url, expectedChecksum, dataEngine, minNumberOfCopies)A shell script was run to search for any remaining references to the old parameter names (
min_copies/data_engine) in the test files, but no output was produced. Since the absence of output lowers confidence in the inference, please manually verify that all calling code is consistent with this update.e2e/tests/negative/component_resilience.robot (5)
61-66: Backing image testing now supports both v1 and v2 data enginesThe removal of conditional statements ensures that backing image tests run for both v1 and v2 data engines, which aligns with the PR objective to support v2 volume for backing image related test cases.
67-70: Consistent testing for persistent volumes based on backing imagesThe storage class and persistent volume claim creation now runs unconditionally for all data engine types, which provides better test coverage and ensures backing image functionality is verified in both v1 and v2 data engines.
88-90: Data integrity checks now run universallyThe checks for volume data integrity and deployment stability are now performed regardless of the data engine type, which strengthens the test verification of backing image functionality.
127-139: Backing Image Volume Recovery test now supports all data enginesThe Backing Image Volume Recovery test now runs for all data engine types, which is an important addition to ensure the reliability of backing images with v2 volumes.
154-169:❓ Verification inconclusive
Inconsistent conditional check for RWX Volume Recovery test
This test still contains a conditional check for v1 data engine, while other tests have had their conditionals removed. This inconsistency should be addressed - either remove this conditional to support both data engines or document why this particular test is limited to v1.
🏁 Script executed:
#!/bin/bash # Check if RWX volumes are supported in v2 engine in Longhorn rg -i "rwx.*v2|v2.*rwx" --type=allLength of output: 36
Clarify RWX Volume Recovery Test Conditional
It appears that in
e2e/tests/negative/component_resilience.robotthe test still checks if${DATA_ENGINE}equals'v1'before running RWX volume operations—even though similar tests have dropped such conditionals. Our search for any indication that RWX volumes are supported in a v2 engine using the command did not produce any output.Please verify manually whether RWX functionality is intended to work with the v2 data engine. If RWX volumes are supported on v2, consider removing the conditional or updating the test to accommodate both engines. If not, please document why this test is limited to v1.
e2e/libs/backing_image/rest.py (3)
15-15: Method signature updated to use camelCase naming conventionThe parameter naming has been updated from snake_case to camelCase to maintain consistency with other parts of the codebase. This change improves the overall code consistency.
18-26: Parameter names in API call updated to match new naming conventionThe parameter names in the create_backing_image API call have been appropriately updated to maintain consistency with the method signature changes.
42-44: Updated assertion parameter names to match camelCase conventionThe parameter names in the assertions have been correctly updated to match the new camelCase naming convention.
e2e/tests/regression/test_backing_image.robot (3)
20-20: Added data engine parameter to backing image creationThe
dataEngine=${DATA_ENGINE}parameter has been added to the backing image creation command, which allows the test to properly utilize the specified data engine.
36-37: Removed skip tag from backing image uninstall testThe
robot:skiptag has been removed, indicating that this test should now run as part of the regular test suite. This increases test coverage for backing images with both v1 and v2 data engines.
42-43: Updated parameter naming and added data engine support for backing image testsThe parameters have been updated to use camelCase naming convention (
minNumberOfCopiesinstead ofmin_copies) and added thedataEngine=${DATA_ENGINE}parameter, ensuring proper handling of different data engine types.
Which issue(s) this PR fixes:
Issue longhorn/longhorn#10001
What this PR does / why we need it:
support v2 volume for backing image related test cases
Special notes for your reviewer:
Additional documentation or context
Summary by CodeRabbit
Refactor
Tests