search

LibreTexts / ngshare

nbgrader sharing service
https://ngshare.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
17 stars 17 forks source link

Make PVC accessMode configurable #120

Closed pcfens closed 4 years ago

pcfens commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

How does this look?

pcfens commented 4 years ago

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

rkevin-arch commented 4 years ago

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 commented 4 years ago

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