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

rootless: write the custom config file before reload #2670

Merged
merged 3 commits into from
Mar 18, 2019
Merged

rootless: write the custom config file before reload #2670

merged 3 commits into from
Mar 18, 2019

Conversation

giuseppe
Copy link
Member

so that when we do a rootlessReload we inherit the correct settings
from the command line.

Signed-off-by: Giuseppe Scrivano [email protected]

@giuseppe
rootless: write the custom config file before reload
7efefde 
so that when we do a rootlessReload we inherit the correct settings
from the command line.

Signed-off-by: Giuseppe Scrivano <[email protected]>
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 15, 2019
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

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-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS labels Mar 15, 2019
@giuseppe
Copy link
Member Author

@tobwen please update this PR if it solves the issues you were having or if we need to do more changes

@tobwen
Copy link
Contributor

tobwen commented Mar 15, 2019

@giuseppe Sorry, no change...

$ git rev-parse HEAD
7efefde3bcb998f37bd2fcde4f004c5bbee13dd5

$ rm -rf /tmp/lib* /tmp/user* ~/.config/containers ~/.local/share/containers ~/rundir

ERRO[0000] error reading container (probably exited) json message: EOF

@mheon
Copy link
Member

mheon commented Mar 15, 2019

That's coming out of runc, looks like. Can you add a --log-level=debug and pastebin anything that pops up in syslog from Conmon (journalctl or /var/log/messages if you're not on systemd)

@tobwen
Copy link
Contributor

tobwen commented Mar 15, 2019

@mheon Sure! https://bpaste.net/show/11b4b4048cb4

@tobwen
Copy link
Contributor

tobwen commented Mar 15, 2019 •
edited
Loading

@giuseppe funny thing:

--tmpdir /tmp/libpod-rundir-$UID/tmp <-- works 
--tmpdir /tmp/libpod-rundir-$UID/libpod/tmp <-- doesn't work
--tmpdir /tmp/libpod-rundir-$UID/xxxxxx/tmp <-- doesn't work
--tmpdir /tmp/libpod-rundir-$UID/xxxxx/tmp <-- works

Seems like we're limited to less than 6 chars?

@giuseppe
utils: avoid too long tmp directory
ea0b36b 
or we will easily pass the 108 chars limits for unix paths.

Signed-off-by: Giuseppe Scrivano <[email protected]>
@tobwen
Copy link
Contributor

tobwen commented Mar 16, 2019 •
edited
Loading

@giuseppe Works for me! Thanks.

@giuseppe giuseppe changed the title [WIP] rootless: write the custom config file before reload rootless: write the custom config file before reload Mar 16, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 16, 2019
@TomSweeneyRedHat
Copy link
Member

LGTM, all green test buttons.

@giuseppe giuseppe changed the title rootless: write the custom config file before reload [WIP] rootless: write the custom config file before reload Mar 17, 2019
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 17, 2019
@giuseppe giuseppe mentioned this pull request Mar 17, 2019
Closed
@giuseppe
Copy link
Member Author

marked as WIP for the comments here: #2659

@giuseppe
utils: split generation and writing of storage.conf
232b46a 
split the generation for the default storage.conf and when we write it
if not existing for a rootless user.

This is necessary because during the startup we might be overriding
the default configuration through --storage-driver and --storage-opt,
that would not be written down to the storage.conf file we generated.

Closes: containers#2659

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe giuseppe changed the title [WIP] rootless: write the custom config file before reload rootless: write the custom config file before reload Mar 17, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 17, 2019
@giuseppe
Copy link
Member Author

dropped WIP, the patch seems to solve the issue

cc @tobwen

@tobwen
Copy link
Contributor

tobwen commented Mar 17, 2019

@giuseppe Thanks. From my side LGTM, but I'm a user only :)

@giuseppe
Copy link
Member Author

@giuseppe Thanks. From my side LGTM, but I'm a user only :)

since you reported the initial issue, your feedback is very important :-)

@rhatdan
Copy link
Member

rhatdan commented Mar 18, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 18, 2019
mheon
mheon reviewed Mar 18, 2019
View reviewed changes
pkg/util/utils.go
storageOpts.RunRoot = defaultRootlessRunRoot
}
if storageOpts.GraphRoot == "" {
storageOpts.GraphRoot = defaultRootlessGraphRoot
Copy link
Member

Choose a reason for hiding this comment

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

@giuseppe Probably a matter for a separate PR, but would it be worth also setting overlay and overlay mountprogram in the defaults here? Might fix a lot of our "overlayfs is not supported over this filesystem" errors

Copy link
Member Author

Choose a reason for hiding this comment

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

I can do that, should I hardcode it to "/usr/bin/fuse-overlayfs"?

Copy link
Member

Choose a reason for hiding this comment

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

I think that ought to be fine - even if it doesn't exist, we get a much nicer error than the current "overlay is not supported over this filesystem" one

Copy link
Member Author

Choose a reason for hiding this comment

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

hm.. looking at the code that should not really happen, if fuse-overlayfs is not found then it uses vfs

@openshift-merge-robot openshift-merge-robot merged commit ea54a1c into containers:master Mar 18, 2019
@tobwen
Copy link
Contributor

tobwen commented Mar 18, 2019

@giuseppe

It might happen id the user forces the storage to be type “overlay”.

@giuseppe
Copy link
Member Author

unprivileged overlay is enabled on some kernels, we still want to give the user an option for enabling it, so we cannot set fuse-overlayfs blindly. What we can possibly do is improving the error message

@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 27, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mheon mheon mheon left review comments

@mrunalp mrunalp Awaiting requested review from mrunalp

@rhatdan rhatdan Awaiting requested review from rhatdan

Assignees

@rhatdan rhatdan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. 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.
7 participants
@giuseppe @openshift-ci-robot @tobwen @mheon @TomSweeneyRedHat @rhatdan @openshift-merge-robot