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.

Sign up for GitHub

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

Prevent git from hanging a process #6140

Closed
wants to merge 1 commit into from
Closed

Prevent git from hanging a process #6140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions lib/OpenQA/Git.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,11 @@ sub _validate_attributes ($self) {

sub _run_cmd ($self, $args, $options = {}) {
my $include_git_path = $options->{include_git_path} // 1;
my $ssh_batchmode = $options->{ssh_batchmode} // 0;
#warn(">>>> $options");
my $batchmode = $options->{batchmode} // 0;
my @cmd;
push @cmd, 'env', 'GIT_SSH_COMMAND=ssh -oBatchMode=yes' if $ssh_batchmode;
push @cmd, 'env', 'GIT_SSH_COMMAND=ssh -oBatchMode=yes' if $batchmode;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you not use GIT_ASKPASS and GIT_TERMINAL_PROMPT like suggested in the ticket?
Waiting for a timing out command seems way more complicated than just telling git to not prompt in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that with the timeout we gonna have a general solution when the _run_cmd (or more specifically run_cmd_with_log_return_error) encounter similar issue.
GIT_ASKPASS and GIT_TERMINAL_PROMPT make the logic more complicated IMO but also provide a better error as I didnt find a way to catch the prompt in the $stdout somehow.
Maybe both can be applied at the end.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what's complicated about using two env vars. It's as simple as it gets?

#push @cmd, 'env', $options->{nopass} if $options->{nopass};
push @cmd, $self->_prepare_git_command($include_git_path), @$args;

my $result = run_cmd_with_log_return_error(\@cmd);
Expand Down Expand Up @@ -97,6 +99,7 @@ sub commit ($self, $args = undef) {

# push changes
if (($self->config->{do_push} || '') eq 'yes') {
#my $options->{nopass} = "GIT_TERMINAL_PROMPT=false";
$res = $self->_run_cmd(['push']);
return $self->_format_git_error($res, 'Unable to push Git commit') unless $res->{status};
}
Expand All @@ -123,14 +126,14 @@ sub check_sha ($self, $sha) {
}

sub get_remote_default_branch ($self, $url) {
my $r = $self->_run_cmd(['ls-remote', '--symref', $url, 'HEAD'], {include_git_path => 0, ssh_batchmode => 1});
my $r = $self->_run_cmd(['ls-remote', '--symref', $url, 'HEAD'], {include_git_path => 0, batchmode => 1});
die qq/Error detecting remote default branch name for "$url": $r->{stdout} $r->{stderr}/
unless $r->{status} && $r->{stdout} =~ m{refs/heads/(\S+)\s+HEAD};
return $1;
}

sub clone_url ($self, $url) {
my $r = $self->_run_cmd(['clone', $url, $self->dir], {include_git_path => 0, ssh_batchmode => 1});
my $r = $self->_run_cmd(['clone', $url, $self->dir], {include_git_path => 0, batchmode => 1});
die $self->_format_git_error($r, qq/Failed to clone "$url"/) unless $r->{status};
}

Expand All @@ -141,7 +144,7 @@ sub get_origin_url ($self) {
}

sub fetch ($self, $branch_arg) {
my $r = $self->_run_cmd(['fetch', 'origin', $branch_arg], {ssh_batchmode => 1});
my $r = $self->_run_cmd(['fetch', 'origin', $branch_arg], {batchmode => 1});
die $self->_format_git_error($r, "Failed to fetch from '$branch_arg'") unless $r->{status};
}

Expand Down
1 change: 0 additions & 1 deletion lib/OpenQA/Task/Needle/Delete.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ sub register {

sub _delete_needles {
my ($app, $minion_job, $args) = @_;

my $schema = $app->schema;
my $needles = $schema->resultset('Needles');
my $user = $schema->resultset('Users')->find($args->{user_id});
Expand Down
36 changes: 25 additions & 11 deletions lib/OpenQA/Utils.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
use Carp;
use Cwd 'abs_path';
use Filesys::Df qw(df);
use IPC::Run();
use IPC::Run qw(run timeout);
use Mojo::URL;
use Regexp::Common 'URI';
use Time::Seconds;
Expand Down Expand Up @@ -333,13 +333,17 @@
sub run_cmd_with_log_return_error ($cmd, %args) {
my $stdout_level = $args{stdout} // 'debug';
my $stderr_level = $args{stderr} // 'debug';
my $timeout_duration = 30;
log_info('Running cmd: ' . join(' ', @$cmd));
my ($stdin, $stdout_err, $stdout, $stderr) = ('') x 4;
try {
my ($stdin, $stdout_err, $stdout, $stderr) = ('') x 4;
my $ipc_run_succeeded = IPC::Run::run($cmd, \$stdin, \$stdout, \$stderr);
my $ipc_run = run($cmd, \$stdin, \$stdout, \$stderr, timeout($timeout_duration));
my $return_code = $?;
chomp $stderr;
if ($ipc_run_succeeded) {
#my $return_code = $?;
# use Data::Dumper;
#print "***";print Dumper($@);
if ($ipc_run) {
OpenQA::Log->can("log_$stdout_level")->($stdout);
OpenQA::Log->can("log_$stderr_level")->($stderr);
log_info("cmd returned $return_code");
Expand All @@ -349,20 +353,30 @@
log_error("cmd returned $return_code");
}
return {
status => $ipc_run_succeeded,
status => $ipc_run,
return_code => $return_code,
stdout => $stdout,
stderr => $stderr,
};
}
catch {
return {
status => 0,
return_code => undef,
stderr => 'an internal error occurred',
stdout => '',
my $additional_info = "If you use http for CASEDIR or NEEDLES_DIR is adviced to

Check warning on line 363 in lib/OpenQA/Utils.pm

View workflow job for this annotation

GitHub Actions / Perlcritic

Useless interpolation of literal string - severity 3 

[ValuesAndExpressions::ProhibitInterpolationOfLiterals] See page 51 of PBP
Copy link
Contributor

Choose a reason for hiding this comment

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

run_cmd_with_log_return_error is a general purpose function not only used for git commands.
It doesn't make sense to add a message about git here.

Also, we shouldn't add lengthy directions into the error message, but link to the documentation, as it was suggested in the ticket

configure git with the insteadof option in the .gitconfig file.
Run the following command to:
git config [--global] url.\"git://github.com\".insteadOf https://github.com";
Copy link
Contributor

Choose a reason for hiding this comment

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

url.insteadOf is not what we want. it is url.pushInsteadOf, which is an important difference.
We still want the fetch to happen with http.

my $stderr_msg = ($_ =~ /timeout/)
? join(' ', @$cmd) . " timed out after $timeout_duration seconds \nNote:\n$additional_info"
: 'an internal error occurred';
#$stderr_msg .= "prompt detected" if $stdout =~ /Username.*:/;
log_error($stderr_msg);
return {
status => 0,
return_code => undef,
stderr => $stderr_msg,
stdout => '',
};
};
};

}

sub asset_type_from_setting {
Expand Down
Loading