Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: add new configuration for volume chunk size #381

Merged
merged 1 commit into from
Jun 7, 2022

Conversation

barpavel
Copy link
Member

@barpavel barpavel commented May 18, 2022

Adding new configuration for volume chunk size.
This duplicates VDSM irs:volume_utilization_chunk_mb configuration.
https://github.com/oVirt/vdsm/blob/v4.50.1/lib/vdsm/common/config.py.in#L325

Duplicating the configuration is bad.
But until the configuration is totally moved to engine, we have no other way to use the right size for creating new volumes.
This commit was part of #305, but in order to enable faster merging and using this new parameter extracting it out.

Bug-Url: https://bugzilla.redhat.com/1958032

@barpavel
Copy link
Member Author

barpavel commented May 18, 2022

Looks like (while adding Storage team members for the code review) I unintentionally removed many people that were added automatically, should I add them back?

@barpavel
Copy link
Member Author

/ost

@barpavel barpavel force-pushed the add_volume_chunk_size_configuration branch from 2b66690 to 1fe6d32 Compare May 18, 2022 19:47
@barpavel
Copy link
Member Author

/ost

@ahadas
Copy link
Member

ahadas commented May 19, 2022

Adding new configuration for volume chunk size. This duplicates VDSM irs:volume_utilization_chunk_mb configuration. https://github.com/oVirt/vdsm/blob/master/lib/vdsm/common/config.py.in#L325

tip for next time - better to use a url from an existing tag like (the file on master may change): https://github.com/oVirt/vdsm/blob/v4.50.1/lib/vdsm/common/config.py.in#L325

@barpavel
Copy link
Member Author

Adding new configuration for volume chunk size. This duplicates VDSM irs:volume_utilization_chunk_mb configuration. https://github.com/oVirt/vdsm/blob/master/lib/vdsm/common/config.py.in#L325

tip for next time - better to use a url from an existing tag like (the file on master may change): https://github.com/oVirt/vdsm/blob/v4.50.1/lib/vdsm/common/config.py.in#L325

Thanks, didn't know that :)
I think others are also used to reference to master.

@ahadas ahadas self-requested a review May 19, 2022 16:03
Adding new configuration for volume chunk size.
This duplicates VDSM "irs:volume_utilization_chunk_mb" configuration.
https://github.com/oVirt/vdsm/blob/v4.50.1/lib/vdsm/common/config.py.in#L325

Duplicating the configration is bad. But until the configuration
is totally moved to engine, we have no other way to use the right
size for creating new volumes.

Signed-off-by: Pavel Bar <pbar@redhat.com>
Bug-Url: https://bugzilla.redhat.com/1958032
@barpavel barpavel force-pushed the add_volume_chunk_size_configuration branch from 1fe6d32 to 4214782 Compare June 2, 2022 16:43
@barpavel
Copy link
Member Author

barpavel commented Jun 2, 2022

Adding new configuration for volume chunk size. This duplicates VDSM irs:volume_utilization_chunk_mb configuration. https://github.com/oVirt/vdsm/blob/master/lib/vdsm/common/config.py.in#L325

tip for next time - better to use a url from an existing tag like (the file on master may change): https://github.com/oVirt/vdsm/blob/v4.50.1/lib/vdsm/common/config.py.in#L325

Done, both in PR and commit descriptions.

@barpavel barpavel requested a review from nirs June 6, 2022 08:48
@barpavel
Copy link
Member Author

barpavel commented Jun 6, 2022

@ahadas fixed both your comments.
The moment this PR will be merged, I can merge it with #305 and simplify the latter.

@ahadas ahadas merged commit bcbaa3a into oVirt:master Jun 7, 2022
@barpavel barpavel deleted the add_volume_chunk_size_configuration branch June 7, 2022 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants