Etcd v3 Support #3176

gambol99 · 2017-08-09T23:44:17Z

Etcd V3 Support

The current implementation is running v2.2.1 which is two years old and end of life. This PR adds the ability to use etcd v3 and set the versions if required. Note at the moment the image is still using the gcr.io registry image and much like Etcd TLS PR there presently is no 'automated' migration path from v2 to v3.

the feature is gated behind the version of the etcd cluster, both clusters events and main must use the same storage type
the version for v2 is unchanged and pinned at v2.2.1 with v3 using v3.0.17
@question: we should consider allowing the user to override the images though I think this should be addressed generically, than one offs here and then. I know @chrislovecnm is working on a asset registry??

k8s-ci-robot · 2017-08-09T23:44:25Z

Hi @gambol99. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

gambol99 · 2017-08-09T23:52:30Z

/assign @justinsb

justinsb · 2017-08-11T08:25:16Z

nodeup/pkg/model/protokube.go

@@ -181,6 +181,7 @@ type ProtokubeFlags struct {
 	DNSInternalSuffix *string  `json:"dnsInternalSuffix,omitempty" flag:"dns-internal-suffix"`
 	DNSProvider       *string  `json:"dnsProvider,omitempty" flag:"dns"`
 	DNSServer         *string  `json:"dns-server,omitempty" flag:"dns-server"`
+	Image             *string  `json:"image,omitempty" flag:"image"`


I think this is the etcd image, so how about EtcdImage *string json:"image,omitempty" flag:"etcd-image"`. It's the flag that is hard to change and matters most.

makes sense and fixed ... image is too generic as well

justinsb · 2017-08-11T08:26:00Z

nodeup/pkg/model/protokube.go

@@ -195,9 +196,14 @@ type ProtokubeFlags struct {

 // ProtokubeFlags is responsible for building the command line flags for protokube
 func (t *ProtokubeBuilder) ProtokubeFlags(k8sVersion semver.Version) *ProtokubeFlags {
+	// @todo: i think we should allow the user to override the source of the image, but for now
+	// lets keep that for another PR and allow the version change


Agreed! @chrislovecnm has some PRs that are working towards this.

justinsb · 2017-08-11T08:31:56Z

pkg/apis/kops/cluster.go

+	// EnableEtcdTLS indicates the etcd service should use TLS between peers and clients
+	EnableEtcdTLS bool `json:"enableEtcdTLS,omitempty"`
+	// StorageType indicates the storage type of the cluster v2 or v3
+	StorageType EtcdStorageType `json:"storageType,omitempty"`


I'm not sure we need to / should specify storageType separately from the etcd version. I think this ends up controlling the apiserver storage-type flag (there is a separate storage-version for etcd, as well as a serialization formation for apiserver, but I think those were measures to provide rollback functionality which aren't widely used - a backup is the way to go).

admittedly this was copied from a previous PR by yourself ... I think your right though, there's no real need to specify storage engine outside of the version .. I suppose if or when kubernetes supports different storage backend's it can be amended

justinsb · 2017-08-11T08:39:30Z

pkg/model/components/context.go

+// @question: I kinda feel like this functionality should be on the kops.CluserSpec itself, though I don't
+// know how to do this without duplicating methods and how it effects the versions v1, v2 etc
+func UseEtcdV3(spec *kops.ClusterSpec) bool {
+	// validation ensure the etcd must be both one or the other


Personally I would probably parse the version, and check if the prefix was v2. / 2. or v3. / 3.. I suppose there is also the chance that when we mirror the image we will put a prefix on it (e.g. gcr.io/myimages/etcd_v3.0.17) but I think we don't do that yet.

Specifying the storage-type separately is sort of appealing here, except that it isn't really the storage-type we're specifying, and as you say we would rather this was on the cluster/apiserver. The flag is mapped on the apiserver (you set StorageBackend), so we could let the user set it there if they really want to.

I suspect we keep it simple for now, don't add storage-type, parse the version to infer the etcd version using some simple heuristics, and see what is needed from there.

And that implies that we replace validateEtcdStorage with a similar loop, but checking that the inferred version of etcd is the same (which also lets us alert the user if they specify a version we don't recognize)

justinsb · 2017-08-11T08:44:04Z

pkg/model/components/etcd.go

+		if x.Version == "" {
+			switch UseEtcdV3(spec) {
+			case true:
+				x.Version = "3.0.17"


We'll want this logic, except I think we'll want to default new clusters to etcd3 in some future kubernetes version.

justinsb · 2017-08-11T08:44:40Z

protokube/cmd/protokube/main.go

@@ -82,7 +82,7 @@ func run() error {
 	flag.StringVar(&tlsKey, "tls-key", tlsKey, "Path to a file containing the private key for etcd server")
 	flags.StringSliceVarP(&zones, "zone", "z", []string{}, "Configure permitted zones and their mappings")
 	flags.StringVar(&dnsProviderID, "dns", "aws-route53", "DNS provider we should use (aws-route53, google-clouddns, coredns)")
-	flags.StringVar(&etcdImageSource, "etcd-image-source", etcdImageSource, "Etcd Source Container Registry")
+	flags.StringVar(&etcdImageSource, "image", "gcr.io/google_containers/etcd:2.2.1", "Etcd Source Container Registry")


I suggest etcd-image, but 👍

justinsb · 2017-08-11T08:46:04Z

upup/pkg/fi/cloudup/populate_cluster_spec.go

@@ -289,6 +289,7 @@ func (c *populateClusterSpec) run() error {
 		switch m {
 		case "config":
 			// Note: DefaultOptionsBuilder comes first
+			codeModels = append(codeModels, &components.EtcdOptionsBuilder{Context: optionsContext})


Let's put this after DefaultOptionsBuilder, just so we don't have to change the comment :-)

justinsb · 2017-08-11T08:53:13Z

Generally looks great. I did have some minor comments, but the big one is about whether we expose storage-type on the EtcdCluster. I think we could infer it from the version. My inclination is to do that for now, and then we can add more options later. It's tricky though because there are 3 concepts that are all intertwined - the storage-engine that etcd uses, the protocol that the apiserver uses to talk to etcd, and the serialization format that the apiserver uses to serialize objects when writing them to etcd3. I suspect we want to just move all 3 at once now that they've mostly been proven out - the benefits of not being an early etcd3 adopter!

justinsb · 2017-08-11T08:53:18Z

/ok-to-test

justinsb · 2017-08-11T09:57:03Z

But oh the hypocrisy :-) I checked #2169 where I had experimented with this previously, and I had also split storage-type from version. I think I'm right now, but given we both split them, maybe you and past-me are right...

gambol99 · 2017-08-11T11:26:52Z

i think your present self is right :-) .. the requirement for a storage type really isn't there and we allow the user to set the storage type in the api-server anyhow

gambol99 · 2017-08-11T11:31:50Z

hi @justinsb ... i've made the following changes and rebased

Requested Changes - Etvd v3

removing the StorageType on the etcd cluster spec (sticking with the Version field only)
changed the protokube flag back to -etcd-image
users have to explicitly set the etcd version now; the latest version in gcr.io is 3.0.17
reverted the ordering on the populate spec

gambol99 · 2017-08-11T11:35:30Z

$ kubectl get no 
NAME                                          STATUS         AGE       VERSION
ip-10-250-34-29.eu-west-2.compute.internal    Ready,master   22m       v1.7.0
ip-10-250-34-59.eu-west-2.compute.internal    Ready,master   22m       v1.7.0
ip-10-250-34-6.eu-west-2.compute.internal     Ready,master   22m       v1.7.0
ip-10-250-35-10.eu-west-2.compute.internal    Ready,node     20m       v1.7.0
ip-10-250-35-27.eu-west-2.compute.internal    Ready,node     20m       v1.7.0
ip-10-250-36-178.eu-west-2.compute.internal   Ready,node     20m       v1.7.0
ip-10-250-36-251.eu-west-2.compute.internal   Ready,node     20m       v1.7.0
ip-10-250-36-91.eu-west-2.compute.internal    Ready,node     20m       v1.7.0
$ kubectl --namespace=kube-system get pod | grep etcd
etcd-server-events-ip-10-250-34-29.eu-west-2.compute.internal        1/1       Running   0          21m
etcd-server-events-ip-10-250-34-59.eu-west-2.compute.internal        1/1       Running   0          21m
etcd-server-events-ip-10-250-34-6.eu-west-2.compute.internal         1/1       Running   0          22m
etcd-server-ip-10-250-34-29.eu-west-2.compute.internal               1/1       Running   0          21m
etcd-server-ip-10-250-34-59.eu-west-2.compute.internal               1/1       Running   0          22m
etcd-server-ip-10-250-34-6.eu-west-2.compute.internal                1/1       Running   0          22m
$ kubectl --namespace=kube-system logs etcd-server-ip-10-250-34-29.eu-west-2.compute.internal | grep version
2017-08-11 10:55:56.800257 I | etcdserver: starting server... [version: 3.0.17, cluster version: to_be_decided]
2017-08-11 10:56:07.293765 N | membership: set the initial cluster version to 2.3
2017-08-11 10:56:07.293817 I | api: enabled capabilities for version 2.3
2017-08-11 10:56:15.438148 N | membership: updated the cluster version from 2.3 to 3.0
2017-08-11 10:56:15.438307 I | api: enabled capabilities for version 3.0

nice :-) ... i also verified v2 was unaffected ...

@question

The current implementation is running v2.2.1 which is two year old and end of life. This PR add the ability to use etcd and set the versions if required. Note at the moment the image is still using the gcr.io registry image. As note, much like TLS their presently is not 'automated' migration path from v2 to v3. - the feature is gated behine the storageType of the etcd cluster, bot clusters events and main must use the same storage type - the version for v2 is unchanged and pinned at v2.2.1 with v2 using v3.0.17 - @question: we shoudl consider allowing the use to override the images though I think this should be addresses more generically, than one offs here and then. I know chris is working on a asset registry??

- removing the StorageType on the etcd cluster spec (sticking with the Version field only) - changed the protokube flag back to -etcd-image - users have to explicitly set the etcd version now; the latest version in gcr.io is 3.0.17 - reverted the ordering on the populate spec

justinsb · 2017-08-12T14:10:20Z

/lgtm

Thanks so much @gambol99

k8s-github-robot · 2017-08-12T14:10:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gambol99, justinsb

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [justinsb]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-08-12T14:36:39Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-08-12T15:07:48Z

Automatic merge from submit-queue

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 9, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 9, 2017

gambol99 changed the title ~~Etcv3 Support~~ Etcd v3 Support Aug 9, 2017

k8s-ci-robot assigned justinsb Aug 9, 2017

justinsb reviewed Aug 11, 2017

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 11, 2017

justinsb mentioned this pull request Aug 11, 2017

Rework legacy validation to use field error helpers #3148

Merged

gambol99 force-pushed the etcv3 branch from 051d78a to 22bdf5d Compare August 11, 2017 11:29

gambol99 added 2 commits August 11, 2017 21:04

gambol99 force-pushed the etcv3 branch from 22bdf5d to 7cd2142 Compare August 11, 2017 20:08

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 12, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 12, 2017

k8s-github-robot merged commit 7942869 into kubernetes:master Aug 12, 2017

gambol99 deleted the etcv3 branch August 25, 2017 19:41

chrislovecnm mentioned this pull request Sep 29, 2017

support etcd3 #2842

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Etcd v3 Support #3176

Etcd v3 Support #3176

gambol99 commented Aug 9, 2017 •

edited

Loading

k8s-ci-robot commented Aug 9, 2017

gambol99 commented Aug 9, 2017

justinsb Aug 11, 2017

gambol99 Aug 11, 2017

justinsb Aug 11, 2017

justinsb Aug 11, 2017

gambol99 Aug 11, 2017

justinsb Aug 11, 2017

justinsb Aug 11, 2017

justinsb Aug 11, 2017

justinsb Aug 11, 2017

justinsb commented Aug 11, 2017

justinsb commented Aug 11, 2017

justinsb commented Aug 11, 2017

gambol99 commented Aug 11, 2017 •

edited

Loading

gambol99 commented Aug 11, 2017

gambol99 commented Aug 11, 2017

justinsb commented Aug 12, 2017

k8s-github-robot commented Aug 12, 2017

k8s-github-robot commented Aug 12, 2017

k8s-github-robot commented Aug 12, 2017

Etcd v3 Support #3176

Etcd v3 Support #3176

Conversation

gambol99 commented Aug 9, 2017 • edited Loading

k8s-ci-robot commented Aug 9, 2017

gambol99 commented Aug 9, 2017

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

justinsb commented Aug 11, 2017

justinsb commented Aug 11, 2017

justinsb commented Aug 11, 2017

gambol99 commented Aug 11, 2017 • edited Loading

gambol99 commented Aug 11, 2017

gambol99 commented Aug 11, 2017

justinsb commented Aug 12, 2017

k8s-github-robot commented Aug 12, 2017

k8s-github-robot commented Aug 12, 2017

k8s-github-robot commented Aug 12, 2017

gambol99 commented Aug 9, 2017 •

edited

Loading

gambol99 commented Aug 11, 2017 •

edited

Loading