do not log error in the syscall crate #1973

yihuaf · 2023-05-27T03:47:54Z

Related to #1974. The integration tests uses stderr to check if there are any errors. However, there are a number of cases where we want to mount with ENOTDIR or ENOENT error but we ignore these errors. Therefore, we do not want to log these errors. Otherwise the integration tests would be broken.

tag along: Bump rust version to 1.70 and fix some comments lint in integration test. As a side effect, this will trigger the integration validation test, so we can test the fix is actually working in CI.

codecov-commenter · 2023-05-27T03:51:21Z

Codecov Report

Merging #1973 (15c7c56) into main (cb75d26) will decrease coverage by 0.02%.
The diff coverage is 21.95%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1973      +/-   ##
==========================================
- Coverage   65.28%   65.26%   -0.02%     
==========================================
  Files         129      129              
  Lines       14784    14800      +16     
==========================================
+ Hits         9651     9659       +8     
- Misses       5133     5141       +8

utam0k

What about tracing at the caller? It is beyond the role of the syscall mod to determine the type of error.

yihuaf · 2023-05-30T17:11:45Z

What about tracing at the caller? It is beyond the role of the syscall mod to determine the type of error.

I made the change for mount only in this PR to unblock the CI. We can extend this to all syscalls in this mod, but I'd like to do this in another PR.

utam0k · 2023-05-31T11:16:25Z

We can extend this to all syscalls in this mod, but I'd like to do this in another PR.

I agree with you. we should do it.

utam0k · 2023-05-31T11:17:29Z

crates/libcontainer/src/syscall/linux.rs

