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

Allow additional service configuration for MysqlCluster #747

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ynnt
Copy link

@ynnt ynnt commented Oct 29, 2021

This pull request adds capability to set ServiceSpec for MysqlCluster.

  • I've made sure the Changelog.md will remain up-to-date after this PR is merged.

@ynnt ynnt force-pushed the master branch 3 times, most recently from fd1138c to adc3d74 Compare October 29, 2021 12:32
Copy link
Collaborator

@cndoit18 cndoit18 left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution。

I think it's a good idea, but we need to think hard about how to design the API

@cndoit18
Copy link
Collaborator

Or we can create some exporter services instead of making changes to the original services.

Copy link
Collaborator

@cndoit18 cndoit18 left a comment

Choose a reason for hiding this comment

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

LGTM
Thank you very much for your contribution, please squash your commits

@cndoit18
Copy link
Collaborator

cndoit18 commented Nov 2, 2021

WDYT, cc @calind

@calind
Copy link
Member

calind commented Nov 4, 2021

The MySQLCluster controller/resource should not be responsible for creating any additional services (as is the case of the k8s StatefulSet controller for example). The extra services should be created by the cluster operator.

The place where extra services are created might be within the mysql-cluster helm chart.

@ynnt
Copy link
Author

ynnt commented Nov 4, 2021

The MySQLCluster controller/resource should not be responsible for creating any additional services (as is the case of the k8s StatefulSet controller for example). The extra services should be created by the cluster operator.

The place where extra services are created might be within the mysql-cluster helm chart.

Thank you for your reply, but could you elaborate more on this?
This pull request does not add any additional services. Instead, it allows changing the type of existing ones.

@cndoit18
Copy link
Collaborator

cndoit18 commented Nov 5, 2021

The MySQLCluster controller/resource should not be responsible for creating any additional services (as is the case of the k8s StatefulSet controller for example). The extra services should be created by the cluster operator.
The place where extra services are created might be within the mysql-cluster helm chart.

Thank you for your reply, but could you elaborate more on this? This pull request does not add any additional services. Instead, it allows changing the type of existing ones.

You can go in the mysql-cluster helm chart and add two additional services as extensions, and with the extensibility of the chart, a new loadbalancer can be added without change the code

@cndoit18 cndoit18 self-requested a review November 5, 2021 11:36
@ynnt
Copy link
Author

ynnt commented Nov 5, 2021

You can go in the mysql-cluster helm chart and add two additional services as extensions, and with the extensibility of the chart, a new loadbalancer can be added without change the code

I see that. The only issue with this approach is that I am forced to use the Helm chart for creating a cluster. Services will not be created when a cluster is created using a plain yaml file or kustomize.

@cndoit18
Copy link
Collaborator

cndoit18 commented Nov 5, 2021

You can go in the mysql-cluster helm chart and add two additional services as extensions, and with the extensibility of the chart, a new loadbalancer can be added without change the code

I see that. The only issue with this approach is that I am forced to use the Helm chart for creating a cluster. Services will not be created when a cluster is created using a plain yaml file or kustomize.

right

@ynnt
Copy link
Author

ynnt commented Nov 5, 2021

right

That's the point of my PR. ;)

@cndoit18
Copy link
Collaborator

You can go in the mysql-cluster helm chart and add two additional services as extensions, and with the extensibility of the chart, a new loadbalancer can be added without change the code

I see that. The only issue with this approach is that I am forced to use the Helm chart for creating a cluster. Services will not be created when a cluster is created using a plain yaml file or kustomize.

It's probably about design, we need to be careful about extending mysqlcluster in order to keep it easy to use, maybe we can start with helm support? WDYT @ynnt

@ynnt
Copy link
Author

ynnt commented Nov 15, 2021

I do agree that this PR changes API layout a little bit (though, I do not think it changes UX or introduces any significant changes to the overall application design).

Unfortunately, the Helm chart approach simply does not work for my use case (creating mysql clusters programmatically). :(

So, feel free to close the PR if you believe it adds unnecessary complexity, I will just stick to my fork.

@cndoit18
Copy link
Collaborator

WDYT @calind

@mglants
Copy link

mglants commented Dec 30, 2021

@cndoit18 @calind Any plans to merge it? We really need this feature

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.

4 participants