Skip to content

Conversation

@rsarm
Copy link
Contributor

@rsarm rsarm commented Feb 22, 2022

Closes #2440

@codecov-commenter
Copy link

codecov-commenter commented Feb 22, 2022

Codecov Report

Merging #2444 (bc20ca7) into master (a3a94fb) will increase coverage by 0.03%.
The diff coverage is 90.62%.

@@            Coverage Diff             @@
##           master    #2444      +/-   ##
==========================================
+ Coverage   85.73%   85.76%   +0.03%     
==========================================
  Files          57       57              
  Lines       10555    10580      +25     
==========================================
+ Hits         9049     9074      +25     
  Misses       1506     1506              
Impacted Files Coverage Δ
reframe/core/config.py 91.53% <88.00%> (+0.43%) ⬆️
reframe/frontend/argparse.py 92.75% <100.00%> (+0.05%) ⬆️
reframe/frontend/cli.py 71.09% <100.00%> (+0.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3a94fb...bc20ca7. Read the comment docs.

Copy link
Contributor

@ekouts ekouts left a comment

Choose a reason for hiding this comment

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

The implementation looks fine to me, but we should document the option in docs/config_reference.rst and docs/manpage.rst.

@ekouts
Copy link
Contributor

ekouts commented Feb 23, 2022

I realize now you also have to update the documentation in Picking a system configuration section in docs/configure.rst. Same for the --system=NAME option in docs/manpage.rst. (look for xthostname in docs)

@rsarm rsarm marked this pull request as ready for review February 23, 2022 19:49
Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

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

I don't think that controlling the method to retrieve the hostname can be part of the configuration. ReFrame uses the hostname to navigate through the configuration and eventually pick the right system subconfig. It's the exactly equivalent of passing --system: this cannot be part of the configuration. This means that you would need an environment variable to control the method, such as RFM_SYSTEM_AUTODETECT_METH=fqdn. Now the problem with that is that whatever we do we will be introducing a breaking behaviour. The current auto-detection works as follows:

  1. Search for xthostname
  2. If not found, use fqdn to retrieve the hostname (in the past we have been using hostname).

I agree that the auto-detection process should be controlled, but assuming that we use an environment variable as the above, what would be the default value? If it's fqdn we would break the auto-detection in Cray systems, if it's anything else, we would break the current behaviour. There are also aspects in auto-detection when you run in the cloud. See this for example, where they construct the system name from the instance information retrieved by a REST call. To support this, we would need to generalise the auto-detection mechanism a bit differently than how this PR tries to do it.

So perhaps we could simplify this PR by introducing three environment variables controlling the auto-detection though hostname:

RFM_AUTODETECT_METHOD=hostname
RFM_AUTODETECT_XTHOSTNAME=1   # (default)
RFM_AUTODETECT_FQDN=1 # default

The hostname autodetection method is the only supported method currently, but the other two variables control whether the xthostname will also be tried and whether the fqdn will be used instead of the hostname.

Does it make sense?

Copy link
Contributor

@ekouts ekouts left a comment

Choose a reason for hiding this comment

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

I still have some small comments but in general lgtm.

@vkarak
Copy link
Contributor

vkarak commented Apr 1, 2022

I'm updating a bit the implementation: I have made our ArgumentParser understand the default argument when we are treating options that are only set from the environment. I think that was a bug. Then I'm refactoring a bit the auto-detection. I don't think that the gethostname_cmd() should be a utility function. I am moving its functionality inside the configuration code. Also, there is no need to pass the auto-detection options at the creation of the configuration, since the auto-detection happens in select_subconfig. I will add a method to set the auto-detection method in _SiteConfig and this one will always be called from the cli based on the command line options.

Vasileios Karakasis added 2 commits April 1, 2022 13:28
- Support defaults in ArgumentParser for environment variable-only options
- Remove `get_hostname_cmd()`
- Autodetection happens entirely in `_SiteConfig`
- New method for setting the autodetection method in `_SiteConfig`.
@vkarak vkarak changed the title [feat] Allow different ways of specifying a hostname command through the configuration [feat] Introduce environment variables for controlling system auto-detection Apr 1, 2022
@vkarak vkarak merged commit b71da59 into reframe-hpc:master Apr 1, 2022
@rsarm rsarm deleted the config-hostname branch February 3, 2023 13:20
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.

Allow different ways of specifying a hostname command through the configuration

4 participants