@@ -489,13 +490,7 @@ impl Syscall for LinuxSyscall {
        flags: MsFlags,
        data: Option<&str>,
    ) -> Result<()> {
-        mount(source, target, fstype, flags, data).map_err(|err| {
-            tracing::error!(


Is there any need to bring this tracing to the callers?

Yes, 47bc859

Signed-off-by: yihuaf <[email protected]>

yihuaf · 2023-06-01T18:05:14Z

@utam0k since this PR hasn't been merged, I went ahead and clean up the syscall crate as well. All the simpler wrappers no longer log error and logs only at the caller site. I kept the other more complex functions such as pivot_root and close_range alone, since it makes sense to log the error there.

Signed-off-by: yihuaf <[email protected]>

yihuaf · 2023-06-05T03:40:34Z

@utam0k Can we get a review if you have some time? I would like to merge this since this will unblock the integration validation test. I would also ask if it is possible to merge this in as is and address any reviews in a follow up PR since the integration validation CI is failing.

YJDoc2 · 2023-06-05T05:19:48Z

Hey @yihuaf I think utam0k is unwell, so I'll take a look at this instead. The changes look ok, and as you said, if there are any issues, we can fix them in follow up PR. The only question I have is that this was opened because we logged errors in mount calls which makes the tests fail. However, I still see that we are doing tracing::error in the mount calls' map_err . I think you had removed it in one of the commits, but it got re-added for some reason. Can you please correct me if I'm wrong? Otherwise this is fine, I'll approve this then so it can unblock rest of the work.

yihuaf · 2023-06-05T06:07:41Z

Hey @yihuaf I think utam0k is unwell, so I'll take a look at this instead. The changes look ok, and as you said, if there are any issues, we can fix them in follow up PR. The only question I have is that this was opened because we logged errors in mount calls which makes the tests fail. However, I still see that we are doing tracing::error in the mount calls' map_err . I think you had removed it in one of the commits, but it got re-added for some reason. Can you please correct me if I'm wrong? Otherwise this is fine, I'll approve this then so it can unblock rest of the work.

@YJDoc2 Thank you for taking a look. So issue that caused the integration failure is the syscall::mount logging error. It is removed in this PR (https://github.com/containers/youki/pull/1973/files#diff-b7916a1b6e425d0953faba26141b604a4b71e65782a58c78dd6f59f620916dccR470). The right thing to do is logging at the caller of the syscall::mount. There are a few places where the syscall::mount will return ENOTDIR or ENOENT, but the caller ignores these error because it is expected. This happens for example in the masked_path (https://github.com/containers/youki/pull/1973/files#diff-9773882fbf80be1f21b71393425530b52a9fc276cd89e5f0910d2435ca45fbb5R217). In this case, ENOENT is considered successful because a non-existent path is considered to be masked already (not accessible).

I believe all the tracing add you see are at the caller of syscall::mount. In most cases, the mount call failure indicates an actual failure, so we log them, unless the error case is handled.

I will also add one more commit to trigger the integration_validation test, so we can be complete in this PR.

yihuaf · 2023-06-05T06:13:36Z

crates/libcontainer/src/rootfs/rootfs.rs

+                MsFlags::MS_BIND | MsFlags::MS_REC,
+                None,
+            )
+            .map_err(|err| {


For example, this call to syscall::mount is inside the rootfs module. Here, mount call failure indicates an actual failure, so we log the error here.

YJDoc2 · 2023-06-05T06:15:52Z

Hey @yihuaf Thanks for the detailed explanation! I think I got the actual call site and implementation mixed up originally. This looks good then, and once the CI pass, I'll approve and merge. Thanks a lot!

YJDoc2

LGTM 👍

yihuaf force-pushed the yihuaf/mount_error branch from 50d0306 to 4dbf3c0 Compare May 27, 2023 04:22

yihuaf requested review from utam0k, YJDoc2 and a team May 27, 2023 04:24

yihuaf mentioned this pull request May 27, 2023

add rsymfollow recursive mount test #1967

Merged

utam0k requested changes May 30, 2023

View reviewed changes

yihuaf force-pushed the yihuaf/mount_error branch from 4dbf3c0 to 802ae7a Compare May 30, 2023 17:10

yihuaf requested a review from utam0k May 30, 2023 17:11

utam0k reviewed May 31, 2023

View reviewed changes

yihuaf force-pushed the yihuaf/mount_error branch from c50e0ef to 47bc859 Compare May 31, 2023 16:13

yihuaf requested a review from utam0k May 31, 2023 16:14

yihuaf self-assigned this May 31, 2023

yihuaf added 4 commits June 1, 2023 11:01

do not log error for mount in specific cases

a72c615

Signed-off-by: yihuaf <[email protected]>

fix the review

085f951

Signed-off-by: yihuaf <[email protected]>

fix review

da5ccb3

Signed-off-by: yihuaf <[email protected]>

clean up the logs in the syscall module

ecf9f8d

Signed-off-by: yihuaf <[email protected]>

yihuaf force-pushed the yihuaf/mount_error branch from 47bc859 to ecf9f8d Compare June 1, 2023 18:02

yihuaf changed the title ~~do not log error for mount in specific cases~~ do not log error in the syscall crate Jun 1, 2023

yihuaf mentioned this pull request Jun 2, 2023

Rust 1.70 #1996

Closed

yihuaf added 2 commits June 3, 2023 18:30

update toolchain to rust 1.70

e86ae4d

Signed-off-by: yihuaf <[email protected]>

fix new lint

15c7c56

Signed-off-by: yihuaf <[email protected]>

yihuaf commented Jun 5, 2023

View reviewed changes

YJDoc2 approved these changes Jun 5, 2023

View reviewed changes

yihuaf merged commit d09c984 into youki-dev:main Jun 5, 2023

yihuaf deleted the yihuaf/mount_error branch June 5, 2023 06:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

do not log error in the syscall crate #1973

do not log error in the syscall crate #1973

yihuaf commented May 27, 2023 •

edited

Loading

codecov-commenter commented May 27, 2023 •

edited

Loading

utam0k left a comment

yihuaf commented May 30, 2023

utam0k commented May 31, 2023

utam0k May 31, 2023

yihuaf May 31, 2023

yihuaf commented Jun 1, 2023

yihuaf commented Jun 5, 2023

YJDoc2 commented Jun 5, 2023 •

edited

Loading

yihuaf commented Jun 5, 2023

yihuaf Jun 5, 2023

YJDoc2 commented Jun 5, 2023

YJDoc2 left a comment

do not log error in the syscall crate #1973

do not log error in the syscall crate #1973

Conversation

yihuaf commented May 27, 2023 • edited Loading

codecov-commenter commented May 27, 2023 • edited Loading

Codecov Report

utam0k left a comment

Choose a reason for hiding this comment

yihuaf commented May 30, 2023

utam0k commented May 31, 2023

utam0k May 31, 2023

Choose a reason for hiding this comment

yihuaf May 31, 2023

Choose a reason for hiding this comment

yihuaf commented Jun 1, 2023

yihuaf commented Jun 5, 2023

YJDoc2 commented Jun 5, 2023 • edited Loading

yihuaf commented Jun 5, 2023

yihuaf Jun 5, 2023

Choose a reason for hiding this comment

YJDoc2 commented Jun 5, 2023

YJDoc2 left a comment

Choose a reason for hiding this comment

yihuaf commented May 27, 2023 •

edited

Loading

codecov-commenter commented May 27, 2023 •

edited

Loading

YJDoc2 commented Jun 5, 2023 •

edited

Loading