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

Allow users to override default storage opts with --storage-opt #9911

Merged
merged 1 commit into from
Apr 5, 2021
Merged

Allow users to override default storage opts with --storage-opt #9911

merged 1 commit into from
Apr 5, 2021

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Mar 31, 2021

We define in the man page that this overrides the default storage
options, but the code was appending to the existing options.

This PR also makes a change to allow users to specify --storage-opt="".
This will turn off all storage options.

#9852

Signed-off-by: Daniel J Walsh [email protected]

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 31, 2021
@rhatdan
Copy link
Member Author

rhatdan commented Mar 31, 2021

@srcshelton PTAL

edsantiago
edsantiago reviewed Mar 31, 2021
View reviewed changes
test/system/005-info.bats Outdated
@@ -53,4 +53,8 @@ store.imageStore.number | 1

}

@test "podman info --storage-opt="" " {
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: change "" (doublequote doublequote) to '' (singlequote singlequote) for log readability.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

test/system/005-info.bats Outdated
@@ -53,4 +53,8 @@ store.imageStore.number | 1

}

@test "podman info --storage-opt="" " {
run_podman --storage-opt="" info
Copy link
Member

Choose a reason for hiding this comment

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

This completely blows up on my laptop (rootless, that is; works fine as root of course):

Error: kernel does not support overlay fs: 'overlay' is not supported over btrfs at "/home/esm/.local/share/containers/storage/overlay": backing file system is unsupported for this graph driver

docs/source/markdown/podman.1.md Outdated
@@ -148,7 +148,7 @@ specify additional options via the `--storage-opt` flag.

#### **\-\-storage-opt**=*value*

Storage driver option, Default storage driver options are configured in /etc/containers/storage.conf (`$HOME/.config/containers/storage.conf` in rootless mode). The `STORAGE_OPTS` environment variable overrides the default. The --storage-opt specified options overrides all.
Storage driver option, Default storage driver options are configured in /etc/containers/storage.conf (`$HOME/.config/containers/storage.conf` in rootless mode). The `STORAGE_OPTS` environment variable overrides the default. The --storage-opt specified options overrides all. If you specify the --storage-opt="", no storage options will be used.
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary the

@TomSweeneyRedHat
Copy link
Member

Unhappy red tests.
LGTM once @edsantiago 's comments are addressed.

@srcshelton
Copy link
Contributor

@srcshelton PTAL

So, to confirm, this replaces the --root argument, or complements it?

If storage.conf defines the graphRoot directory as /a and an additionalStorage location of /b, then what command-line/environment variable configuration would I need to be able to pull or build an image directly into directory/b?

@rhatdan
Copy link
Member Author

rhatdan commented Apr 2, 2021

Complements it. Basically --root follows the settings in storage.conf. --storage-opt will ignore the storage.conf settings.

@srcshelton
Copy link
Contributor

Complements it. Basically --root follows the settings in storage.conf. --storage-opt will ignore the storage.conf settings.

I'm afraid that I'm not sure I understand how these two options are intended to be used… would be be able to give an example command-line which either pulls or builds a container definition into a directory declared as an additionalStorage location but which it not the main graphRoot?

podman --root /path/to/additional/storage --storage-opt="" pull image:tag or similar?

(Is the intent that storage-opt can be used without a value in order to clear the storage.conf settings, and then root causes the additionalStorage directory - if specified - to be the location the image is written to?)

@rhatdan
Copy link
Member Author

rhatdan commented Apr 5, 2021

@srcshelton Yes, exactly you use case.

@rhatdan
Allow users to override default storage opts with --storage-opt
69ace20 
We define in the man page that this overrides the default storage
options, but the code was appending to the existing options.

This PR also makes a change to allow users to specify --storage-opt="".
This will turn off all storage options.

containers#9852

Signed-off-by: Daniel J Walsh <[email protected]>
@rhatdan
Copy link
Member Author

rhatdan commented Apr 5, 2021

@containers/podman-maintainers PTAL

@edsantiago
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 5, 2021
@openshift-merge-robot openshift-merge-robot merged commit 6b23fec into containers:master Apr 5, 2021
@srcshelton
Copy link
Contributor

Is there any use-case for --root which doesn't require --storage-opt="" to clear any pre-existing configuration (… perhaps running without any storage.conf whereby everything has to be passed on the command-line)?

Should podman then implicitly clear any defined storage options if --root is used, even if --storage-opt isn't specified?

@srcshelton srcshelton mentioned this pull request Apr 8, 2021
Closed
@vikas-goel vikas-goel mentioned this pull request Jul 10, 2021
Closed
@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 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@edsantiago edsantiago edsantiago left review comments

Assignees

@edsantiago edsantiago

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.
6 participants
@rhatdan @openshift-ci-robot @TomSweeneyRedHat @srcshelton @edsantiago @openshift-merge-robot