K8SPXC-1688 feat: add support for extraPVCs #2128
Conversation
eminaktas
requested review from
egegunes,
gkech,
hors,
nmarukovich and
pooknull
as code owners
|
|
eminaktas
force-pushed
the
feat/add-extra-pvcs
branch
from
05eea44 to
4b200ca
Compare
eminaktas
force-pushed
the
feat/add-extra-pvcs
branch
2 times, most recently
from
f311f14 to
0784a09
Compare
eminaktas
force-pushed
the
feat/add-extra-pvcs
branch
3 times, most recently
from
5079c45 to
b833676
Compare
eminaktas
force-pushed
the
feat/add-extra-pvcs
branch
2 times, most recently
from
2e9ee8b to
b4cdf92
Compare
|
hey @eminaktas, since you're still pushing changes, I wonder if this is ready for review? |
Hey @egegunes, yes, it’s ready for review now. While running a few tests, I noticed some issues caused by this change. The recent pushes include improvements and fixes I made while digging into the code for other related work. |
|
we have some thoughts about this implementation, but first let us run a small test to verify this approach. |
|
we also need to confirm the new field name (extraPVCs) with Peter to see if it's okay. |
|
Since you’re the expert on this, I went with what made the most sense to me at the time. I’d really appreciate hearing your thoughts, since you’re the maintainer and have deeper context. I haven’t looked into it too deeply yet, but one idea we could also explore is supporting volume expansion for the extra volume. |
This commit introduces the `extraPVCs` field in CRD, allowing users to attach additional PVC to main container. percona#2126 Signed-off-by: Emin Aktas <[email protected]>
eminaktas
force-pushed
the
feat/add-extra-pvcs
branch
from
b4cdf92 to
7baf735
Compare
|
@gkech could you please play with this to see if it works and feasible? |
commit: 2b1a171 |
|
Hi @eminaktas Are we still planning this as part of v1.19.0 I was also interested in this PR to mount volumes |
@remidinishanth-rubrik current implementation has problems but yes, we are still planning to have this in v1.19.0. |
gkech
changed the title
| obj.Spec.VolumeClaimTemplates = sfsVolume.PVCs | ||
| } | ||
| obj.Spec.VolumeClaimTemplates = api.AddSidecarPVCs(log, obj.Spec.VolumeClaimTemplates, podSpec.SidecarPVCs) | ||
| obj.Spec.VolumeClaimTemplates = api.AddExtraPVCs(log, obj.Spec.VolumeClaimTemplates, podSpec.ExtraPVCs) |
There was a problem hiding this comment.
VolumeClaimTemplates in a StatefulSet cannot be modified after creation. If we try to add extra PVCs to an existing StatefulSet, we will get an error like:
The StatefulSet "xyz" is invalid: spec: Forbidden: updates to statefulset spec for fields other ... are forbidden
This means that adding extra pvcs on a running cluster will not work and only fresh cluster will be able to utilize this feature, so we have a major limitation by approaching it like that.
There was a problem hiding this comment.
Can we consider cascading deletion here? In the code, there is an existing example here.
There was a problem hiding this comment.
Given the use case you shared on the GH issue you originally opened,
Use-Case
We have a scenario where our application stores certain domain-specific information into a shared volume that DB needs access to. This data is not part of the database itself, but is used in conjunction with queries and procedures. For example, it may contain reference files, large binary objects, or externally generated lookup tables.
I believe we should approach this in a slightly different angle. We can only consider adding as extra pvcs, actual pvcs that are created externally and contain the data we want already. Then these PVCs are going to be available for all sts pods with either read or write permissions (accepting all the individual storage type limitations and potential risks of the shared storage). The implementation for that is different from what we have right now available on this PR, but it is more suitable for the use case you are describing and indeed is valid.
For having the operator create additional PVCs and mount them, as you suggested an approach is the cascade deletion, but this can have various other implications that maybe are out of the context of this issue we are describing here.
8 participants
This commit introduces the
extraPVCsfield in CRD, allowing users to attach additional PVC to main container. #2126CHANGE DESCRIPTION
Problem:
Short explanation of the problem.
Cause:
Short explanation of the root cause of the issue if applicable.
Solution:
Short explanation of the solution we are providing with this PR.
CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
compare/*-oc.yml)?Config/Logging/Testability