[feat] Add configuration parameter to control the timeout of the git clone commands

[mahendrapaipuri]

Fixes #2218

[jenkins-cscs]

Can I test this patch?

[codecov-commenter]

Codecov Report

Merging #2225 (49d7093) into master (3132e52) will increase coverage by 0.00%.
The diff coverage is 40.00%.

@@           Coverage Diff           @@
##           master    #2225   +/-   ##
=======================================
  Coverage   85.54%   85.54%           
=======================================
  Files          55       55           
  Lines        9746     9747    +1     
=======================================
+ Hits         8337     8338    +1     
  Misses       1409     1409           

Impacted Files	Coverage Δ
reframe/core/pipeline.py	92.24% <0.00%> (ø)
reframe/utility/osext.py	84.71% <33.33%> (ø)
reframe/frontend/cli.py	76.36% <100.00%> (+0.04%)	⬆️

---

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 3132e52...49d7093. Read the comment docs.

[jjotero]

Having this git timeout parameter feels a bit too specific for the config file. In theory, once #2166 is merged, one could create a very simple fixture that clones a repo with all the timeout requirements and so on.

This being said, I don't have that much of a strong opinion and I could live with this config option being added :)

[mahendrapaipuri]

@jjotero Yes, I agree with your overall comment. Once the fixtures become available, this will become more easier to manage. However, we still need to expose timeout argument of the git_clone function which is not the case as of now.

I thought of using this configurable timeout value wherever run_command function is called within ReFrame. But then, looking at sources, I figured out that timeout is only used in git related functions and nowhere else. That is why it became specific to git commands. Do you think it is worth using it at other places where run_command is used?

[jjotero]

@mahendrapaipuri I like the idea of having a config parameter for run_command a lot more than just for git_clone. How about something like min_timeout that sets the minimum timeout for any call to run_command? That can be done doing something like this in the run_command function:

if timeout is not None:
    try:
        min_timeout = getattr(run_command, 'min_timeout')
    except AttributeError:
        min_timeout = rt.runtime().get_option('general/0/min_timeout')
        run_command.min_timeout = min_timeout
    except Exception:
        min_timeout = 0

    timeout = timeout if timeout > min_timeout else min_timeout

What do you think?

In any case, I think it still makes sense to extend the git_clone function to have the timeout argument.

[mahendrapaipuri]

@jjotero Yes, your suggestion sounds good and more generic. Why can't we do something more simpler like:

if timeout is not None:
    timeout = max(timeout, rt.runtime().get_option('general/0/min_timeout'))

As we define a default value for min_timeout in config schema, it will never raise an exception. Am I missing something here?

Also, in the slack conversion I had with @vkarak he told
"Because the utility package is meant to be independent, not knowing anything about the runtime, including the configuration."
This solution will bring runtime configuration into utility package. Would it have any side effects?

[jjotero]

@mahendrapaipuri your option requires the runtime to be present in order to call this run_command function. If the runtime is not setup, you get a ReframeFatalError when you call rt.runtime(). And as you said, since this is a utility function, it should never require the runtime to be present. However, with what I suggested, the runtime being present is entirely optional.

In any case, circular imports prevent us from doing this (😭), so I guess the git_timeout option looks like the best solution. After all, cloning a repo on a reframe test is a very common thing to do, so I can see valid reasons to keep it as a config option.

[mahendrapaipuri]

@jjotero Fair enough. Anyways, if I am not wrong, this timeout argument to run_command is not None only in git_repo_exists function. So, your suggestion would have effect only on this function.

Another possibility is to use a global min_timeout config parameter at the places where run_command is called. I see it is mostly used in modules classes. But again, I am not sure if it really necessary to use timeout in those function calls.

[jjotero]

I'm not too worried on whether that timeout gets used elsewhere or not. This could change in the future, who knows. My concern was simply that I wasn't sure if it made sense from a conceptual point of view to have the git option in the config file. And considering all the other options, this is certainly the one that makes more sense to me 👍

[vkarak]

I have just a minor comment regarding the PR as is. On the discussion that you had, I don't think that the fixtures would address this same issue, unless you call git_repo_clone() explicitly, but ReFrame calls that implicitly when sourcesdir refers to a Git repo. So this option practically controls that. Regarding setting the run command timeout globally, I would be against, not only because the utility package should not depend on the runtime, but also because it could have more obscure side effects in the future. I prefer specific options. For example, we used to have a timeout in the past for the sbatch command; I wouldn't want to have this controlled by a very generic option, but rather from a specific scheduler backend option.

[pep8speaks]

Hello @mahendrapaipuri, Thank you for updating!

Cheers! There are no PEP8 issues in this Pull Request!Do see the ReFrame Coding Style Guide

Comment last updated at 2021-10-22 20:49:44 UTC

[vkarak]

I've also added an associated environment variable RFM_GIT_CLONE_TIMEOUT.

Comments are resolved.