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.

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

[1.1] nsexec: Check for errors in write_log() #3721

Merged

Conversation

rata
Copy link
Member

@rata rata commented Feb 3, 2023

This is a backport of #3712 for release 1.1 branch.


First, check if strdup() fails and error out.

While we are there, the else case was missing brackets, as we only need to check ret in the else case. Fix that too

Signed-off-by: Rodrigo Campos rodrigoca@microsoft.com
(cherry picked from commit 5ce511d)

@rata
Copy link
Member Author

rata commented Feb 3, 2023

Test failures are unrelated to runc code, it seems like a network issue:

curl: (22) The requested URL returned error: 404 
Failed to get https://github.com/docker-library/busybox/raw/dist-amd64/stable/glibc/busybox.tar.xz

Will re push in a few minutes.

@rata
Copy link
Member Author

rata commented Feb 3, 2023

Hmm, it seems that link is not working anymore. Tests might need to use a new URL :(

@tianon
Copy link
Member

tianon commented Feb 3, 2023

#3701 (and the other PR it links to) 👀

@rata
Copy link
Member Author

rata commented Feb 3, 2023

@tianon do we need to backport that to 1.1, then?

@tianon
Copy link
Member

tianon commented Feb 3, 2023 via email

@rata rata mentioned this pull request Feb 6, 2023
@kolyshkin kolyshkin added this to the 1.1.5 milestone Feb 7, 2023
@kolyshkin kolyshkin mentioned this pull request Feb 7, 2023
@kolyshkin
Copy link
Contributor

@rata CI fixes were landed, please rebase.

@rata
Copy link
Member Author

rata commented Feb 9, 2023

@kolyshkin thanks! Rebased 🤞

@rata
Copy link
Member Author

rata commented Feb 9, 2023

Grr, tests are not passing due to this unit test:

=== RUN   TestFdLeaksSystemd
    exec_test.go:1838: extra fd 29 -> 
    exec_test.go:1838: extra fd 31 -> 
    exec_test.go:1841: found 2 extra fds after container.Run
--- FAIL: TestFdLeaksSystemd (0.19s)

@rata
Copy link
Member Author

rata commented Feb 9, 2023

It seems to be a flaky test, now test pass fine!

@kolyshkin should I open an issue for this flaky test?

@kolyshkin
Copy link
Contributor

It seems to be a flaky test, now test pass fine!

@kolyshkin should I open an issue for this flaky test?

Yes please (I think this is the first time I am seeing this).

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@kolyshkin
Copy link
Contributor

It seems to be a flaky test, now test pass fine!

@kolyshkin should I open an issue for this flaky test?

Opened #3735 trying to attack this (although I'm still not quite sure how the failure happened, the fix makes sense overall).

@AkihiroSuda
Copy link
Member

This branch is out-of-date with the base branch

Looks like another rebasing is needed for release-1.1 branch

First, check if strdup() fails and error out.

While we are there, the else case was missing brackets, as we only need
to check ret in the else case. Fix that too

Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
(cherry picked from commit 5ce511d)
@rata
Copy link
Member Author

rata commented Feb 10, 2023

Done, all should be green now (fedora still running, the rest is green)

@AkihiroSuda AkihiroSuda merged commit c6781d1 into opencontainers:release-1.1 Feb 10, 2023
@rata rata deleted the rata/nsfixes-backport branch February 10, 2023 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants