Skip to content
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

[api] cluster spec etcd endpts; per-cluster envs #99

Merged
merged 4 commits into from
Mar 12, 2019

Conversation

schallert
Copy link
Collaborator

@schallert schallert commented Mar 6, 2019

This PR has a few changes to improve the interaction between
operator-operated M3DB clusters and etcd:

  1. Users can now specify etcd endpoints on the m3dbcluster spec,
    removing one of the most common changes users would have to make in
    their config files.

  2. The operator will include a per-cluster environment header in all
    requests to the coordinator, meaning that namespaces and placements will
    be stored under separate keys per-cluster in etcd and multiple
    m3dbclusters can share the same etcd cluster.

NOTE: This PR introduces a breaking change that will require users
to 1) rename 2 keys in their etcd clusters and 2) change a value in
their configmap if they would like the operator to continue being able
to manage clusters from before this commit. The changelog for this
release will include instructions for this.

Fixes #64
Fixes #88

@schallert schallert marked this pull request as ready for review March 6, 2019 19:53
Copy link
Contributor

@nerd0 nerd0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the direction of the PR, had a couple of questions.

cmd/m3db-operator/main.go Show resolved Hide resolved
docs/api.md Outdated
@@ -43,7 +43,8 @@ ClusterSpec defines the desired state for a M3 cluster to be converge to.
| numberOfShards | NumberOfShards defines how many shards in total | int32 | false |
| isolationGroups | IsolationGroups specifies a map of key-value pairs. Defines which isolation groups to deploy persistent volumes for data nodes | [][IsolationGroup](#isolationgroup) | false |
| namespaces | Namespaces specifies the namespaces this cluster will hold. | [][Namespace](#namespace) | false |
| configMapName | ConfigMapName specifies the ConfigMap to use for this cluster. If unset a sane default will be used. | *string | false |
| etcdEndpoints | EtcdEndpoints defines the etcd endpoints to use for service discovery. Must be set if no custom configmap is defined. If set, etcd endpoints will be templated in to the default configmap template. | []string | false |
| configMapName | ConfigMapName specifies the ConfigMap to use for this cluster. If unset a default configmap with template variables for etcd endpoints will be used. | *string | false |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we link the default template that will be used as default-config above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah good point. I think I'll add a comment saying "see configuration section of docs" just so we don't have too many links in the comments

m.logger,
} {
assert.NotNil(t, v)
}
}

func newM3DBCluster(name string) *myspec.M3DBCluster {
func newM3DBCluster(ns, name string) *myspec.M3DBCluster {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this assuming a 1:1 association of namespace to cluster?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, but it's just generating a fake metadata for the tests

pkg/controller/m3admin_client.go Show resolved Hide resolved
@schallert schallert force-pushed the schallert/m3admin_client_env branch from 89e05e8 to 73a07df Compare March 11, 2019 02:21
This PR has a few changes to improve the interaction between
operator-operated M3DB clusters and etcd:

1. Users can now specify etcd endpoints on the m3dbcluster spec,
removing one of the most common changes users would have to make in
their config files.

2. The operator will include a per-cluster environment header in all
requests to the coordinator, meaning that namespaces and placements will
be stored under separate keys per-cluster in etcd and multiple
m3dbclusters can share the same etcd cluster.

**NOTE**: This PR introduces a breaking change that will require users
to 1) rename 2 keys in their etcd clusters and 2) change a value in
their configmap if they would like the operator to continue being able
to manage clusters from before this commit. The changelog for this
release will include instructions for this.
@schallert schallert force-pushed the schallert/m3admin_client_env branch from 73a07df to 89c86fc Compare March 12, 2019 20:53
Copy link
Contributor

@nerd0 nerd0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

pkg/controller/m3admin_client.go Show resolved Hide resolved
@schallert schallert merged commit 684333d into master Mar 12, 2019
@schallert schallert deleted the schallert/m3admin_client_env branch March 12, 2019 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants