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

Add cwd argument for hooks exec Run func #1514

Conversation

fangpenlin
Copy link
Contributor

For fixing the wrong cwd bug in podman:

containers/podman#18907

@rhatdan
Copy link
Member

rhatdan commented Jun 17, 2023

Shouldn't we be calling this from somewhere in the code? I did not notice that they calls were all in test function.
What should a caller to this function set the CWD to?

@fangpenlin
Copy link
Contributor Author

@rhatdan The Run function is called from podman and buildah , here

https://github.com/containers/podman/pull/18921/files#diff-694b11f8e3815ed5986dc6e81cbc40ecff32b50938a38b44c8d0cb405adb3330

and here

https://github.com/containers/buildah/pull/4871/files

I checked in the common code base, it's not called anywhere except from tests.

@fangpenlin fangpenlin force-pushed the add-cwd-argument-for-hook-exec-run branch from 5ba137e to 48bdf46 Compare June 17, 2023 18:39
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

Also please squash the commits.

@vrothberg PTAL

pkg/hooks/exec/exec.go Outdated Show resolved Hide resolved
@Luap99
Copy link
Member

Luap99 commented Jun 19, 2023

Maybe taking a step back, is it anywhere defined what the cwd when executing hooks should be?
It is not mentioned here: https://github.com/opencontainers/runtime-spec/blob/v1.0.2/config.md#posix-platform-hooks
But is says you should get the state via stdin. And the state contains the bundle path: https://github.com/opencontainers/runtime-spec/blob/v1.0.2/runtime.md#state.

@vrothberg
Copy link
Member

That's a fair point, @Luap99. Technically, the cwd shouldn't matter and if the hook relies on this behavior it may work with Podman/crun but not other container engines.

@fangpenlin
Copy link
Contributor Author

fangpenlin commented Jun 19, 2023

The only reason I created the issue is because I saw a difference between hook executable invoked by crun and podman (poststop in this case). It's a bit odd to see a bit different behaivor for the hooks. But I also understand one probably should not rely on cwd for reading config.json or other files as it's not in the spec any, as the state from stdin should be used.

I guess because before making the crun call by podman, the cwd will be set to use the bundle dir, so the hook calls coming from crun is going to be the bundle dir. But for poststop hook call, it's made by podman directly, so no explicit dir set, as a result, the current working dir of podman will be used.

Please feel free to close my issues and PRs as if this is not part of the spec and not really a bug 😅

@Luap99
Copy link
Member

Luap99 commented Jun 20, 2023

I think it is a good idea overall to have a stable cwd for the hooks and not depend on the cmd of the calling process.

@rhatdan
Copy link
Member

rhatdan commented Jun 20, 2023

I agree.

@fangpenlin fangpenlin force-pushed the add-cwd-argument-for-hook-exec-run branch from 48bdf46 to 89b5dbd Compare June 23, 2023 17:39
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 23, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fangpenlin
Once this PR has been reviewed and has the lgtm label, please assign luap99 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Fang-Pen Lin <hello@fangpenlin.com>
@fangpenlin
Copy link
Contributor Author

Close in favor of #1521

@fangpenlin fangpenlin closed this Jun 26, 2023
@fangpenlin fangpenlin deleted the add-cwd-argument-for-hook-exec-run branch June 26, 2023 07:36
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