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.

Sign up for GitHub

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

Jump to bottom

Allow setting resources and allowMultipleNodesPerWorker in the values #160

Merged
merged 15 commits into from
Jan 10, 2021
Merged

Allow setting resources and allowMultipleNodesPerWorker in the values #160

merged 15 commits into from
Jan 10, 2021

Conversation

florissmit10
Copy link
Contributor

Setting allowMultipleNodesPerWorker to true is only allowed when resources are set.

@jsanda
Copy link
Contributor

jsanda commented Jan 7, 2021

Hi @florissmit10

Thank you for submitting the PR! Before we can accept any contributions, we need you to sign the CLA at https://cla.datastax.com/.

Thanks

John

@jsanda
Copy link
Contributor

jsanda commented Jan 7, 2021

Fixes #156

jsanda
jsanda requested changes Jan 7, 2021
View reviewed changes
Copy link
Contributor

@jsanda jsanda left a comment

Choose a reason for hiding this comment

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

Can you also please add/update unit tests?

charts/k8ssandra-cluster/templates/cassdc.yaml Outdated
@@ -28,6 +28,11 @@ spec:
resources:
requests:
storage: {{ .Values.k8ssandra.cassandraLibDirVolume.size | default "5Gi" }}
allowMultipleNodesPerWorker: {{ .Values.allowMultipleNodesPerWorker | default false}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move these properties under the top-level k8ssandra property?

charts/k8ssandra-cluster/values.yaml Outdated
@@ -6,6 +6,8 @@ name: k8ssandra
clusterName: k8ssandra
datacenterName: dc1
size: 1
allowMultipleNodesPerWorker: false
Copy link
Contributor

Choose a reason for hiding this comment

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

If we have allowMultipleNodesPerWorker: true, should there be a validation check for resources being defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this time the validation of the CassandraDatacenter crd gives a pretty clear error message when you fail to set the resources:
Error: admission webhook "cassandradatacenter-webhook.cassandra.datastax.com" denied the request: CassandraDatacenter write rejected, attempted to use multiple nodes per worker without cpu and memory requests and limits

I could add some conditional statements but it would make the template bigger, and the error message less clear I think. Maybe a comment in the default values.yaml?

Copy link
Contributor

Choose a reason for hiding this comment

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

The error message from the webhook is great. The problem is that the chart install still happens and other objects still get created. The user would have to helm uninstall before trying the install again. We could use the required template function to make sure resources is defined. It would fail fast before anything gets deployed.

Copy link
Contributor 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!

jsanda
jsanda requested changes Jan 8, 2021
View reviewed changes
charts/k8ssandra-cluster/templates/cassdc.yaml Outdated
{{- if .Values.k8ssandra.allowMultipleNodesPerWorker}}
resources:
limits:
{{ toYaml (required "set resource limits/requets when enabling allowMultipleNodesPerWorker" .Values.k8ssandra.resources.limits) | indent 6}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Mispelling at "requets"

tests/unit/template_cassdc_test.go
@@ -130,5 +133,42 @@ var _ = Describe("Verify CassandraDatacenter template", func() {
// Two containers, medusa and cassandra
Expect(len(cassdc.Spec.PodTemplateSpec.Spec.Containers)).To(Equal(2))
})

It("setting allowMultipleNodesPerWorker to true", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add two more unit tests - one where allowMultipleNodesPerWorker is true and resources are not specified and one where allowMultipleNodesPerWorker is false and resources is set?

tests/unit/template_cassdc_test.go

renderTemplate(options)

Expect(*cassdc.Spec.Resources.Limits.Memory()).To(Equal(resource.MustParse("2Gi")))
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

jsanda
jsanda requested changes Jan 8, 2021
View reviewed changes
Copy link
Contributor

@jsanda jsanda left a comment

Choose a reason for hiding this comment

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

There is one other thing needed for the PR. Can you please bump the chart versions to 0.20.0 in each of the 4 charts?

tests/unit/template_cassdc_test.go
It("setting allowMultipleNodesPerWorker to true", func() {
options := &helm.Options{
SetValues: map[string]string{
"k8ssandra.allowMultipleNodesPerWorker": "true",
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 is a duplicate test of the one at line 184, minus the resource verification? If so, can you remove it?

jsanda
jsanda requested changes Jan 8, 2021
View reviewed changes
tests/unit/template_cassdc_test.go Outdated
@@ -33,13 +34,23 @@ var _ = Describe("Verify CassandraDatacenter template", func() {
err = nil
})

renderTemplate := func(options *helm.Options) {
renderedOutput := helm.RenderTemplate(
renderTemplateE := func(options *helm.Options) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a similar change in #178 :)

tests/unit/template_cassdc_test.go Outdated
renderTemplate := func(options *helm.Options) {
renderedOutput := helm.RenderTemplate(
renderTemplateE := func(options *helm.Options) error {
renderedOutput, renderError := helm.RenderTemplateE(
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be simplified a bit as,

		renderedOutput, err := helm.RenderTemplateE(
			GinkgoT(), options, helmChartPath, helmReleaseName,
			[]string{"templates/cassdc.yaml"},
		)

		if err == nil {
			err = helm.UnmarshalK8SYamlE(GinkgoT(), renderedOutput, cassdc)
		}

		return err

Because we using functions that return an error instead of panic, we need to update all the renderTemplate that should succeed to calls like this:

Expect(renderTemplate(options)).To(Succeed())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah that looks good!
First time programming Go for me, so these patterns are a bit new :P

jsanda
jsanda requested changes Jan 9, 2021
View reviewed changes
Copy link
Contributor

@jsanda jsanda left a comment

Choose a reason for hiding this comment

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

Some other PRs have been merged before yours, so you need to bump the chart versions to 0.22.0. After that I think we are ready to merge :)

jsanda
jsanda approved these changes Jan 10, 2021
View reviewed changes
Copy link
Contributor

@jsanda jsanda left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@jsanda jsanda merged commit 22944a2 into k8ssandra:main Jan 10, 2021
jdonenine added a commit that referenced this pull request Jan 11, 2021
@jdonenine
Merge branch 'main' of github.com:k8ssandra/k8ssandra into feature/va…
7a6962b 
…grant-dev-setup

* 'main' of github.com:k8ssandra/k8ssandra:
  Allow setting resources and allowMultipleNodesPerWorker in the values (#160)
  #174 Adding a small blurb to the docs noting the metric retention period for our prometheus deployment (#180)
  add support for setting C* version (#178)
@jeffbanks jeffbanks mentioned this pull request Jan 11, 2021
Merged
@jdonenine jdonenine added this to the 1.0.0 milestone Jan 22, 2021
@jdonenine jdonenine added this to In progress in K8ssandra via automation Jan 22, 2021
@jdonenine jdonenine moved this from In progress to Done in K8ssandra Jan 22, 2021
@jsanda jsanda mentioned this pull request Jan 27, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsanda jsanda jsanda approved these changes

Assignees
No one assigned
Labels
complexity:low
Projects
No open projects
K8ssandra
  
Done
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@florissmit10 @jsanda @jdonenine