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

Added watermark_scale_factor tuning #609

Merged
merged 10 commits into from
Sep 23, 2021

Conversation

brettmilford
Copy link
Contributor

Adds a method for determining an appropriate vm.watermark_scale_factor
value for baremetal systems.

Implements: spec memory-fragmentation-tuning

https://review.opendev.org/c/openstack/charm-specs/+/792383

Adds a method for determining an appropriate vm.watermark_scale_factor
value for baremetal systems.

Implements: spec memory-fragmentation-tuning
Copy link
Contributor

@ajkavanagh ajkavanagh left a comment

Choose a reason for hiding this comment

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

Thank you for the patch for the additional functionality. I have a few inline comments (please see against the file), and also I think the tests need to be expanded to test the calculate_watermark_scale_factor() function (i.e. only the watermark_scale_factor() function is tested).

It would also be good to actually feed it test fixtures for the /proc/zoneinfo and /proc/meminfo/.

Also, technically, the library still supports Py27 charms, and definitely supports Py35 (xenial) charms.

Thanks.

charmhelpers/contrib/sysctl/watermark_scale_factor.py Outdated Show resolved Hide resolved
charmhelpers/contrib/sysctl/watermark_scale_factor.py Outdated Show resolved Hide resolved
charmhelpers/contrib/sysctl/watermark_scale_factor.py Outdated Show resolved Hide resolved
charmhelpers/contrib/sysctl/watermark_scale_factor.py Outdated Show resolved Hide resolved
charmhelpers/contrib/sysctl/watermark_scale_factor.py Outdated Show resolved Hide resolved
charmhelpers/contrib/sysctl/watermark_scale_factor.py Outdated Show resolved Hide resolved
charmhelpers/contrib/sysctl/watermark_scale_factor.py Outdated Show resolved Hide resolved
charmhelpers/contrib/sysctl/watermark_scale_factor.py Outdated Show resolved Hide resolved
- improving tests
- implemented suggested changes
@brettmilford
Copy link
Contributor Author

Thanks for the feedback @ajkavanagh.
I've tried implementing your suggestions and improving testing in the latest commit.

Copy link
Contributor

@ajkavanagh ajkavanagh left a comment

Choose a reason for hiding this comment

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

Very much more readable this time; much improved. Unfortunately(!) this let me see some more areas (mostly docstrings) and also a suggestion on improving the code around the searching or proc/zoneinfo.

Many thanks.

charmhelpers/contrib/sysctl/watermark_scale_factor.py Outdated Show resolved Hide resolved
charmhelpers/contrib/sysctl/watermark_scale_factor.py Outdated Show resolved Hide resolved
charmhelpers/contrib/sysctl/watermark_scale_factor.py Outdated Show resolved Hide resolved
charmhelpers/contrib/sysctl/watermark_scale_factor.py Outdated Show resolved Hide resolved
charmhelpers/contrib/sysctl/watermark_scale_factor.py Outdated Show resolved Hide resolved
charmhelpers/contrib/sysctl/watermark_scale_factor.py Outdated Show resolved Hide resolved
charmhelpers/contrib/sysctl/watermark_scale_factor.py Outdated Show resolved Hide resolved
charmhelpers/contrib/sysctl/watermark_scale_factor.py Outdated Show resolved Hide resolved
tests/contrib/sysctl/test_watermark_scale_factor.py Outdated Show resolved Hide resolved
@brettmilford
Copy link
Contributor Author

Hi Alex,
Thanks again for your extensive feedback.
I much prefer your approach to parsing /proc/zoneinfo so I've largely used that.

The heuristics for calculating vm.watermark_scale_factor might change based on this charm spec, and subsequent reviews.
Also currently the unit tests for get_memtotal and get_normal_managed_pages are failing as mock_open doesn't appear to be using read_data as I expected.

ajkavanagh
ajkavanagh previously approved these changes Jun 2, 2021
Copy link
Contributor

@ajkavanagh ajkavanagh left a comment

Choose a reason for hiding this comment

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

LGTM - let me know if it's ready for merging; you mentioned that there might be a change? Also, thanks very much for your work on this and the changes.

@brettmilford
Copy link
Contributor Author

@ajkavanagh I think this should now be ready for merging. The associated spec is approved and the tests are passing for me in my env with the latest commit.

@ajkavanagh
Copy link
Contributor

@ajkavanagh I think this should now be ready for merging. The associated spec is approved and the tests are passing for me in my env with the latest commit.

Thanks @brettmilford ; I'm back from leave now and have just kicked off the tests.

@brettmilford
Copy link
Contributor Author

Hey @ajkavanagh, can you please run the tests again. TY

@niedbalski niedbalski self-requested a review September 21, 2021 08:11
Copy link

@niedbalski niedbalski left a comment

Choose a reason for hiding this comment

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

LGTM, minor changes required.

@brettmilford
Copy link
Contributor Author

@niedbalski @ajkavanagh please take another look, I've added the changes requested. TY

Copy link

@niedbalski niedbalski left a comment

Choose a reason for hiding this comment

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

Minor change requested.

@niedbalski niedbalski merged commit 26efcd0 into juju:master Sep 23, 2021
openstack-mirroring pushed a commit to openstack/charm-swift-storage that referenced this pull request Sep 12, 2022
Depends: juju/charm-helpers#609
Implements: spec memory-fragmentation-tuning
Change-Id: I0195d5f65f36442abf1355dd5150d48a37184e97
openstack-mirroring pushed a commit to openstack/openstack that referenced this pull request Sep 12, 2022
* Update charm-swift-storage from branch 'master'
  to 34b00a0530d5241b43fda7ba2dbe1abdbf31ecd3
  - Merge "Add vm.watermark_scale_factor tuning"
  - Add vm.watermark_scale_factor tuning
    
    Depends: juju/charm-helpers#609
    Implements: spec memory-fragmentation-tuning
    Change-Id: I0195d5f65f36442abf1355dd5150d48a37184e97
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