qemu: Report all errors on virtiofsd execution by c3d · Pull Request #2685 · kata-containers/runtime

c3d · 2020-05-13T14:10:49Z

qemu: Report all errors on virtiofsd execution

The virtiofs daemon may run into errors other than the file not existing, e.g. the file may not be executable.

Message is now:

virtiofs daemon /usr/local/bin/hello returned with error: fork/exec /usr/local/bin/virtiofsd: permission denied

instead of a segmentation fault with stack trace in setupVirtiofsd:

panic: runtime error: invalid memory address or nil

Fixes: #2582

This makes the original fix for #2582 unnecessary. The result is a change in error message.

Message is now:

virtiofs daemon /usr/local/bin/hello-not-found returned with error: fork/exec /usr/local/bin/hello-not-found: no such file or directory

instead of:

virtiofsd path (/usr/local/bin/hello-no-found) does not exist

jcvenegas · 2020-05-13T14:15:46Z

/test

devimc · 2020-05-13T14:19:30Z

hold on, I see the same commit qemu: Don't leak file descriptors in case of error in other PR #2684

c3d · 2020-05-13T14:23:14Z

hold on, I see the same commit qemu: Don't leak file descriptors in case of error in other PR #2684

Yes. I saw the other one while fixing this one. I assumed that since this was a "merge" workflow, it was cleaner to make it two separate issues and two separate commits.

codecov · 2020-05-13T14:55:59Z

Codecov Report

Merging #2685 into master will increase coverage by 6.16%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #2685      +/-   ##
==========================================
+ Coverage   45.54%   51.70%   +6.16%     
==========================================
  Files         118      118              
  Lines       17281    18943    +1662     
==========================================
+ Hits         7871     9795    +1924     
+ Misses       8533     8012     -521     
- Partials      877     1136     +259

fidencio · 2020-05-13T15:06:18Z

@c3d, a few comments:

What does happen when you run your code with the virtiofsd binary removed?
Your commit message is duplicated, please remove the duplicated part;
As your PR goes against runtime and the issue is also filled against the runtime, you can use only Fixes: #NNN instead of Fixes: kata-containers#NNN

c3d · 2020-05-14T11:03:39Z

@c3d, a few comments:

What does happen when you run your code with the virtiofsd binary removed?

From commit message (I split it on two lines to make it easier to read):

virtiofs daemon /usr/local/bin/hello-not-found returned with error:
fork/exec /usr/local/bin/hello-not-found: no such file or directory

Your commit message is duplicated, please remove the duplicated part;

No, one part is for #2682, the other for #2582 (as luck would have it, single-digit difference). #2582 was your old fix, which is no longer necessary with this change, so I removed the code after checking the bug was still fixed. The commit message indicates that this results in a change of error message.

As your PR goes against runtime and the issue is also filled against the runtime, you can use only Fixes: #NNN instead of Fixes: kata-containers#NNN

I'm not sure where you see the "Fixes:kata-containers#NNN"? To me, the PR only shows #NNN. Or are you saying I should add kata-containers?

fidencio · 2020-05-14T12:00:36Z

@c3d, a few comments:

What does happen when you run your code with the virtiofsd binary removed?

From commit message (I split it on two lines to make it easier to read):
virtiofs daemon /usr/local/bin/hello-not-found returned with error:
fork/exec /usr/local/bin/hello-not-found: no such file or directory
Your commit message is duplicated, please remove the duplicated part;

No, one part is for #2682, the other for #2582 (as luck would have it, single-digit difference).

Ouch, and I read both as the same. You're right.

As your PR goes against runtime and the issue is also filled against the runtime, you can use only Fixes: #NNN instead of Fixes: kata-containers#NNN

I'm not sure where you see the "Fixes:kata-containers#NNN"? To me, the PR only shows #NNN. Or are you saying I should add kata-containers?

For some reason I saw "Fixes: kata-containers#NNN" Yesterday. But well, clearly it's not the case. So, just ignore the comment. :-)

tedyu · 2020-06-04T17:56:25Z

lgtm

jodh-intel

Thanks @c3d.

