spec: add PVSource to PodPolicy #1373
Conversation
| @@ -66,26 +66,15 @@ func (bp *BackupPolicy) Validate() error { | |||
| } | |||
|
|
|||
| type StorageSource struct { | |||
| // PV represents a Persistent Volume resource, operator will claim the | |||
| // required size before creating the etcd cluster for backup purpose. | |||
There was a problem hiding this comment.
for backup purpose .
This is not true after we have PV support for etcd pod.
There was a problem hiding this comment.
That comment is for StorageSource that is a backupPolicy only option.
pkg/spec/backup.go
Outdated
| @@ -66,26 +66,15 @@ func (bp *BackupPolicy) Validate() error { | |||
| } | |||
|
|
|||
| type StorageSource struct { | |||
| // PV represents a Persistent Volume resource, operator will claim the | |||
| // required size before creating the etcd cluster for backup purpose. | |||
| // If the snapshot size is larger than the size specified, backup fails. | |||
There was a problem hiding this comment.
backup fails -> operator would kill the cluster and report failure condition in status.
I guess the right direction is that we would change our ClusterStatus to be more k8s native and add a failure condition, reason for such.
There was a problem hiding this comment.
I just copied and moved the corrent comment from PVSource to StorageSource. Are you asking for some changes in it in this patch?
There was a problem hiding this comment.
Yeah.. Please change it to
operator would kill the cluster and report failure condition in status.
There was a problem hiding this comment.
@hongchaodeng thanks, updated. It wasn't clear to me if you just wanted to update the comment (I wasn't aware of this behaviour) or you were talking about a future change and how it was related to this patch.
|
Update: design doc is done in #1374 @sgotti Thanks for doing the work!!! I'm sorry to forget earlier and mention this late. But we need a design doc for this feature. I want to mention explicitly that it is not meant to obstruct you :) Basically, we follow k8s upstream process to have design written down: https://github.com/kubernetes/community/blob/master/contributors/design-proposals/api-chunking.md We have much simpler design docs in this repo: We also do design process in multiple internal projects. So it's both open source process and our standard. What's in the design doc?Please at least answer these two questions in design docs:
You can assume:
|
pkg/spec/cluster.go
Outdated
| @@ -112,6 +123,10 @@ type ClusterSpec struct { | |||
|
|
|||
| // etcd cluster TLS configuration | |||
| TLS *TLSPolicy `json:"TLS,omitempty"` | |||
|
|
|||
| // PV represents a Persistent Volume resource. | |||
| // If defined new pods will use a persistent volume to store etcd data. | |||
There was a problem hiding this comment.
// If defined pods will use a persistent volume to store etcd data.
// When pod/node fails, the replacement pod will try to reuse the existing PV.
// When scale up, ...
// When scale down,
Basically, we probably need to describe the pv claime/cleanup policy a little bit here.
There was a problem hiding this comment.
Adding a TODO here. this is not supported as it is. We are not very strict about versioning here since etcd operator is not stable yet.
There was a problem hiding this comment.
Basically, we probably need to describe the pv claime/cleanup policy a little bit here.
Ok. Perhaps lets wait when the proposal (that talks about these logics) is accepted?
Adding a TODO here. this is not supported as it is. We are not very strict about versioning here since etcd operator is not stable yet.
That's probably the problem on splitting spec and implementation changes. Are you asking to a TODO here and removing in the implementation PR?
There was a problem hiding this comment.
yea. just a simple TODO in the comment. and remove it in the implementation.
|
I am fine with merging the spec as it is. But, yes, we need to have a design doc along with the implementation. it is easier for other people to understand how things work, and help to debug issues. |
|
@hongchaodeng @xiang90 Created the design doc: #1374. While writing it I noticed it was difficult to just put the first part in it because you won't see the final goal and what gains persistent volumes will bring with only that part. So I preferred putting down both parts and all the related notes. |
sgotti
force-pushed
the
spec_add_pvsource_to_podpolicy
branch
from
0697663
to
e7dd544
Compare
|
Updated adding an "unimplemented" TODO. About #1373 (comment) It's unclear to me what comment changes should I do. |
|
lgtm after nit |
This patch adds the initial spec changes (without implementation) to support storing etcd data inside a persistent volume. The PVSource type used by the backups policy is now shared between backups and pod policies.
sgotti
force-pushed
the
spec_add_pvsource_to_podpolicy
branch
from
e7dd544
to
3d69cd3
Compare
| // PV represents a Persistent Volume resource. | ||
| // If defined new pods will use a persistent volume to store etcd data. | ||
| // TODO(sgotti) unimplemented | ||
| PV *PVSource `json:"pv,omitempty"` |
There was a problem hiding this comment.
one thing to note here, the backup pod also need to respect (or explicitly ignore this) this in the future. right now, it saves intermediate snapshot data onto its container fs.
|
lgtm |
As requested by @hongchaodeng here (#1349 (comment)) I opened this PR to discuss only spec changes to add pv support for etcd data.
I wanted to propose to directly use StorageClass in the spec instead of the
--pv-provisioneroption but I noticed this has been already accepted during my vacations in #1361. So I used it, moving it from spec/backup.go to spec/cluster.go and added PVSource also to PodPolicy.This patch adds the initial spec changes (without implementation) to
support storing etcd data inside a persistent volume.
The PVSource type used by the backups policy is now shared between
backups and pod policies.
Since this only a spec change the required spec validation isn't part of this patch.