Skip to content

runroot: add check that it is on volatile storage #317

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
Closed
giuseppe wants to merge 1 commit into from
Closed

runroot: add check that it is on volatile storage #317

giuseppe wants to merge 1 commit into from

Conversation

@giuseppe
Copy link
Member

@giuseppe giuseppe commented Apr 12, 2019

Make sure the runroot won't persist after a reboot, if it happens then
we can carry wrong information on the current active mounts.

Closes: containers/podman#2150

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

@giuseppe giuseppe mentioned this pull request Apr 12, 2019
Closed
@giuseppe giuseppe force-pushed the add-check-runroot-tmpfs branch from 04ce96b to 6c4cf83 Compare April 12, 2019 11:49
@rhatdan
Copy link
Member

rhatdan commented Apr 12, 2019

LGTM
@nalind @vrothberg PTAL

@rhatdan
Copy link
Member

rhatdan commented Apr 15, 2019

@TomSweeneyRedHat @nalind @vrothberg PTAL

vrothberg
vrothberg reviewed Apr 16, 2019
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

The logic LGTM but I think we need the check in multiple paths. Maybe we can add an internal (s *store) validate() error func?

@giuseppe giuseppe force-pushed the add-check-runroot-tmpfs branch from 6c4cf83 to 4bf47bf Compare April 17, 2019 11:02
@rhatdan
Copy link
Member

rhatdan commented Apr 17, 2019

@giuseppe This breaks tests. I think you will to mount a tmpfs on local storage to get this to pass. I wonder if this will break CI/CD Systems when we test also. Should we have a way to ignore this test.

@giuseppe giuseppe force-pushed the add-check-runroot-tmpfs branch from 4bf47bf to fc244d6 Compare April 17, 2019 14:32
rhatdan
rhatdan reviewed Apr 17, 2019
View reviewed changes
store.go Outdated
if onTmpfs, err := mount.IsOnVolatileStorage(runRoot); err != nil || !onTmpfs {
if err != nil {
return errors.Wrapf(err, "cannot check if %s is on tmpfs", runRoot)
} else {
Copy link
Member

@rhatdan rhatdan Apr 17, 2019

Choose a reason for hiding this comment

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

Nit: No need for the else, just keep the return line.

@giuseppe giuseppe force-pushed the add-check-runroot-tmpfs branch 2 times, most recently from 4a1e4fc to 797c7ad Compare April 18, 2019 12:36
TomSweeneyRedHat
TomSweeneyRedHat reviewed Apr 18, 2019
View reviewed changes
Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

LGTM

@vrothberg
Copy link
Member

vrothberg commented Apr 18, 2019

CI is not yet happy:

go build -compiler gc -tags " libdm_no_deferred_remove ostree "  ./cmd/containers-storage
# github.com/containers/storage/pkg/mount
pkg/mount/mountinfo_linux.go:111:21: undefined: err
pkg/mount/mountinfo_linux.go:112:31: undefined: err

@giuseppe giuseppe force-pushed the add-check-runroot-tmpfs branch from 797c7ad to 77a5f90 Compare April 18, 2019 15:44
@rhatdan
Copy link
Member

rhatdan commented Apr 18, 2019

@giuseppe Could you set up a WIP vendor of this patch into libpod and make sure the CI system passes.

@giuseppe
Copy link
Member Author

giuseppe commented Apr 19, 2019

opened here: containers/podman#2976

@vrothberg
Copy link
Member

vrothberg commented Apr 23, 2019

opened here: containers/podman#2976

CI is not running on a tmpfs :^)

@rhatdan
Copy link
Member

rhatdan commented Apr 23, 2019

This is also going to cause issues with running containers within containers, will now require /run have a tmpfs mounted on /run

@giuseppe giuseppe force-pushed the add-check-runroot-tmpfs branch from 77a5f90 to c566b2f Compare April 23, 2019 14:10
@giuseppe
Copy link
Member Author

giuseppe commented Apr 23, 2019

This is also going to cause issues with running containers within containers, will now require /run have a tmpfs mounted on /run

I've pushed a new version where it is possible to disable this check. I've also updated the WIP PR for podman to set it correctly when the "container" environment variable is set

@giuseppe giuseppe force-pushed the add-check-runroot-tmpfs branch from c566b2f to 6518b34 Compare April 24, 2019 07:00
@giuseppe giuseppe force-pushed the add-check-runroot-tmpfs branch from 6518b34 to 20b8545 Compare April 24, 2019 15:06
nalind
nalind reviewed Apr 25, 2019
View reviewed changes
pkg/mount/mountinfo_linux.go Outdated
(11) super options: per super block options*/
mountinfoFormat = "%d %d %d:%d %s %s %s %s"

TMPFS_MAGIC = 0x1021994
Copy link
Member

@nalind nalind Apr 25, 2019

Choose a reason for hiding this comment

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

Should probably reuse github.com/containers/storage/drivers.FsMagicTmpfs and the adjacent GetFSMagic() function here.

Copy link
Member Author

@giuseppe giuseppe Apr 25, 2019

Choose a reason for hiding this comment

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

should I move these functions to github.com/containers/storage/drivers? Otherwise it introduces an import cycle.

Copy link
Member

@rhatdan rhatdan Apr 25, 2019

Choose a reason for hiding this comment

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

Maybe pkg/util?

Copy link
Member

@rhatdan rhatdan Apr 25, 2019

Choose a reason for hiding this comment

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

Never mind that does not exists.

Copy link
Member

@rhatdan rhatdan Apr 25, 2019

Choose a reason for hiding this comment

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

Seems to make sense.

@giuseppe giuseppe force-pushed the add-check-runroot-tmpfs branch from 20b8545 to fae4ed4 Compare April 25, 2019 21:43
@giuseppe
runroot: add check that it is on volatile storage
3c4f600 
Make sure the runroot won't persist after a reboot, if it happens then
we can carry wrong information on the current active mounts.

Closes: containers/podman#2150

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe giuseppe force-pushed the add-check-runroot-tmpfs branch from fae4ed4 to 3c4f600 Compare April 26, 2019 06:59
@nalind
Copy link
Member

nalind commented May 1, 2019

LGTM, with one nit: we appear to be checking the location if one is specified in the configuration file, but not if one isn't. If that's intended, then it's fine.

@rhatdan
Copy link
Member

rhatdan commented May 1, 2019

This still concerns me for the buildah inside of a podman container case, or inside of a docker or other container runtime. We need a mechanism to figure out whether we are running inside of a container. Does Docker always set the container environment variable?

giuseppe added a commit to giuseppe/libpod that referenced this pull request May 3, 2019
@giuseppe
WIP: test for containers/storage#317
6417924 
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe
Copy link
Member Author

giuseppe commented May 6, 2019

docker doesn't seem to set that. I am going to close this PR for now, as it probably introduces more issues than it solves

@giuseppe giuseppe closed this May 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nalind nalind nalind left review comments

@rhatdan rhatdan rhatdan left review comments

@vrothberg vrothberg vrothberg left review comments

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Container doesn't start after a system reboot

5 participants

@giuseppe @rhatdan @vrothberg @nalind @TomSweeneyRedHat