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

ReloadConfigurationFile should Reset storage options #867

Merged
merged 1 commit into from
Apr 12, 2021

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Mar 31, 2021

Currently in Podman if we reset the CONTAINERS_STORAGE_CONF path, we get
the graph driver options from the original config file as well as the
options from the override path. This PR resets the storageconf to the
initial state when called multiple times.

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

@rhatdan rhatdan force-pushed the reload branch 2 times, most recently from 1007bdd to 0838108 Compare March 31, 2021 20:17
@rhatdan rhatdan changed the title Reset GraphDriverOptions, and storageOptions when called multiple times ReloadConfigurationFile should Reset storage options Mar 31, 2021
@rhatdan rhatdan force-pushed the reload branch 2 times, most recently from b9ecc81 to d21a962 Compare March 31, 2021 20:22
@rhatdan
Copy link
Member Author

rhatdan commented Mar 31, 2021

Basically I am trying to not use /var/lib/shared as an addiitonal store.

CONTAINERS_STORAGE_CONF=/etc/containers/storage.conf.shared ./bin/podman --root /var/lib/shared pull alpine

Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

LGTM

@rhatdan
Copy link
Member Author

rhatdan commented Apr 1, 2021

@containers/podman-maintainers PTAL
@nalind @vrothberg @giuseppe PTAL

@nalind
Copy link
Member

nalind commented Apr 1, 2021

How much of storeOptions's previous value before ReloadConfigurationFile() was called is retained here? If the answer is "none of it", it'd be much simpler to clear the whole structure once at the start of the function.

types/options.go Outdated Show resolved Hide resolved
@rhatdan
Copy link
Member Author

rhatdan commented Apr 2, 2021

I don't believe we want to retain anything, so just resetting storeOptions to init value.

@rhatdan rhatdan force-pushed the reload branch 2 times, most recently from 121ca21 to 82c4a9c Compare April 6, 2021 12:16
types/options.go Outdated
@@ -278,6 +278,9 @@ func ReloadConfigurationFile(configFile string, storeOptions *StoreOptions) {
fmt.Printf("Failed to parse %s %v\n", configFile, err.Error())
return
}

// Clear storeOptions of previos settings
storeOptions = StoreOptions{}
Copy link
Member

Choose a reason for hiding this comment

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

should this be storeOptions = &StoreOptions{} ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup

Currently in Podman if we reset the CONTAINERS_STORAGE_CONF path, we get
the graph driver options from the original config file as well as the
options from the override path. This PR resets the storageconf to the
initial state when called multiple times.

Also if user sets STORAGE_OPTS="" then we should use it to override all
other storage options.

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

rhatdan commented Apr 12, 2021

@rhatdan
Copy link
Member Author

rhatdan commented Apr 12, 2021

I want to get this in so we can cut a release.

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.

LGTM

Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

LGTM

@rhatdan rhatdan merged commit 6a7e256 into containers:master Apr 12, 2021
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.

5 participants