Sitelet https://web.archive.org/web/20220615145755/https://github.com/osm-search/Nominatim/pull/2552
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

add utils/collect_os_info.sh script #2552

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

micahcochran
Copy link

@micahcochran micahcochran commented Dec 13, 2021

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:

  • OS version
  • RAM
  • Number of CPUs
  • PostgreSQL version

What doesn't work:

  • Nominatim Version - this will work if the script is ran from the "utils" folder. Python does a local import of the version.
  • PostGIS Version - I have an idea of what to do, but I don't have a test machine at the moment.
  • Type and size of disks: - I would need some help with what is the relevant information. I put some links in the script for ways to get information. This could vary wildly based on the the setup (bare metal/Cloud/VM/Container).
    • bare metal/AWS/other cloud service: - Could just query the user and fill out common answers.

Copy link
Member

@lonvia lonvia left a comment

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))"`
Copy link
Member

@lonvia lonvia Dec 15, 2021

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?

Copy link
Author

@micahcochran micahcochran Dec 15, 2021

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: _
Copy link
Member

@lonvia lonvia Dec 15, 2021

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/
Copy link
Member

@lonvia lonvia Dec 15, 2021

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=
Copy link
Contributor

@otbutz otbutz Dec 15, 2021

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:
Copy link
Contributor

@otbutz otbutz Dec 15, 2021

Choose a reason for hiding this comment

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

systemd-detect-virt

Copy link
Author

@micahcochran micahcochran Dec 15, 2021

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!

@micahcochran
Copy link
Author

@micahcochran micahcochran commented Jan 3, 2022

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.

Copy link
Member

@lonvia lonvia left a comment

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:
Copy link
Member

@lonvia lonvia Jan 5, 2022

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.
Copy link
Member

@lonvia lonvia Jan 5, 2022

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
Copy link
Member

@lonvia lonvia Jan 5, 2022

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)
Copy link
Member

@lonvia lonvia Jan 5, 2022

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.

@lonvia
Copy link
Member

@lonvia lonvia commented Feb 25, 2022

What you've done so far looks really good and I'd be happy to see that merged. So I'm tagging this with help wanted, so that maybe somebody else can pick up the integration into nominatim CLI.

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