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

Scaling #264

Closed
wants to merge 18 commits into from
Closed

Scaling #264

wants to merge 18 commits into from

Conversation

naredula-jana
Copy link

@naredula-jana naredula-jana commented Nov 23, 2018

Added scaling feature. scaling means vertical scaling of data nodes. details of scaling feature are available here: https://github.com/naredula-jana/elasticsearch-operator/blob/scaling/docs/scaling.md .

Copy link
Member

@stevesloka stevesloka left a comment

Choose a reason for hiding this comment

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

Hey thanks for this PR! My biggest question is why we have a statefulset per data node? Also there are places with hard coded ip's, not sure why those are now requirements. We should have a system that's dynamic and adapts to change. Also, there are various code comments, new lines, etc all throughout, not sure if this is a final PR or still WIP. Thanks!

cmd/operator/main.go Outdated Show resolved Hide resolved
docs/scaling.md Outdated Show resolved Hide resolved
docs/scaling.md Outdated Show resolved Hide resolved
pkg/k8sutil/deployments.go Outdated Show resolved Hide resolved
pkg/k8sutil/deployments.go Outdated Show resolved Hide resolved
}
return ret;
}
func es_checkForGreen(es_ip string)(error){ /* TODO */
Copy link
Member

Choose a reason for hiding this comment

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

My original thoughts around the ES status was to make a set of stats that are broken out. Doing this allows us to use the same ES status checks and also enable other features like rolling restarts.

Copy link
Author

Choose a reason for hiding this comment

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

Can you pls provide more details on this, Do you expect any change here.

@stevesloka
Copy link
Member

@naredula-jana you can change how statefulsets roll out changes which allow the operator to manage the statefulset in the manner it sees fit: https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#on-delete

@naredula-jana
Copy link
Author

naredula-jana commented Nov 30, 2018

@naredula-jana you can change how statefulsets roll out changes which allow the operator to manage the statefulset in the manner it sees fit: https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#on-delete

Changed to single statefulset using on-delete, and the code is committed.

@stevesloka
Copy link
Member

Thanks! Let me test this out @naredula-jana.

The only other thing that would be great to have is tests for all this code.

@naredula-jana
Copy link
Author

Thanks! Let me test this out @naredula-jana.

The only other thing that would be great to have is tests for all this code.
Regarding Testing, I was not able to test on a local storage, able to test scaling feature successfully on block network storage on azure with latest committed code.

without scaling patch: does not support/not tested on local storage.
With scaling patch + multiple statefullset : supports local storage/nfs, scaling was sucessfull tested with local and network storage.
With scaling patch + single statefullset (latest code) : does not support local storage, this is same as without scaling patch. I have placed TODO in k8util.go(line-449), the reason is all data nodes as same mount point, In the multiple statefullset each datanode as separate mount point so each dfatanodes stores in a multiple sub directories in local/nfs.

Is there any way to make local/nfs storage to work with the latest code or single statefullset?.

@stevesloka
Copy link
Member

Hey @naredula-jana, I've been tied up in Kubecon last week. Could you look to add tests to this PR? I don't see any included.

@naredula-jana
Copy link
Author

Hey @naredula-jana, I've been tied up in Kubecon last week. Could you look to add tests to this PR? I don't see any included.

I have added Test cases in the latest commit.

@stevesloka
Copy link
Member

@naredula-jana what I'm looking for are unit tests, things that run on each build so we can verify that the code works as expected. Manual tests are ok, but they are difficult to run and are time-consuming.

@naredula-jana
Copy link
Author

@naredula-jana what I'm looking for are unit tests, things that run on each build so we can verify that the code works as expected. Manual tests are ok, but they are difficult to run and are time-consuming.

I have added detail steps for each testcase. Automising the testcases to run during build will be difficult because in all testcases, the test case need to communicate with k8, operator and elastic cluster to verify the state of test if it is need to automise, communicating and verfying the test need lot code if it need to build from scratch.

@naredula-jana
Copy link
Author

@naredula-jana what I'm looking for are unit tests, things that run on each build so we can verify that the code works as expected. Manual tests are ok, but they are difficult to run and are time-consuming.

I have added detail steps for each testcase. Automising the testcases to run during build will be difficult because in all testcases, the test case need to communicate with k8, operator and elastic cluster to verify the state of test if it is need to automise, communicating and verfying the test need lot code if it need to build from scratch.

Is there any sample/example unit test that runs at build time and communicate with k8,ES operator and ES cluster ?. In the above testcases the test script need to communicate with 3 components: a) K8, b) ES operator c) ES cluster.

