test(integration): support v2 volume test on test_provisioner.py #2308
Conversation
WalkthroughThe changes adjust test logic in two files. In the common module, the assertion in the Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test Function
participant F as node_default_tags Fixture
participant N as Node
T->>F: Invoke fixture
F->>F: Check DATA_ENGINE
alt DATA_ENGINE=="v2"
F->>N: Assert 2 disks
else
F->>N: Assert 1 disk
end
sequenceDiagram
participant TP as test_provisioner_params
participant C as Common Module (DATA_ENGINE)
participant SC as Storage Class
TP->>C: Import DATA_ENGINE
C-->>TP: Provide DATA_ENGINE value
TP->>SC: Add 'dataEngine' to parameters
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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 (2)
manager/integration/tests/common.py (2)
332-336: Consider using constants for better maintainability.The data engine selection logic is clear, but consider defining constants for the engine types to avoid magic strings.
+DATA_ENGINE_V1 = "v1" +DATA_ENGINE_V2 = "v2" enable_v2 = os.environ.get('RUN_V2_TEST') if enable_v2 == "true": - DATA_ENGINE = "v2" + DATA_ENGINE = DATA_ENGINE_V2 else: - DATA_ENGINE = "v1" + DATA_ENGINE = DATA_ENGINE_V1🧰 Tools
🪛 Ruff (0.8.2)
333-336: Use ternary operator
DATA_ENGINE = "v2" if enable_v2 == "true" else "v1"instead ofif-else-blockReplace
if-else-block withDATA_ENGINE = "v2" if enable_v2 == "true" else "v1"(SIM108)
1687-1690: Enhance assertion error message for disk count check.The disk count assertion is correct but could benefit from a more descriptive error message to help with debugging.
- if DATA_ENGINE == "v2": - assert len(node.disks) == 2 - else: - assert len(node.disks) == 1 + expected_disk_count = 2 if DATA_ENGINE == "v2" else 1 + assert len(node.disks) == expected_disk_count, \ + f"Expected {expected_disk_count} disk(s) for {DATA_ENGINE} engine, " \ + f"but got {len(node.disks)} disk(s)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
manager/integration/tests/common.py(1 hunks)manager/integration/tests/test_provisioner.py(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build images
- GitHub Check: Summary
🔇 Additional comments (3)
manager/integration/tests/test_provisioner.py (3)
12-12: LGTM!The import of
DATA_ENGINEfrom the common module is correctly placed and used in the storage class parameters.
31-31: LGTM! Test categorization is correctly implemented.The
@pytest.mark.v2_volume_testdecorator is consistently applied to all relevant test functions, aligning with the PR objective of supporting v2 volume testing.Also applies to: 87-87, 132-132, 174-174, 226-226
112-116: Verify the impact of the new dataEngine parameter.The addition of
dataEngineparameter to the storage class is consistent with v2 volume support. However, let's verify its usage and potential impact.Run the following script to check how the
dataEngineparameter is used across the codebase:✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check usage of dataEngine parameter in storage class definitions and related code # Search for dataEngine parameter usage rg -A 5 'dataEngine' # Search for storage class parameter handling ast-grep --pattern $'storage_class.*parameters.*=.*{ $$$ }'Length of output: 65699
The new
dataEngineparameter is consistently integrated across the codebase.
- The shell script search reveals that the
dataEngineparameter is passed from theDATA_ENGINEvariable in tests such astest_provisioner.py,test_csi.py,test_basic.py, and many others.- It is also used in the storage class creation functions in modules like
e2e/libs/storageclass/storageclass.pyand related volume management implementations.- This wide usage indicates that the parameter is well-integrated and its impact has been accounted for across volume provisioning, stateful set handling, and other related functionalities.
manager/integration/tests/common.py
Outdated
| if DATA_ENGINE == "v2": | ||
| assert len(node.disks) == 2 | ||
| else: | ||
| assert len(node.disks) == 1 |
There was a problem hiding this comment.
Could you leave a comment explaining why the len(node.disks) is 2 while enabling v2 data engine?
There was a problem hiding this comment.
Updated, thank you.
chriscchien
force-pushed
the
v2_provisioner_test
branch
from
53ce038 to
71096af
Compare
manager/integration/tests/common.py
Outdated
| @@ -1684,7 +1684,12 @@ def node_default_tags(): | |||
|
|
|||
| tag_mappings = {} | |||
| for tags, node in zip(DEFAULT_TAGS, nodes): | |||
| assert len(node.disks) == 1 | |||
| if DATA_ENGINE == "v2": | |||
| # if v2 data engine is enabled, both a file system disk | |||
There was a problem hiding this comment.
Can you elaborate on the purpose? For v2 backing image, right?
There was a problem hiding this comment.
Yes, I have updated the comment to below, because a v2 volume use block disk but a v2 backing image can not function without file system disk so the two types of disks need to coexist if we want to test v2 volume with v2 backing image.
# if the v2 data engine is enabled, both a file system disk
# and a block disk will coexist. This is because a v2 backing image
# requires a file system disk to function.
71096af to
95c2516
Compare
ref: longhorn/longhorn 9760 Signed-off-by: Chris Chien <chris.chien@suse.com>
derekbit
force-pushed
the
v2_provisioner_test
branch
from
95c2516 to
1358f84
Compare
|
@mergify backport v1.8.x |
✅ Backports have been created
|
Which issue(s) this PR fixes:
Issue #9760
What this PR does / why we need it:
support v2 volume test on test_provisioner.py
Special notes for your reviewer:
Test result
Additional documentation or context
N/A
Summary by CodeRabbit