lgtm

jodh-intel · 2020-06-05T10:43:07Z

This needs to be ported to Kata 2.0 (https://github.com/kata-containers/kata-containers/tree/2.0-dev)

fidencio · 2020-06-05T10:45:59Z

/test-ubuntu

fidencio

lgtm!

fidencio · 2020-06-05T11:32:40Z

-	if err == nil {
-		q.state.VirtiofsdPid = cmd.Process.Pid
+	if err != nil {
+		return fmt.Errorf("virtiofs daemon %v returned with error: %v", q.config.VirtioFSDaemon, err);


@c3d, travis is failing here:

virtcontainers/qemu.go:675: File is not `gofmt`-ed with `-s` (gofmt) return fmt.Errorf("virtiofs daemon %v returned with error: %v", q.config.VirtioFSDaemon, err); WARN [runner/golint] Golint: can't lint 53 files: no file name for file &{Doc:0xc00dc5ac40 Package:19278537 Name:main Decls:[0xc01f5913c0 0xc01f591400 0xc01f591440 0xc01f591500 0xc01f591540 0xc01f591580 0xc01f5915c0 0xc01f591600 0xc01f591680 0xc01f591700 0xc01f591780 0xc01f591800 0xc01f591840 0xc01f591880] Scope:scope 0xc0256e55e0 { var commit var version var envCmd var configFilePathOption const name const project const projectURL const defaultRootDirectory var showConfigPathsOption const projectPrefix var checkCmd var defaultRuntimeConfiguration var defaultSysConfRuntimeConfiguration } Imports:[0xc021557710] Unresolved:[fmt fmt fmt fmt] Comments:[0xc00dc5ac40 0xc00dc5ad80 0xc00dc5ae60 0xc00dc5bdc0 0xc013c3c420 0xc013c3d460 0xc013c3d9e0 0xc013e782c0 0xc013e789a0 0xc013e79b20 0xc013e79f00]} The command ".ci/static-checks.sh" failed and exited with 1 during .

Seems that you'd need to have the (at least) first %v replaced by %s, so it wouldn't complain.

Leftover semicolon. To much coding in C :-)

And I didn't even realised that. /o.

Let me rerun the CI.

The virtiofs daemon may run into errors other than the file not existing, e.g. the file may not be executable. Fixes: kata-containers#2682 Message is now: virtiofs daemon /usr/local/bin/hello returned with error: fork/exec /usr/local/bin/virtiofsd: permission denied instead of panic: runtime error: invalid memory address or nil Fixes: kata-containers#2582 Message is now: virtiofs daemon /usr/local/bin/hello-not-found returned with error: fork/exec /usr/local/bin/hello-not-found: no such file or directory instead of: virtiofsd path (/usr/local/bin/hello-no-found) does not exist Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>

fidencio · 2020-06-05T14:36:26Z

/test-ubuntu

fidencio · 2020-06-05T16:31:30Z

Here's the link to the travis job, which never reported its result back
https://travis-ci.org/github/kata-containers/runtime/builds/695057752

fidencio · 2020-06-05T16:53:51Z

Adding the backport label as would be good to have this one backported after merged.

fidencio · 2020-06-05T17:04:57Z

@c3d, after a brief talk with @devimc, I've learned that the way to get travis-ci reporting back the results is via a repush. Unfortunately this has the drawback of having to re-run the CI. :-(

Please, would you mind re-pushing the patches?

c3d · 2020-06-05T17:56:02Z

@fidencio I re-pushed after fixing the semi-colon. I think your beams crossed.

fidencio · 2020-06-05T18:13:17Z

So, as travis is not collaborating much, I've talked to @c3d privately and we'll revisit this one next week. Now, too-late-o'clock in our timezone.

fidencio · 2020-06-09T19:13:03Z

I'm closing and re-opening this PR with the hope that it'll bring us the travis results.

fidencio · 2020-06-10T06:41:40Z

/test-power

fidencio · 2020-06-10T06:41:50Z

/test-vfio

fidencio · 2020-06-10T07:22:49Z

jenkins-ci-ubuntu-18-04-initrd
Our ol' and good Fedora error due to virtiofsd attributes.

error: unpacking of archive failed on file /usr/bin/newgidmap;5edfe711: cpio: cap_set_file failed - Inappropriate ioctl for device
error: shadow-utils-2:4.6-9.fc30.x86_64: install failed

jenkins-ubuntu-18-04-vfio
@devimc, we've seen this one before ...

08:45:41 + ssh_vm 'sudo /home/jenkins/run.sh'
08:45:41 + cmd='sudo /home/jenkins/run.sh'
08:45:41 + ssh -q -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -o IdentitiesOnly=yes -i /tmp/kata-containers/vfio-test/key -p 10022 jenkins@127.0.15.1 'sudo /home/jenkins/run.sh'
08:45:41 ++ handle_error 260
08:45:41 ++ local exit_code=255
08:45:41 ++ local line_number=260
08:45:41 ++ echo 'Failed at 260: ssh -q -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -o IdentitiesOnly=yes -i "${ssh_key_file}" -p "${vm_port}" "${USER}@${vm_ip}" "${cmd}"'
08:45:41 Failed at 260: ssh -q -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -o IdentitiesOnly=yes -i "${ssh_key_file}" -p "${vm_port}" "${USER}@${vm_ip}" "${cmd}"
08:45:41 ++ exit 255

jcvenegas approved these changes May 13, 2020

View reviewed changes

fidencio reviewed May 14, 2020

View reviewed changes

Comment thread virtcontainers/qemu.go

c3d force-pushed the github-2682-notexec-crash branch from c586f18 to e58d0fb Compare June 5, 2020 10:40

jodh-intel approved these changes Jun 5, 2020

View reviewed changes

jodh-intel added the port-to-2.0 PRs that need to be ported to kata 2.0-dev branch label Jun 5, 2020

fidencio approved these changes Jun 5, 2020

View reviewed changes

fidencio reviewed Jun 5, 2020

View reviewed changes

c3d force-pushed the github-2682-notexec-crash branch from e58d0fb to 4ee382c Compare June 5, 2020 14:33

fidencio added the backport label Jun 5, 2020

fidencio closed this Jun 9, 2020

fidencio reopened this Jun 9, 2020

fidencio mentioned this pull request Jun 10, 2020

katatestutils: Use the configured virtiofs daemon path #2687

Merged

fidencio merged commit 9d04ea7 into kata-containers:master Jun 10, 2020

Conversation

c3d commented May 13, 2020

Uh oh!

jcvenegas commented May 13, 2020

Uh oh!

devimc commented May 13, 2020

Uh oh!

c3d commented May 13, 2020

Uh oh!

codecov Bot commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fidencio commented May 13, 2020

Uh oh!

c3d commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fidencio commented May 14, 2020

Uh oh!

Uh oh!

tedyu commented Jun 4, 2020

Uh oh!

jodh-intel left a comment

Choose a reason for hiding this comment

Uh oh!

jodh-intel commented Jun 5, 2020

Uh oh!

fidencio commented Jun 5, 2020

Uh oh!

fidencio left a comment

Choose a reason for hiding this comment

Uh oh!

fidencio Jun 5, 2020

Choose a reason for hiding this comment

Uh oh!

c3d Jun 5, 2020

Choose a reason for hiding this comment

Uh oh!

fidencio Jun 5, 2020

Choose a reason for hiding this comment

Uh oh!

fidencio commented Jun 5, 2020

Uh oh!

fidencio commented Jun 5, 2020

Uh oh!

fidencio commented Jun 5, 2020

Uh oh!

fidencio commented Jun 5, 2020

Uh oh!

c3d commented Jun 5, 2020

Uh oh!

fidencio commented Jun 5, 2020

Uh oh!

fidencio commented Jun 9, 2020

Uh oh!

fidencio commented Jun 10, 2020

Uh oh!

fidencio commented Jun 10, 2020

Uh oh!

fidencio commented Jun 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov Bot commented May 13, 2020 •

edited

Loading

c3d commented May 14, 2020 •

edited

Loading