@stevesloka
Copy link
Member

@naredula-jana I think you are referencing integration tests which would run on a live cluster. I'm just asking for unit tests which test out the functionality of how each method functions. e.g. if you pass nil to it, what happens? If you pass the wrong value (or right value), does the code do what it's supposed to.

I've been very lax with unit tests, but that shouldn't be the case. We need to have good coverage to understand when a change breaks the logic of something somewhere.

@naredula-jana
Copy link
Author

@naredula-jana I think you are referencing integration tests which would run on a live cluster. I'm just asking for unit tests which test out the functionality of how each method functions. e.g. if you pass nil to it, what happens? If you pass the wrong value (or right value), does the code do what it's supposed to.

I've been very lax with unit tests, but that shouldn't be the case. We need to have good coverage to understand when a change breaks the logic of something somewhere.

I am preparing unit tests, I will commit tests as soon as possible. Thanks Jana

@naredula-jana
Copy link
Author

@naredula-jana I think you are referencing integration tests which would run on a live cluster. I'm just asking for unit tests which test out the functionality of how each method functions. e.g. if you pass nil to it, what happens? If you pass the wrong value (or right value), does the code do what it's supposed to.
I've been very lax with unit tests, but that shouldn't be the case. We need to have good coverage to understand when a change breaks the logic of something somewhere.

I am preparing unit tests, I will commit tests as soon as possible. Thanks Jana

unit test code added in the latest commit.

@stevesloka
Copy link
Member

stevesloka commented Jan 13, 2019

Looks like there needs a rebase @naredula-jana, some files are out of sync.

@naredula-jana
Copy link
Author

Looks like there needs a rebase @naredula-jana, some files are out of sync.

Scaling feature needed an extra argument to this function "p.k8sclient.CreateDataNodeDeployment", this need code merge , due to this there are some conflicts.

@naredula-jana
Copy link
Author

Looks like there needs a rebase @naredula-jana, some files are out of sync.

Scaling feature needed an extra argument to this function "p.k8sclient.CreateDataNodeDeployment", this need code merge , due to this there are some conflicts.

Do you want me to pull the latest master code and apply the "scaling" patch to it?.

@naredula-jana
Copy link
Author

Looks like there needs a rebase @naredula-jana, some files are out of sync.

Created New PR (scaling-new #277 ) rebased with latest master.

@naredula-jana naredula-jana mentioned this pull request Jan 14, 2019
@naredula-jana
Copy link
Author

Looks like there needs a rebase @naredula-jana, some files are out of sync.

Created New PR (scaling-new #277 ) rebased with latest master.

@stevesloka Any update to this PR or scaling-new #277 ?. Waiting for the code merging.

@stevesloka
Copy link
Member

Are the 2 PR's the same? Can you please close one or the other? Then we can work on getting the chosen one working.

@naredula-jana
Copy link
Author

Are the 2 PR's the same? Can you please close one or the other? Then we can work on getting the chosen one working.

Yes, Both are same. Pls pick any one of the PR for merging, remaining one can be closed. The PR scaling-new #277 is the rebased on the new master. but PR scaling-new #277 getting cycling dependency in one of the unit test that is not related to the scaling patch.

@naredula-jana
Copy link
Author

Scaling path is rebased to the latest master at PR scaling-new #277 . So closing this PR.

@naredula-jana
Copy link
Author

naredula-jana commented Jan 28, 2019 via email

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