Skip to content

Conversation

gaurav-splunk
Copy link
Collaborator

This PR targets the following changes -

Created a new CRD of type ClusterMaster. So now, to deploy an indexer cluster you need to create 2 CRs, one with type as ClusterMaster for CM and one for indexer cluster part with type as IndexerCluster.
With the introduction of new CRD for ClusterMaster, we don't need the old way of using replicas:0 to identify that this is for CM.
We have deprecated indexerClusterRef in favor of ClusterMasterRef. This is done to explicitly mention that any other resource which wants to connect to an indexer cluster will have to connect to Cluster Master part of the indexer cluster.
Moved all the SmartStore related logic to ClusterMaster code since SmartStore can only be configured through CM.
Made changes to UT and System tests accordingly.

Copy link
Collaborator

@akondur akondur left a comment

Choose a reason for hiding this comment

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

Looks good.

@romain-bellanger
Copy link
Contributor

Hi @gaurav-splunk,
I've reviewed the PR which looks great to me. I wanted to test it, I fetched the PR, built the image and deployed the operator, but I forgot to request our cluster admin to create the new CRD... This should be done tomorrow.
Just one comment: the definitions of the CRDs in deploy/crds/ have been reformatted. This is not really an issue, but it was easier for me to read the description of the properties split over multiple lines, than in a single very long line. I just wanted to share this feedback but it should not block merge.

@gaurav-splunk
Copy link
Collaborator Author

Hi @gaurav-splunk,
I've reviewed the PR which looks great to me. I wanted to test it, I fetched the PR, built the image and deployed the operator, but I forgot to request our cluster admin to create the new CRD... This should be done tomorrow.
Just one comment: the definitions of the CRDs in deploy/crds/ have been reformatted. This is not really an issue, but it was easier for me to read the description of the properties split over multiple lines, than in a single very long line. I just wanted to share this feedback but it should not block merge.

Hi @romain-bellanger Thanks for the feedback. This is an issue with the yq version I am using. I will update the PR with correct yq version installed on my machine. That should solve the indentation problem.

@gaurav-splunk gaurav-splunk merged commit aa298a0 into develop Sep 23, 2020
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.

7 participants