LightOS driver: change QoS policy without migration - #27
Open
yuval-lb wants to merge 2 commits into
Open
Conversation
Changing the IOPS limit of a volume meant retyping it, and the driver implemented no retype, so the volume manager fell back to a host-assisted migration. Every QoS change copied the whole volume into a new one and deleted the original. For an operator moving a group of volumes between limits this is the slowest possible way to change a number. A QoS policy is a property of the volume, not of where it sits, and the volume update API already accepts one. The driver now implements retype: when the only difference between the two volume types is the QoS policy, it sets the policy on the volume and reports the retype done, so no data moves. Anything else is handed back to the volume manager, which migrates as before: a different project, replica count or compression all describe where the volume lives. Removing a policy is handed back too, because falling back to the project default is not expressible as a volume update. A rejected policy raises, which the manager logs and then migrates, so an operator gets the retype they asked for either way. Assisted-by: Claude Opus 5 Signed-off-by: Yuval Brave <yuval@lightbitslabs.com>
ronen-lb
reviewed
Aug 9, 2026
ronen-lb
left a comment
ronen-lb
left a comment
There was a problem hiding this comment.
Review scope: full diff.
Review assisted by an AI agent; findings were checked by a human before posting.
ronen-lb
reviewed
Aug 10, 2026
| return False | ||
|
|
||
| changed = self._changed_specs(diff.get('extra_specs')) | ||
| if not changed: |
There was a problem hiding this comment.
what is the expected behaviour here?
this flow (return true) is for a scenario where volume type is exactly the same, so we do no action?
possibly a safer flow would be to return false here? (this would also cover case of a new spec added in future?)
Collaborator
Author
There was a problem hiding this comment.
@ronen-lb I leaving it as it is - meaning if you do retyoe from Volume A to A it will not do anything
Motivation: review findings on the in-place QoS retype. The reason set_volume_qos_policy is a separate command - a QoS-only change must not disturb a live ACL - was stated in the change description but pinned by no test, so folding the QoS field back into update_volume would pass the suite. The driver document still described retype as host assisted only, and the fallback debug line named the mechanism rather than the decision. Implementation: a retype test sets host ACLs on the volume and asserts they survive the QoS update. The driver document's capability list and QoS section now describe the in-place path and when it falls back. The fallback log line says what was decided and why, and a comment records that placement specs are compared textually, so a semantically equal respelling falls back to migration rather than risking a wrong in-place match. Issue: LBM1-47082 Assisted-by: Claude Fable 5 Change-Id: I7f5488be71565515be2fd75f052f67ba48445608 Signed-off-by: Yuval Brave <yuval@lightbitslabs.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR description
Changing a volume's QoS policy meant retyping it, and the LightOS driver had
no
retype, so Cinder always fell back to a host-assisted migration. EveryQoS change copied the whole volume and deleted the original, at a cost that
scales with the data rather than with the change.
A QoS policy is a property of the volume, and the LightOS volume update API
accepts one. The driver now implements
retype: when the QoS policy is theonly difference between the two volume types, it sets the policy on the volume
and reports the retype done. No data moves.
Anything else is handed back to Cinder and migrates as it does today — a
different project, replica count or compression all describe where the volume
lives. Dropping a policy is handed back too, since falling back to a project
default is not expressible as a volume update.
Reviewer note: the QoS update is its own command carrying only the volume UUID
and the policy, rather than an extension of
update_volume, which also carriesthe ACL. A QoS-only change must not disturb a live ACL.
How was the PR tested?
project / replicas / compression / policy removal / Cinder
qos_specs, andthe failure path. 44/44 pass in the module; all 9 fail without this change.
flake8 clean.
policy. The LightOS volume UUID and name were unchanged, the policy moved,
and it took 4s with no create, delete or migration. An ACL set beforehand
survived untouched.
declined and Cinder migrated, as intended.
Not tested: the speed-up on a volume holding real data. The test volume was
empty, so its migration was cheap; the in-place path is constant time while a
migration scales with the data.
PR dependencies
Jira Ticket
Issue: LBM1-47082