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

Make PVC accessMode configurable #120

Merged
merged 4 commits into from Jul 28, 2020

Conversation

pcfens
Copy link
Contributor

@pcfens pcfens commented Jul 28, 2020

Rather than making the PV accessMode defined by the replica count this allows users to set it directly. Tieing the accessMode to
replicaCounts make it difficult for users to make changes in the future without also forcing an accessMode change, which can be disruptive.

The previous logic set it so that a PVs were provisioned with ReadWriteMany when defaults are used, this retains that behavior.

Rather than making the PV accessMode defined by the replica count
this allows users to set it directly. Tieing the accessMode to
replicaCounts make it difficult for users to make changes in the
future without also forcing an accessMode change, which can be disruptive.

The previous logic set it so that a PVs were provisioned with
ReadWriteMany when defaults are used, this retains that behavior.
@rkevin-arch
Copy link
Member

LGTM, although IMO we should keep the default behavior (as in, if it isn't set in the helm values, the accessMode should be ReadWriteOnce if there's 1 replica or ReadWriteMany if there's more than 1), just to keep it backwards compatible (since some PV provisioners do not support ReadWriteMany). That's just my opinion, though, feel free to disagree.

Also, currently ngshare uses SQLite as the database, and we aren't sure if multiple instances of ngshare can use the same underlying SQLite database mounted ReadWriteMany without race conditions. Just a heads up.

@pcfens
Copy link
Contributor Author

pcfens commented Jul 28, 2020

I agree, but when I render the template locally I get ReadWriteMany instead of the expected ReadWriteOnce (that's what originally had me go to look at this). I think the conditional in the PVC template is backwards (ReadWriteOnce is set only when the replica count is greater than 1).

The PR maintains the current behavior when replicas equals 1. If that's a bug I'm happy to submit a patch for that too.

@rkevin-arch
Copy link
Member

Whoops, that's definitely a bug. I guess we can just add a note in the changelog and mention this bug, and the default behavior can just be ReadWriteMany and users can override it if it's an issue for them.

@rkevin-arch
Copy link
Member

How does this look?

@pcfens
Copy link
Contributor Author

pcfens commented Jul 28, 2020

LGTM - do you want to change the default value here or leave it as ReadWriteMany?

@rkevin-arch
Copy link
Member

I think leaving it as ReadWriteMany is fine if no existing users has complained about the bug until now. Future new users can just change it if necessary.

@pcfens
Copy link
Contributor Author

pcfens commented Jul 28, 2020

Sounds good - I know changing defaults can cause weird problems for folks.

@rkevin-arch rkevin-arch merged commit b71f390 into LibreTexts:master Jul 28, 2020
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.

None yet

2 participants