-
Notifications
You must be signed in to change notification settings - Fork 83
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 GetProcessSettings not return volumeClaimTemplate for stateless … #1965
make GetProcessSettings not return volumeClaimTemplate for stateless … #1965
Conversation
b1f93dc
to
19e3efc
Compare
Result of fdb-kubernetes-operator-pr on Linux CentOS 7
|
Result of fdb-kubernetes-operator-pr on Linux CentOS 7
|
Result of fdb-kubernetes-operator-pr on Linux CentOS 7
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes in general look good to me, could you make sure that the tests are passing?
Result of fdb-kubernetes-operator-pr on Linux CentOS 7
|
f736650
to
5d7a123
Compare
Result of fdb-kubernetes-operator-pr on Linux CentOS 7
|
Result of fdb-kubernetes-operator-pr on Linux CentOS 7
|
514f05a
to
2f3c192
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
My recollection is that we added a deliberate mechanism to use a non-persistent volume for storage pods in the early stages of the operator development, as a workaround for issues with PVC compatibility. Those issues have since been resolved, so I agree that this option is more dangerous than helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Result of fdb-kubernetes-operator-pr on Linux CentOS 7
|
…processes
Description
This addresses #1310.
Some odd logic was discovered in the
usePvc()
function, which roughly resulted in its logic being "use PVC if a processGroup is stateful and does not have a volumeClaimTemplate (nil), or if a processGroup is stateful and has a non-zero request for storage resources". This seemed a bit counter-intuitive, and could result in the case where someone accidentally creates storage pods with no PVC, which is dangerous for a non-test FDB cluster.As a result, I removed the
usePvc()
function and replaced it with aprocessClass.isStateful()
check. I think this more closely matches the original intent. (As of writing, it is valid for a stateful process to have a nil volumeClaimTemplate, see #1966, so, as far as I can tell the only meaningful check isisStateful()
)Type of change
Please select one of the options below.
"Bug" fix (non-breaking change which fixes an issue)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Other
Discussion
See forum post https://forums.foundationdb.org/t/stateful-processgroups-can-no-longer-be-run-without-pvcs-operator-1-35/4391
Testing
unit tests
Documentation
Follow-up