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.

Sign up for GitHub

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

Jump to bottom

mountOpts: dont do early failure while getting DefaultMountOpts #12681

Closed
wants to merge 1 commit into from
Closed

mountOpts: dont do early failure while getting DefaultMountOpts #12681

wants to merge 1 commit into from

Conversation

flouthoc
Copy link
Collaborator

OCI hooks can still do magic to source mount paths, so if they are not
accessible do not fail for getting DefaultMountOpts instead warn
and fallback to programmed defaults.

If anything bad happens let OCI runtime perform failure.

Closes: #12650

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 22, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc

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

The pull request process is described 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 22, 2021
@flouthoc
Copy link
Collaborator Author

flouthoc commented Dec 22, 2021 •
edited
Loading

/hold till discussion

Edit: Marking as draft till discussion.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 22, 2021
@flouthoc flouthoc marked this pull request as draft December 22, 2021 11:45
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 22, 2021
rhatdan
rhatdan reviewed Dec 22, 2021
View reviewed changes
pkg/util/mountOpts.go Outdated
@@ -136,7 +137,8 @@ func ProcessOptions(options []string, isTmpfs bool, sourcePath string) ([]string
}
defaults, err := getDefaultMountOptions(sourcePath)
if err != nil {
return nil, err
logrus.Warnf("Unable to get default mount options: %v", err)
logrus.Warnf("If not specified podman will fallback to default [noexec=false, nosuid=true, nodev=true] for mount source: %s", sourcePath)
Copy link
Member

Choose a reason for hiding this comment

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

nit: Can we get the list of defaults, rather then hard coding them into the warning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks I agree. Getting it from configured defaults.

@flouthoc
mountOpts: dont do early failure while getting DefaultMountOpts
a0f9cc6 
OCI `hooks` can still do magic to source mount paths, so if they are not
accessible do not fail instead for getting `DefaultMountOpts` instead `warn`
and fallback to configured `defaults`.

If anything bad happens let OCI runtime perform failure.

Signed-off-by: Aditya Rajan <[email protected]>
@flouthoc flouthoc force-pushed the no_stat_fail_before_oci branch from ff6dd1c to a0f9cc6 Compare December 22, 2021 12:17
@mheon
Copy link
Member

mheon commented Dec 22, 2021

Approach LGTM, but I do doubt this is going to be enough to handle the hooks use-case, I think we have checks on the source directory in other places as well.

@flouthoc
Copy link
Collaborator Author

@mheon I have tried following PR it reaches crun successfully however crun is setting mounts before calling createContainer hooks.

I think we should run createContainer hooks before setting up mounts for a container in crun but its a proposal so I'll tag @giuseppe here to discuss this and here is the proposal PR which make things work: containers/crun#827

@flouthoc flouthoc mentioned this pull request Dec 23, 2021
Closed
@flouthoc
Copy link
Collaborator Author

Upstream issue is closed with discussion that hooks are not the best thing to achieve this goal hence i am closing this one.

@flouthoc flouthoc closed this Dec 23, 2021
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rhatdan rhatdan rhatdan left review comments

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

hooks.d running stat on volumes before hooks can do volume work
3 participants
@flouthoc @mheon @rhatdan