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
add utils/collect_os_info.sh script #2552
base: master
Are you sure you want to change the base?
Conversation
Thank you. This looks quite useful already. And very well documented. I added some thoughts to specific points inline.
I do wonder about how to best integrate such a script, though. When the issue was originally opened, Nominatim wasn't installable yet, so everybody had the source code with the utils directory lying around. That is not the case anymore. So we might be better off to offer this functionality as part of the nominatim command. I could imagine a verbose version output nominatim --version -vv or something as part of the admin command nominatim admin --system-info. It means that the script needs to be rewritten in Python. The added bonus is that you have access to stuff like the postgresql and postgis version helper functions. What do you think?
| # NOTE: Getting this version will NOT work if it is being ran from in another | ||
| # folder than Nominatim/utils. It call python3 to import version.py locally and | ||
| # prints it in the version format. | ||
| NominatimVersion=`cd ../nominatim/ && python3 -c "import version; print('{0[0]}.{0[1]}.{0[2]}-{0[3]}'.format(version.NOMINATIM_VERSION))"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nicer solution here would be to have a nominatim --version command. It should be fairly simple to add in cli.py. Do you want to give that a try?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will give that a try.
| # - bare metal/AWS/other cloud service: | ||
| # Unsure of how to detect this, but it might be useful for reporting disk storage. | ||
| # One options would be to prompt the user something like this: | ||
| # Enter system configuration (1) bare metal (2) AWS (3) Other Cloud (4) Docker (5) Other: _ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These days a simple 'Does it run under docker' would already suffice. That should be possible to detect.
| # `df -h` - show the free space on drives | ||
| # `lsblk` - this tell you what the server has not necessarily this machine. So in a container environment | ||
| # (like docker) this wouldn't be the correct report. | ||
| # This guide shows ways to get various storage device information: https://www.cyberciti.biz/faq/find-hard-disk-hardware-specs-on-linux/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
df -h and lsblk look good on bare metal. Maybe start with that?
| # ASSUME the username is nominatim | ||
| # This needs to be ran under the account with the appropriate permissions. | ||
| # This has been left blank. | ||
| PostGISVersion= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming that the default postgis3 version is installed, maybe something like this:
grep '^default_version' "$(pg_config --sharedir)/extension/postgis-3.control"| echo "**Hardware Configuration (please correct the following information):**" | ||
| echo - RAM: $RAM | ||
| echo - number of CPUs: $NumCPUs | ||
| echo - type and size of disks: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
systemd-detect-virt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works really well to report if there is a container or VM and what type environment. Thanks!
|
Thank you again for the feedback. In commit 8bda59f, I have rewritten the script as a separate Python file. I have not integrated with the cli.py, but at least this being written in Python it would be a step closer to being able to be integrated with the file. There are a few fields that still need to be filled in. I have included the bash file and the Python file so that they can both be available for the review. Not looking for this to get merged at this point. Just looking for feedback. |
Works like a charm.
To integrate it with the nominatim tool, just move the script to nominatim/tools and add a new subcommand in nominatim/clicmd/admin.py. Please run pylint against it and make sure you use spaces instead of tabs.
If you don't have the means to set up a test machine, you can use the CI for testing. Just add the execution of your command around here, push the change and check the action output in Github.
| # done, do that action in the __init__() or another function. | ||
| message = """ | ||
| Use this information in your issue report at https://github.com/osm-search/Nominatim/issues | ||
| Copy and paste or redirect the output of the file: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably should add a warning here along the lines of: "Review the output before posting to make sure there is no sensitive data in it."
| {self.postgresql_config} | ||
| ``` | ||
| **Notes** | ||
| Please add any notes about anything above anything above that is incorrect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicate 'anything above'.
On a more general note: people should rather edit the things that are incorrect and only add additional information.
| mag = 0 | ||
| # determine order of magnitude | ||
| while mem > 1000: | ||
| mem /= 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably should be 1024?
| self.friendly_memory: str = self._friendly_memory_string(self._memory) | ||
| # psutil.cpu_count(logical=False) returns the number of CPU cores. | ||
| # For number of logical cores (Hypthreaded), call psutil.cpu_count() or os.cpu_count() | ||
| self.num_cpus: int = psutil.cpu_count(logical=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both, logical CPUs and physical CPUs would be interesting.
|
What you've done so far looks really good and I'd be happy to see that merged. So I'm tagging this with |
This was requested in issue #307 . It is a bash script that generates a report of some the information needed for a bug report.
What works:
What doesn't work: