Skip to content

Adding QoS functionalities#65

Merged
rounak-adhikary merged 3 commits into
mainfrom
usr/rounak-adhikary/qos
May 26, 2026
Merged

Adding QoS functionalities#65
rounak-adhikary merged 3 commits into
mainfrom
usr/rounak-adhikary/qos

Conversation

@rounak-adhikary
Copy link
Copy Markdown
Contributor

No description provided.

@rounak-adhikary rounak-adhikary force-pushed the usr/rounak-adhikary/qos branch from 2ad3b38 to 3abfc7b Compare May 21, 2026 11:16
- Add docstrings to all methods in file_io_limit_rule.py entity
- Add docstrings to all methods in io_limit_rule.py entity
- Add docstrings to QoS methods in policy.py entity
- Fix line-too-long errors in common_data.py f-strings
- Fix unused variable warning in test_io_limit_rule.py
@rounak-adhikary rounak-adhikary force-pushed the usr/rounak-adhikary/qos branch from 1e7bf79 to be2edcf Compare May 21, 2026 12:11
@Krunal-Thakkar
Copy link
Copy Markdown
Contributor

PR Review – Adding QoS Functionalities

TL;DR

The changes introduce QoS / File‑Performance policies, IO‑Limit rules, and File‑IO‑Limit rules, update payload helpers, and add a comprehensive test suite. No obvious security, dependency, or functional defects were found. The PR does not reference a Jira ticket, which should be added to the description if one exists.


✅ What the PR Does

  1. Provisioning layer

    • Adds qos_performance_policy_id to creation/modification payload helpers for volumes and volume groups.
    • Extends clone_volume and clone_volume_group to accept the same new field.
    • Updates public methods (create_volume, modify_volume, etc.) to expose the new argument.
  2. Protection layer

    • Implements generic QoS / File‑Performance policy CRUD (get_policy_details, get_policy_by_name, create_policy, modify_policy, delete_policy).
    • Uses the existing /policy endpoint – the same API used for protection policies.
  3. Configuration layer

    • Adds full CRUD support for IO‑Limit Rules and File‑IO‑Limit Rules (list, details, create, modify, delete).
  4. Constants

    • Extends SELECT strings with the new fields (qos_performance_policy_id, performance_policy_id).
    • Introduces endpoint URLs and query‑string templates for the new objects.
  5. Tests

    • New unit‑test modules for IO‑Limit Rule, File‑IO‑Limit Rule, and QoS policies.
    • Updated myrequests mapping to include the new mock entities.
    • Coverage includes happy‑path calls, request‑method verification, URL correctness, and error handling (invalid IDs).

🔍 Code & Test Quality

  • Consistency – The new parameter is added consistently across all relevant payload‑building helpers and public methods.
  • Error handling – Invalid IDs trigger PowerStoreException via the underlying client, and the tests verify this behaviour.
  • Naming – Clear, descriptive names (qos_performance_policy_id, io_limit_rule_id, etc.).
  • Documentation – Docstrings updated to reflect new arguments.
  • Tests – Reasonable unit‑test coverage for the newly added APIs; they assert both response data and that the correct HTTP method / URL is used.

🛡️ Security / Dependency Concerns

  • No new third‑party dependencies are introduced.
  • No hard‑coded credentials or insecure handling of data.
  • All new API calls are routed through the existing client abstraction, which already implements authentication and SSL handling.

📌 Minor Observations

  • In PolicyResponse.get_api_name the QoS detection checks only for "io_limit_rule" in the select field.
    The File‑Performance policy also contains file_io_limit_rule; the current check will still fallback to the generic protection handler but may be less explicit. This is not a blocker for the current test suite.

  • The constant QOS_POLICY_DETAILS_QUERY is defined but not used – the protection methods build the query inline. This is harmless but slightly redundant.

  • The PR description is empty. According to the repository guidelines, the description should contain the related Jira ticket in the format [JIRA-XXXX]. If this work is tied to a ticket, please add it; otherwise, consider adding a short summary of the changes for future reviewers.


📋 Jira Requirement Check

  • Does the PR reference a Jira ticket?
    No. The PR description does not contain a ticket identifier.

  • Is this a problem?
    Only if the change is supposed to be tracked against a Jira story. If that is the case, please edit the PR description to include the ticket (e.g., [PROJ-1234]). If the work is intentionally generic or experimental, the current state is acceptable.


✅ Final Verdict

The implementation appears correct, complete, and well‑tested. Apart from the missing Jira reference (which may simply be an omission in the description), there are no critical issues.

Recommendation: Approve the PR (or request the minor documentation update) and merge.


This review notes that the PR does not have an associated Jira ticket. If a ticket should exist, update the PR description accordingly.

PR Details

  • Org/Repo/PR_Number dell/python-powerstore/65

Jira Story

  • ID:

Jira Issue

  • Title: Generic PR with no specific Jira Ticket
  • Issue Type: Free Form Review with no Jira ticket

LLM Stats

  • Total Time Taken: 11.83 seconds
  • Total Input Tokens: 24650

🧠 Chain of Thought (click to expand)

@rounak-adhikary rounak-adhikary merged commit 315ccc7 into main May 26, 2026
6 checks passed
@rounak-adhikary rounak-adhikary deleted the usr/rounak-adhikary/qos branch May 26, 2026 05:43
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.

3 participants