*: implement PersistentVolume for etcd data design part 1 #1434

sgotti · 2017-09-22T08:07:03Z

this patch implements part 1 of the etcd data on persistent volumes design.

when pod pvsource is defined in the spec it'll create a PVC for every etcd
member and use it as the volume for etcd data.

pvc without a member will be removed during the reconcile.

NOTES

As discussed during the design phase I haven't changed to pod restart policy to Always. So beside putting data on a PV this patch shoudln't change any other logic and behavior.

sgotti · 2017-09-22T08:09:16Z

hack/test

 	go test "./test/e2e/" -run "$E2E_TEST_SELECTOR" -timeout 30m --race --kubeconfig $KUBECONFIG --operator-image $OPERATOR_IMAGE --namespace ${TEST_NAMESPACE}
+	# Run tests with PV support enabled
+	PV_TEST=true go test "./test/e2e/" -run "$E2E_TEST_SELECTOR" -timeout 30m --race --kubeconfig $KUBECONFIG --operator-image $OPERATOR_IMAGE --namespace ${TEST_NAMESPACE}


To see how the tests works on your jenkins I just rerun them enabling pv support. Do you have better ideas on how to do this without duplicating the code?

Create a separate set of test.
For example, test/e2e/etcd_on_pv_test.go .
We will pool some PV ahead to reduce time overhead and cost on cloud.

Since this part doesn't change the logic but only places etcd data on a PV I was thinking to redo all the tests with PV enabled. Using a different test files means a lot of duplicated code and maintenance burden. Perhaps we could have just different timeouts for the non PV and PV cases?

@hongchaodeng gentle ICMP ECHO_REQUEST. I'm quite blocked trying to understand how you'd like to organize tests. See above comment.

Truly apologize... This message is inundated in too many notifications...

Yes. That sounds right. But the thing I worry about is that this way it creates too many temporary persistent disks on our GCE and is costly... For initial stage, let's just create one test to make sure it work. We can think of ideas to poll those PVs later?

sgotti · 2017-09-22T08:10:32Z

test/e2e/basic_test.go

@@ -40,7 +40,7 @@ func TestCreateCluster(t *testing.T) {
 		}
 	}()

