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

Remove chdir in container init and start #2777

Closed
wants to merge 3 commits into from

Conversation

Mossaka
Copy link
Contributor

@Mossaka Mossaka commented May 1, 2024

Remove unnecessary chdir in container init and start, the invokes seem not necessary.

Pending comments from #2772 (comment) and will close issue #2772

@Mossaka Mossaka changed the title Remove chdir Remove chdir in container init and start May 1, 2024
@YJDoc2
Copy link
Collaborator

YJDoc2 commented May 2, 2024

Hey, the CI is failing because cross now requires rust 1.77.2 and we use 1.77.1. Have opened #2779 which should fix all the CI running issues, so once that is merged, you can rebase on that.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented May 2, 2024

It is merged, so you can go ahead 👍

the invoke to chdir is immediately called again in notify_socket.notify_container_start()
and it seems not necessary. This commit removes it

Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>
Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>
@jprendes
Copy link
Contributor

jprendes commented May 2, 2024

Hey @Mossaka , I've rebased your branch, I hope that's fine.

@YJDoc2 YJDoc2 added the kind/bug label May 2, 2024
Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
@Mossaka
Copy link
Contributor Author

Mossaka commented May 2, 2024

It seems like the integration test failed. I am not able to tell if it's related to my changes

@jprendes jprendes closed this May 2, 2024
@jprendes jprendes reopened this May 2, 2024
Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

Please wait until we investigat this as it could break pseudo terminal.

@Mossaka
Copy link
Contributor Author

Mossaka commented May 2, 2024

Please wait until we investigat this as it could break pseudo terminal.

Sounds good. That's why I made this PR a draft one until we figure out if it's not breaking existing functionality of youki.

@jprendes
Copy link
Contributor

jprendes commented May 2, 2024

3 tests failed, all related to setting up tty.

        Error: exec process failed with error error in executing process : failed to setup tty
        : unknown
--- FAIL: TestContainerPTY (0.15s)
        Error: exec process failed with error error in executing process : failed to setup tty
        : unknown
--- FAIL: TestTaskResize (0.08s)
    container_test.go:1890: OCI runtime exec failed: runc did not terminate successfully: exit status 255: exec failed : exec process failed with error error in executing process : failed to setup tty
        : unknown
--- FAIL: TestContainerExecLargeOutputWithTTY (0.13s)

@utam0k
Copy link
Member

utam0k commented May 3, 2024

I was worried about catching it on the test, but it looks like they were testing it! I want to thank our past selves.

@utam0k utam0k mentioned this pull request May 3, 2024
@utam0k
Copy link
Member

utam0k commented May 3, 2024

Sorry, but I've created another PR to fix it.
#2780

@utam0k utam0k closed this May 3, 2024
@Mossaka Mossaka deleted the remove-chdir branch May 6, 2024 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants