OSAC-1699: Revert QoS policies created with wrong type for block storage tiers - #402
Conversation
|
@wgordon17: This pull request references OSAC-1699 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wgordon17 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThis PR restricts VAST QoS policy creation to non-block (VIEW/USER) tiers per VAST 5.4.x limitations, removing the ChangesBlock-tier QoS removal
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Playbook as Ansible Task
participant Filter as Tier Filter
participant VMS as VAST REST API
Playbook->>Filter: evaluate storage tiers
Filter-->>Playbook: keep tiers with qos_policy and protocol != block
Playbook->>VMS: POST qospolicies (name, tenant_id, mode: STATIC)
VMS-->>Playbook: created policy (no policy_type)
Related issues: None referenced in the provided diff. Related PRs: None referenced in the provided diff. Suggested labels: tests, vast-storage, bugfix Suggested reviewers: None specified. 🐇 A tier once tried to block its way in, 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/integration/targets/storage_provider_ensure_sc/tasks/main.yml (2)
384-411: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winQoS creation/exclusion is no longer verified via mock call log.
The previous
qospoliciesPOST call assertions (per-protocolpolicy_typeexpectations) were removed entirely rather than adjusted. Given the default tier still declaresqos_policy(line 75), the role should still POST toqospoliciesfor that tier — and critically, should make no such call for the block tier. Neither is currently verified, leaving the fixed bug (block tier wrongly getting a QoS policy) without direct test coverage.✅ Proposed fix
- name: Assert view policies were created ansible.builtin.assert: that: - _call_log | selectattr('path', 'search', 'viewpolicies') | list | length > 0 fail_msg: "No view policy API calls found — ensure_storage_class should create view policies" success_msg: "View policy API calls detected" + + - name: Assert QoS policy was created for the NFS tier only + ansible.builtin.assert: + that: + - _call_log | selectattr('path', 'search', 'qospolicies') | selectattr('method', 'equalto', 'POST') | list | length > 0 + fail_msg: "No qospolicies POST call found — ensure_storage_class should create QoS policy for the default tier" + success_msg: "QoS policy API call detected"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/targets/storage_provider_ensure_sc/tasks/main.yml` around lines 384 - 411, Restore mock call log assertions in the integration test to cover QoS policy behavior in addition to views and view policies. Update the task sequence in main.yml to verify that `ensure_storage_class` POSTs to `qospolicies` for the default tier that defines `qos_policy`, and also verify that no `qospolicies` call is made for the block tier. Use the existing `_mock_calls` / `_call_log` checks and add assertions keyed on the `path` and `method` fields so the fixed block-tier QoS regression is directly covered.
223-234: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression assertion that block SC has no
qos_policy.The block StorageClass parameter assertions dropped the
qos_policy == test-qos-blockcheck but don't add a negative assertion (parameters.qos_policy is not defined). Since this PR's entire purpose is preventingqos_policyfrom being wired into block-tier StorageClasses, this is the exact regression this test suite should guard against.✅ Proposed fix
- name: Assert block StorageClass parameters ansible.builtin.assert: that: - _block_sc_result.resources[0].parameters.vip_pool_name == "osac-test-pool" - _block_sc_result.resources[0].parameters.subsystem == "view-test-ensuresc-d0f7f176-block-tier" - "_block_sc_result.resources[0].parameters['csi.storage.k8s.io/provisioner-secret-name'] == 'vast-csi-test-ensuresc'" - "_block_sc_result.resources[0].parameters['csi.storage.k8s.io/provisioner-secret-namespace'] == 'osac-system'" - _block_sc_result.resources[0].parameters.storagePath is not defined - _block_sc_result.resources[0].parameters.viewPolicy is not defined - _block_sc_result.resources[0].parameters.secretName is not defined + - _block_sc_result.resources[0].parameters.qos_policy is not defined fail_msg: "Block StorageClass parameters incorrect: {{ _block_sc_result.resources[0].parameters }}" success_msg: "Block StorageClass parameters correct"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/targets/storage_provider_ensure_sc/tasks/main.yml` around lines 223 - 234, The block StorageClass assertion in the storage_provider_ensure_sc test is missing coverage for the qos_policy regression. Update the existing assert for _block_sc_result.resources[0].parameters to explicitly verify qos_policy is not defined, alongside the other negative parameter checks, so this test guards against block-tier StorageClasses accidentally inheriting qos_policy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/integration/targets/storage_provider_ensure_sc/tasks/main.yml`:
- Around line 384-411: Restore mock call log assertions in the integration test
to cover QoS policy behavior in addition to views and view policies. Update the
task sequence in main.yml to verify that `ensure_storage_class` POSTs to
`qospolicies` for the default tier that defines `qos_policy`, and also verify
that no `qospolicies` call is made for the block tier. Use the existing
`_mock_calls` / `_call_log` checks and add assertions keyed on the `path` and
`method` fields so the fixed block-tier QoS regression is directly covered.
- Around line 223-234: The block StorageClass assertion in the
storage_provider_ensure_sc test is missing coverage for the qos_policy
regression. Update the existing assert for
_block_sc_result.resources[0].parameters to explicitly verify qos_policy is not
defined, alongside the other negative parameter checks, so this test guards
against block-tier StorageClasses accidentally inheriting qos_policy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: edfdc465-8b25-4749-90b0-b0c93d601317
📒 Files selected for processing (7)
collections/ansible_collections/osac/templates/roles/vast_storage/tasks/create_qos_policy.yamlcollections/ansible_collections/osac/templates/roles/vast_storage/tasks/ensure_storage_class.yamltests/integration/fixtures/storage/secret-vast-tenant-config-test.yamltests/integration/mock_vms_server.pytests/integration/setup_test_env.shtests/integration/targets/storage_provider_ensure_sc/tasks/main.ymltests/integration/targets/storage_provider_setup/tasks/main.yml
💤 Files with no reviewable changes (1)
- tests/integration/targets/storage_provider_setup/tasks/main.yml
|
@wgordon17: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest |
|
No failed workflow runs found for this PR at commit |
|
This PR moves the state back to a previously known working state (it's a straightforward PR/commit revert). I'm proceeding with a self-merge |
This branch was previously deployed
2 participants
Reverts #393
Summary by CodeRabbit
Bug Fixes
Tests