-	if _, err := e2eutil.WaitUntilSizeReached(t, f.CRClient, 3, 6, testEtcd); err != nil {
+	if _, err := e2eutil.WaitUntilSizeReached(t, f.CRClient, 3, 20, testEtcd); err != nil {


Since creating and mounting PVs takes some time I just blindly increased the retries. Better ideas?

Don't change any timeout in test.
All test timeout is delicately tuned because we understand why we allocate this much time and have expectation how long each operation takes.

Let's create a separate set of test for etcd pv scenario.
Additionally, we will probably poll some PVs before test in the future. But we don't need to worry about this right now.

sgotti · 2017-09-22T08:15:15Z

pkg/cluster/cluster.go

+
+	// TODO: We set timeout to 60s here since PVC binding could take up to 60s for GCE/PD. See https://github.com/kubernetes/kubernetes/issues/40972 .
+	//       Change the wait time once there are official p99 SLA.
+	err = retryutil.Retry(4*time.Second, 15, func() (bool, error) {


This part is copied from https://github.com/coreos/etcd-operator/blob/master/pkg/util/k8sutil/backup.go#L63. I think is shoul unified but this will need some more refactoring. One key difference is that etcd pods and (with this patch) PVCs are created (the api call) here while for backups they are created inside pkg/util/k8sutil.

Should not wait PVC ready here.
It should reconcile until PVC is ready before creating the pod.

sgotti · 2017-09-22T08:19:06Z

pkg/cluster/cluster.go

+	}
+
+	// TODO: We set timeout to 60s here since PVC binding could take up to 60s for GCE/PD. See https://github.com/kubernetes/kubernetes/issues/40972 .
+	//       Change the wait time once there are official p99 SLA.


Should we wait for pvc to be created or should we just define them and then continue creating the pod? It'll work also without waiting and the pod will be scheduled only when the pvc goes in bound state.

Should not wait PVC ready here.
It should reconcile until PVC is ready before creating the pod.

sgotti · 2017-09-22T08:24:06Z

pkg/util/k8sutil/k8sutil.go

+				"etcd_node":    m.Name,
+				"etcd_cluster": clusterName,
+				"app":          "etcd",
+			},


@hongchaodeng I remember that you said to just set the owner ref without using labels and I agree. I just noticed that with the current gc logic this could lead to a lot of logged warnings (https://github.com/coreos/etcd-operator/pull/1434/files#diff-40a34818cc02bcfb053e6102e57f3177R173 ) since every pvc in the namespace is inspected. So I added the same labels used for the other resources just to do an initial filtering.

Not sure what's the context.
Re. labels, use:

"etcd_cluster": clusterName, "app": "etcd",

is good enough

xiang90 · 2017-09-22T18:08:53Z

pkg/apis/etcd/v1beta2/cluster.go

@@ -27,6 +27,8 @@ import (
 const (
 	defaultBaseImage = "quay.io/coreos/etcd"
 	defaultVersion   = "3.1.8"
+
+	minPVSize = 512 // 512MiB


minPVSizeInMB

minPodPV...

xiang90 · 2017-09-22T19:37:27Z

pkg/apis/etcd/v1beta2/cluster.go

@@ -206,6 +207,11 @@ func (c *ClusterSpec) Validate() error {
 				return errors.New("spec: pod labels contains reserved label")
 			}
 		}
+		if c.Pod.PV != nil {
+			if c.Pod.PV.VolumeSizeInMB < minPVSize {


maybe we just set it back to min instead of erroring it? i forgot about how k8s resource limit works here for the similar case though. probably we want the consistency here.

Having a default sounds a better option to prevent user mistakes.

Additionally, we should also write docs to recommend setting limitrange to require minimum storage:
https://kubernetes.io/docs/tasks/administer-cluster/limit-storage-consumption/#limitrange-to-limit-requests-for-storage

xiang90 · 2017-09-22T19:38:27Z

pkg/cluster/cluster.go

@@ -404,7 +411,12 @@ func (c *Cluster) startSeedMember(recoverFromBackup bool) error {
 		SecureClient: c.isSecureClient(),
 	}
 	ms := etcdutil.NewMemberSet(m)
-	if err := c.createPod(ms, m, "new", recoverFromBackup); err != nil {
+	if c.UsePodPV() {


ShouldUsePodPV or IsPVEnabled?

IsPodPVEnabled ?

xiang90 · 2017-09-22T19:39:16Z

pkg/cluster/cluster.go

+			return fmt.Errorf("failed to create persistent volume claim for seed member (%s): %v", m.Name, err)
+		}
+	}
+	if err := c.createPod(ms, m, "new", recoverFromBackup, c.UsePodPV()); err != nil {


no need to pass in usePodPV here. the receiver is still onc.

xiang90 · 2017-09-22T19:41:18Z

pkg/garbagecollection/gc.go

@@ -158,3 +161,28 @@ func (gc *GC) collectDeployment(option metav1.ListOptions, runningSet map[types.

 	return nil
 }
+
+func (gc *GC) collectPVCs(option metav1.ListOptions, runningSet map[types.UID]bool) error {


@sgotti

when this lands, we probably already move to k8s 1.8, which supports crd GC natively. so we do not need this thing anymore.

/cc @hongchaodeng

+1
We will rm -rf entire custom GC code :)

hongchaodeng · 2017-09-22T18:20:14Z

hack/test

 	go test "./test/e2e/" -run "$E2E_TEST_SELECTOR" -timeout 30m --race --kubeconfig $KUBECONFIG --operator-image $OPERATOR_IMAGE --namespace ${TEST_NAMESPACE}
+	# Run tests with PV support enabled
+	PV_TEST=true go test "./test/e2e/" -run "$E2E_TEST_SELECTOR" -timeout 30m --race --kubeconfig $KUBECONFIG --operator-image $OPERATOR_IMAGE --namespace ${TEST_NAMESPACE}


Create a separate set of test.
For example, test/e2e/etcd_on_pv_test.go .
We will pool some PV ahead to reduce time overhead and cost on cloud.

hongchaodeng · 2017-09-22T18:28:18Z

pkg/debug/debug_logger.go

@@ -75,6 +75,10 @@ func (dl *DebugLogger) LogPodDeletion(podName string) {
 	dl.fileLogger.Infof("deleted pod (%s)", podName)
 }

+func (dl *DebugLogger) LogPVCDeletion(pvcName string) {


Let's not add this logging.
The debugLogger was built for self-hosted hack..

hongchaodeng · 2017-09-22T19:52:20Z

pkg/apis/etcd/v1beta2/cluster.go

@@ -206,6 +207,11 @@ func (c *ClusterSpec) Validate() error {
 				return errors.New("spec: pod labels contains reserved label")
 			}
 		}
+		if c.Pod.PV != nil {
+			if c.Pod.PV.VolumeSizeInMB < minPVSize {


Having a default sounds a better option to prevent user mistakes.

Additionally, we should also write docs to recommend setting limitrange to require minimum storage:
https://kubernetes.io/docs/tasks/administer-cluster/limit-storage-consumption/#limitrange-to-limit-requests-for-storage

hongchaodeng · 2017-09-22T19:52:44Z

pkg/apis/etcd/v1beta2/cluster.go

@@ -27,6 +27,8 @@ import (
 const (
 	defaultBaseImage = "quay.io/coreos/etcd"
 	defaultVersion   = "3.1.8"
+
+	minPVSize = 512 // 512MiB


minPodPV...

hongchaodeng · 2017-09-22T19:54:36Z

pkg/cluster/cluster.go

@@ -404,7 +411,12 @@ func (c *Cluster) startSeedMember(recoverFromBackup bool) error {
 		SecureClient: c.isSecureClient(),
 	}
 	ms := etcdutil.NewMemberSet(m)
-	if err := c.createPod(ms, m, "new", recoverFromBackup); err != nil {
+	if c.UsePodPV() {


IsPodPVEnabled ?

hongchaodeng · 2017-09-22T19:59:18Z

pkg/cluster/cluster.go

+
+	// TODO: We set timeout to 60s here since PVC binding could take up to 60s for GCE/PD. See https://github.com/kubernetes/kubernetes/issues/40972 .
+	//       Change the wait time once there are official p99 SLA.
+	err = retryutil.Retry(4*time.Second, 15, func() (bool, error) {


Should not wait PVC ready here.
It should reconcile until PVC is ready before creating the pod.

hongchaodeng · 2017-09-22T20:00:20Z

pkg/util/k8sutil/k8sutil.go

+	return pvc
+}
+
+func NewEtcdPod(m *etcdutil.Member, initialCluster []string, clusterName, state, token string, cs api.ClusterSpec, usePVC bool, owner metav1.OwnerReference) *v1.Pod {


Don't add a new param.

Instead, add a new func AddPVCIntoPod() to compose the spec.

hongchaodeng · 2017-09-22T20:03:05Z

pkg/garbagecollection/gc.go

@@ -158,3 +161,28 @@ func (gc *GC) collectDeployment(option metav1.ListOptions, runningSet map[types.

 	return nil
 }
+
+func (gc *GC) collectPVCs(option metav1.ListOptions, runningSet map[types.UID]bool) error {


+1
We will rm -rf entire custom GC code :)

hongchaodeng · 2017-09-22T20:04:29Z

pkg/util/etcdutil/member.go

@@ -70,6 +70,10 @@ func (m *Member) PeerURL() string {
 	return fmt.Sprintf("%s://%s:2380", m.peerScheme(), m.Addr())
 }

+func (m *Member) PVCName() string {


This is not really etcd related method..
It should be in k8sutil.

hongchaodeng · 2017-09-22T20:07:48Z

test/e2e/basic_test.go

@@ -40,7 +40,7 @@ func TestCreateCluster(t *testing.T) {
 		}
 	}()

-	if _, err := e2eutil.WaitUntilSizeReached(t, f.CRClient, 3, 6, testEtcd); err != nil {
+	if _, err := e2eutil.WaitUntilSizeReached(t, f.CRClient, 3, 20, testEtcd); err != nil {


Don't change any timeout in test.
All test timeout is delicately tuned because we understand why we allocate this much time and have expectation how long each operation takes.

Let's create a separate set of test for etcd pv scenario.
Additionally, we will probably poll some PVs before test in the future. But we don't need to worry about this right now.

mrIncompetent · 2017-10-12T16:24:33Z

@sgotti Is there anything i can help with?
I would love to use this feature!

hongchaodeng · 2017-10-17T18:29:34Z

@mrIncompetent
Hasn't seen any update for a while.
Would you like to take it over? I'm fine to move forward if you can submit a PR.

sgotti · 2017-10-17T19:56:19Z

@hongchaodeng @mrIncompetent Sorry, I've ben busy with other stuff and lost your comments notifications. I'm fine if you want to take over or I'll be able to update it in the next days.

hongchaodeng · 2017-10-17T21:01:22Z

@sgotti
No worries.

@mrIncompetent
Just wait for the PR and help with review. Thanks!

this patch implements part 1 of the etcd data on persistent volumes design. when pod pvsource is defined in the spec it'll create a PVC for every etcd member and use it as the volume for etcd data. pvc without a member will be removed during the reconcile.

sgotti · 2017-10-27T16:47:33Z

@hongchaodeng Updated

hongchaodeng

Awesome work.
Looks very good!
Just some minor code issues.
I will give it a try later over the weekend too and provide more feedback.

hongchaodeng · 2017-10-27T19:39:46Z

test/e2e/e2eutil/spec_util.go

@@ -37,6 +37,15 @@ func NewCluster(genName string, size int) *api.EtcdCluster {
 	}
 }

+func AddPV(c *api.EtcdCluster, storageClass string) {


AddPVToCluster

hongchaodeng · 2017-10-27T19:42:16Z

pkg/cluster/cluster.go

@@ -415,6 +422,11 @@ func (c *Cluster) startSeedMember(recoverFromBackup bool) error {
 		SecureClient: c.isSecureClient(),
 	}
 	ms := etcdutil.NewMemberSet(m)
+	if c.IsPodPVEnabled() {
+		if err := c.createPVC(m); err != nil {


This is duplicated in two places. Can you combine it with createPod() into a new function? Maybe createResource?

hongchaodeng · 2017-10-27T19:44:41Z

pkg/cluster/cluster.go

+		if !k8sutil.IsKubernetesResourceNotFoundError(err) {
+			return err
+		}
+		if c.isDebugLoggerEnabled() {


Can you discard this path:

if c.isDebugLoggerEnabled() {...}

Just ignore isDebugLoggerEnabled now

hongchaodeng · 2017-10-27T19:45:04Z

pkg/cluster/cluster.go

+func (c *Cluster) removePVC(name string) error {
+	ns := c.cluster.Namespace
+	err := c.config.KubeCli.Core().PersistentVolumeClaims(ns).Delete(name, nil)
+	if err != nil {


if err !=nil && !NotFound(err){...

hongchaodeng · 2017-10-27T19:46:56Z

pkg/util/k8sutil/k8sutil.go

+				"etcd_node":    m.Name,
+				"etcd_cluster": clusterName,
+				"app":          "etcd",
+			},


Not sure what's the context.
Re. labels, use:

"etcd_cluster": clusterName, "app": "etcd",

is good enough

hongchaodeng · 2017-10-27T19:48:36Z

pkg/cluster/cluster.go

@@ -537,6 +575,29 @@ func (c *Cluster) pollPods() (running, pending []*v1.Pod, err error) {
 	return running, pending, nil
 }

+func (c *Cluster) pollPVCs() (pvcs []*v1.PersistentVolumeClaim, err error) {


hongchaodeng · 2017-10-27T19:48:53Z

pkg/cluster/reconcile.go

@@ -32,7 +32,7 @@ import (
 // reconcile reconciles cluster current state to desired state specified by spec.
 // - it tries to reconcile the cluster to desired size.
 // - if the cluster needs for upgrade, it tries to upgrade old member one by one.
-func (c *Cluster) reconcile(pods []*v1.Pod) error {
+func (c *Cluster) reconcile(pods []*v1.Pod, pvcs []*v1.PersistentVolumeClaim) error {


change doc?

hongchaodeng · 2017-10-27T19:49:01Z

pkg/util/k8sutil/k8sutil.go

@@ -209,6 +214,14 @@ func newEtcdServiceManifest(svcName, clusterName, clusterIP string, ports []v1.S
 	return svc
 }

+func AddEtcdVolumeToPod(pod *v1.Pod, m *etcdutil.Member, usePVC bool) {


hongchaodeng · 2017-10-27T19:49:15Z

pkg/util/k8sutil/k8sutil.go

@@ -228,6 +241,36 @@ func NewSeedMemberPod(clusterName string, ms etcdutil.MemberSet, m *etcdutil.Mem
 	return pod
 }

+func NewPVC(m *etcdutil.Member, cs api.ClusterSpec, clusterName, namespace string, owner metav1.OwnerReference) *v1.PersistentVolumeClaim {


hongchaodeng · 2017-10-27T19:49:21Z

test/e2e/etcd_on_pv_test.go

+	"github.com/coreos/etcd-operator/test/e2e/framework"
+)
+
+func TestCreateClusterWithPV(t *testing.T) {


swapnilgm · 2017-11-06T06:45:55Z

pkg/util/k8sutil/k8sutil.go

+			},
+		},
+		Spec: v1.PersistentVolumeClaimSpec{
+			StorageClassName: &cs.Pod.PV.StorageClass,


Since storageClassName is optional in cluster specification, unspecified storageClassName in cluster spec leads to storageClassName = "" in PVC specification. In case storageClassName is not specified in cluster specification, it should be removed from PVC to use default storageClass. Otherwise there won't be option for user to make use of default storage class. Please have look at this https://kubernetes.io/docs/concepts/storage/persistent-volumes/#class-1

I didn't realize empty means default.
If so, the correct thing to do is to change the storageclads field into a pointer. But This is not urgent here. We can fix It after This pr

hongchaodeng · 2017-11-23T15:24:26Z

Hi @sgotti
Do you have time to address it in next few days? If not, I am having holidays and can help finish the work.

sgotti · 2017-11-27T15:46:27Z

@hongchaodeng I don't have a lot of spare time in these days so feel free to finish it. Thanks.

hongchaodeng · 2018-01-18T19:01:48Z

Mind if I close this PR in favor of #1861?

hongchaodeng · 2018-01-20T21:07:32Z

Done in #1861

sgotti commented Sep 22, 2017

View reviewed changes

xiang90 reviewed Sep 22, 2017

View reviewed changes

hongchaodeng reviewed Sep 22, 2017

View reviewed changes

sgotti force-pushed the pv_etcd_data_part1 branch 3 times, most recently from 56019ef to 4d30f6c Compare October 27, 2017 15:28

sgotti force-pushed the pv_etcd_data_part1 branch from 4d30f6c to cc4ca65 Compare October 27, 2017 16:46

hongchaodeng reviewed Oct 27, 2017

View reviewed changes

swapnilgm reviewed Nov 6, 2017

View reviewed changes

hongchaodeng mentioned this pull request Nov 21, 2017

Persistent/Durable etcd cluster #1323

Open

hongchaodeng mentioned this pull request Nov 24, 2017

*: PersistentVolume for etcd data #1695

Closed

hongchaodeng closed this Jan 18, 2018

hongchaodeng reopened this Jan 18, 2018

hongchaodeng closed this Jan 20, 2018

*: implement PersistentVolume for etcd data design part 1 #1434

*: implement PersistentVolume for etcd data design part 1 #1434

Conversation

sgotti commented Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongchaodeng Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

sgotti Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrIncompetent commented Oct 12, 2017

hongchaodeng commented Oct 17, 2017

sgotti commented Oct 17, 2017

hongchaodeng commented Oct 17, 2017

sgotti commented Oct 27, 2017

hongchaodeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongchaodeng commented Nov 23, 2017

sgotti commented Nov 27, 2017

hongchaodeng commented Jan 18, 2018

hongchaodeng commented Jan 20, 2018

sgotti commented Sep 22, 2017 •

edited

Loading

hongchaodeng Oct 3, 2017 •

edited

Loading

sgotti Sep 22, 2017 •

edited

Loading