Skip to content

Configurable mixer cluster names for mixer filter #358

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

Merged
merged 2 commits into from
Feb 6, 2018
Merged

Conversation

rshriram
Copy link
Member

@rshriram rshriram commented Feb 5, 2018

The mixer cluster names is hardcoded to "mixer_server". This name is prone to collisions if user has one by the same name. Also, it is quite inconvenient to have a hardcoded cluster that one cannot control/customize.

This PR introduces configuration options for check cluster and report cluster. By default, they will continue to use the currently hardcoded "mixer_server" as the cluster names. Over time, we can expose global configs that will let users point to separate upstream clusters for mixer check and report calls (for load distribution/management/etc.).

@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Feb 5, 2018
@rshriram
Copy link
Member Author

rshriram commented Feb 5, 2018

fyi @elevran

@douglas-reid
Copy link
Contributor

In general, I don't have any problems with this change. It does impact some of the assumptions hardcoded in the mixer grafana dashboard. I'm not sure what the right approach is to making that more flexible is, but I'm not sure it should block this feature (if needed).

@rshriram
Copy link
Member Author

rshriram commented Feb 5, 2018

I think those dashboards won't break. We could document that if you change the default names of check/report cluster, it is the user's responsibility to update the out-of-box dashboards that Istio provides.

// checkCluster and reportCluster, it is possible to have one set of
// mixer servers handle check calls, while another set of mixer servers
// handle report calls.
string report_cluster = 7;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment about the impact of renaming these clusters on downstream things like the dashboards.

Please replicate the comments between check_cluster and report_cluster. That is, don't assume someone reads the comment of report_cluster when they're looking at the check_cluster field.

@mandarjog
Copy link
Contributor

This is great. I was going to add this for Mixer HA story anyway.

@rshriram rshriram merged commit ebec456 into master Feb 6, 2018
istio-merge-robot pushed a commit to istio/proxy that referenced this pull request Feb 6, 2018
Automatic merge from submit-queue.

make mixer cluster name configurable

Please see istio/api#358 for context.
Essentially remove hardcoded "mixer_server" and allow users to override the name, in addition to allowing separate clusters for check vs reports so that it makes it easier to load balance/manage the system.
@rshriram rshriram deleted the mixer_cluster branch February 13, 2018 19:41
incfly pushed a commit to incfly/api that referenced this pull request Jun 13, 2018
incfly pushed a commit to incfly/api that referenced this pull request Jun 13, 2018
Automatic merge from submit-queue.

[DO NOT MERGE] Auto PR to update dependencies of mixerclient

This PR will be merged automatically once checks are successful.
```release-note
none
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants