Skip to content

lightbits: qos support#24

Open
yuval-lb wants to merge 1 commit intomasterfrom
yuval/add_qos_support
Open

lightbits: qos support#24
yuval-lb wants to merge 1 commit intomasterfrom
yuval/add_qos_support

Conversation

@yuval-lb
Copy link
Collaborator

@yuval-lb yuval-lb commented Jan 14, 2025

TBD: update README files

acl=['ALLOW_NONE'],
ip_acl=vol_ipAcl
ip_acl=vol_ipAcl,
qos_policy=qos_policy)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you have one extra closing parenthesis )

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

compression = self._parse_extra_spec(type_compression,
default_compression)
num_replicas = str(specs.get('lightos:num_replicas', num_replicas))
qos_policy = str(specs.get('lightos:qos_policy', None))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuval-lb
If you don't provide "lightos:qos_policy" to volume Type, this call will convert the default None to a string-value, this string "None" will pass your check at line #254 assuming you have a policy with name "None" on Lighybits storage and volume creation will fail.

if kwargs.get("qos_policy", None) is not None:

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right

@yuval-lb yuval-lb force-pushed the yuval/add_qos_support branch 2 times, most recently from 8b3121b to ea45d56 Compare January 16, 2025 09:30
@husam-lb
Copy link

@yuval-lb do you want to merge to master!
I think it should be mirror of openstack master and we shouldn't modify it

@yuval-lb yuval-lb force-pushed the yuval/add_qos_support branch from ea45d56 to 2de2abb Compare January 21, 2025 10:43
@yuval-lb
Copy link
Collaborator Author

@yuval-lb do you want to merge to master! I think it should be mirror of openstack master and we shouldn't modify it

I am not merging its just for your review

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.

4 participants