Override memory limits of rook operator to 512Mi #938

surajssd · 2020-09-10T10:37:56Z

Add tests for rook-ceph

Fixes: #932

johananl · 2020-09-10T11:54:44Z

make test is failing for me:

--- FAIL: TestConversion (0.16s)
    --- FAIL: TestConversion/EnvVars (0.00s)
        --- FAIL: TestConversion/EnvVars/CSI_PROVISIONER_NODE_AFFINITY (0.00s)
            component_test.go:198: expected value of env "CSI_PROVISIONER_NODE_AFFINITY":"storage_node_key1=storage_node_value1; storage_node_key2=storage_node_value2;", got "CSI_PROVISIONER_NODE_AFFINITY":"storage_node_key2=storage_node_value2; storage_node_key1=storage_node_value1;"
        --- FAIL: TestConversion/EnvVars/AGENT_NODE_AFFINITY (0.00s)
            component_test.go:198: expected value of env "AGENT_NODE_AFFINITY":"storage_node_key1=storage_node_value1; storage_node_key2=storage_node_value2;", got "AGENT_NODE_AFFINITY":"storage_node_key2=storage_node_value2; storage_node_key1=storage_node_value1;"
        --- FAIL: TestConversion/EnvVars/DISCOVER_AGENT_NODE_AFFINITY (0.00s)
            component_test.go:198: expected value of env "DISCOVER_AGENT_NODE_AFFINITY":"storage_node_key1=storage_node_value1; storage_node_key2=storage_node_value2;", got "DISCOVER_AGENT_NODE_AFFINITY":"storage_node_key2=storage_node_value2; storage_node_key1=storage_node_value1;"
FAIL
coverage: 77.4% of statements
FAIL    github.com/kinvolk/lokomotive/pkg/components/rook       0.267s

surajssd · 2020-09-10T13:17:17Z

@johananl Thanks for catching that.

make test is failing for me:

I have filed an issue here: #942.

Also I have disabled the tests that have inconsistent output and are prone to fail randomly.

pkg/components/rook/component_test.go

invidian

OK

pkg/components/rook/component_test.go

johananl · 2020-09-11T10:57:01Z

pkg/components/rook/component_test.go

+	return deploy
+}
+
+// nolint:funlen


Suggested change

// nolint:funlen

//nolint:funlen

A space after comment makes it more readable. Also this is consistent with other comments.

@johananl is right: https://golangci-lint.run/usage/false-positives/.

Also:

0 ✓ (78.1ms) 13:55:01 invidian@dellxps15mateusz ~/repos/kinvolk/lokomotive (invidian/unused-code)$ git grep '//nolint' | wc -l 27 0 ✓ (115ms) 13:55:03 invidian@dellxps15mateusz ~/repos/kinvolk/lokomotive (invidian/unused-code)$ git grep '// nolint' | wc -l 26

😄

# Comments with space after // $ grep '// ' | wc -l 127153 # Comments without space after // $ grep '//' | grep -v '// ' | wc -l 16671

Yes, but IMO the rule does not apply, as nolint is a meta comment. Regular comments should have the space for readability.

Yes, my suggestion is strictly (!) for nolint comments. This seems to be the prevailing standard. I don't care too much which one we do as long as we're consistent.

pkg/components/rook/component_test.go

invidian · 2020-09-11T12:27:02Z

pkg/components/rook/component_conversion_test.go

+		tc := tc
+		t.Run(tc.Name, func(t *testing.T) {
+			t.Parallel()
+			tc.Test(t, m)


After seeing @johananl comments and looking at this code, This indeed doesn't seem to make sense to define those tests in the array?

You could just have a helper function returning deploy and m and have separate test cases with proper names.

That would be simpler case if there is no logic shared between the tests at the end.

What's wrong in defining them in an array? Which test cases has "improper" names?

I think it's just too complex for no reason and inconsistent with other tests we have written. We could write all tests we have this way.

IMHO this way of writing code is more readable because you can look at the array and understand what is going on. All the boilerplate can be ignored for a reader.

I think table-driven tests (tests cases in a slice) are a very good practice in Go. They are also recommended officially and unofficially. Besides being easy to read and maintain, defining all the test-case-specific data in a declarative data structure forces you to think about your tests cases in a more structured - and generic - way.

What I think doesn't make sense is to declare a function in every test case. This indeed bloats the test case data structure.

Yes, I agree table-driven tests are good, but if they share common test logic, which is not the case here, as we only share preparation logic and we define testing function in a table... All examples you linked @johananl actally share the test logic.

My point was that perhaps we can structure the tests so that they have a shared logic. I didn't try to refactor the tests myself and don't want to halt the merging for this, just something for @surajssd to think about in case there is an obvious way to improve how we're doing things here.

Changed the code to be in different functions which resue the boilterplate.

invidian

Some thoughts, but nothing really blocking.

pkg/components/rook/component_conversion_test.go

invidian · 2020-09-14T10:10:37Z

pkg/components/rook/component_conversion_test.go

+	}
+}
+
+func memoryResources(t *testing.T, m map[string]string, deploy *appsv1.Deployment) {


To be honest, I'd prefer to have isolated tests with common helper rather than a dispatcher test, I think it is simpler while not adding much of a boilerplate. This make setup part of the test obvious/explicit.

Suggested change

func memoryResources(t *testing.T, m map[string]string, deploy *appsv1.Deployment) {

func TestMemoryResources(t *testing.T, m map[string]string, deploy *appsv1.Deployment) {

m, deploy := manifestsAndDeployment(t)

invidian · 2020-09-14T10:12:08Z

pkg/components/rook/component_conversion_test.go

+`
+
+	m := renderManifests(t, configHCL)
+	if len(m) == 0 {


With such code, this condition should be put in separate test IMO.

If I put it in a separate function, how do I stop when such an error is encountered? If this fails as in len(m) is zero then this test will fail on error: Operator deployment config not found.

So catching this before we proceed further makes sense and gives up precise error.

Hmm, yeah, I this sort of make sense. But other tests will fail gracefully if we remove this condition and we assume all rendering fails? I wonder how this should be handled in Go 🤔 I guess it's fine then 😄

The way the tests are currently structured, rendering the manifest is not the actual check in the unit test but rather a preliminary step. What @invidian is talking about sounds like treating a zero-length manifest as a test case on its own (which is valid, but possibly a different "category" than the tests in this function).

invidian · 2020-09-14T11:35:13Z

It seems this PR hit #940 many times :| And #952 simplifies modifying the timeouts.

surajssd · 2020-09-14T11:50:30Z

It seems this PR hit #940 many times :| And #952 simplifies modifying the timeouts.

Yeah, I thought it was one off, but then my cluster that I created locally also timed out.

pkg/components/rook/component_conversion_test.go

johananl

LGTM. We can keep iterating on the tests but the current state is obviously better than current master and I don't see a good reason to delay merging further.

invidian

LGTM. We can keep iterating on the tests but the current state is obviously better than current master and I don't see a good reason to delay merging further.

I'm a bit concerned about introducing the dispatcher pattern as it may be then copied in the future and become a habit and I disagree that this is the right way to write unit tests in Go. But I'm also fine with merging.

johananl · 2020-09-15T10:14:53Z

I think I have an alternative proposal to the current test structure. I'm still typing it but if you want to wait a bit I can push it somewhere.

johananl · 2020-09-15T10:56:14Z

Pushed an example here: https://github.com/kinvolk/lokomotive/blob/39e3a898fa955e8493d8ffca187ca5736d6d1050/pkg/components/rook/component_conversion_test.go

Add a memory limit override, by increasing it to 512Mi. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

Add unit tests that verify if the conversion worked correctly. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

surajssd requested review from invidian and johananl September 10, 2020 10:38

surajssd force-pushed the surajssd/upgrade-memory-limits-rook branch from a6ac636 to 58bcbf7 Compare September 10, 2020 13:15

invidian reviewed Sep 10, 2020

View reviewed changes

pkg/components/rook/component_test.go Outdated Show resolved Hide resolved

surajssd force-pushed the surajssd/upgrade-memory-limits-rook branch from 58bcbf7 to fe336ff Compare September 10, 2020 14:55

surajssd requested a review from invidian September 10, 2020 15:01

invidian previously approved these changes Sep 10, 2020

View reviewed changes

knrt10 mentioned this pull request Sep 11, 2020

Fix output of convertNodeSelector in rook #945

Merged

johananl reviewed Sep 11, 2020

View reviewed changes

surajssd dismissed invidian’s stale review via 032acfe September 11, 2020 11:59

surajssd force-pushed the surajssd/upgrade-memory-limits-rook branch from fe336ff to 032acfe Compare September 11, 2020 11:59

knrt10 requested review from johananl and invidian September 11, 2020 12:25

invidian suggested changes Sep 11, 2020

View reviewed changes

invidian reviewed Sep 14, 2020

View reviewed changes

surajssd force-pushed the surajssd/upgrade-memory-limits-rook branch from 21e761c to 8a1baa6 Compare September 14, 2020 11:03

johananl reviewed Sep 14, 2020

View reviewed changes

surajssd force-pushed the surajssd/upgrade-memory-limits-rook branch from 8a1baa6 to bc4d404 Compare September 15, 2020 07:11

surajssd requested review from johananl and invidian September 15, 2020 07:11

surajssd force-pushed the surajssd/upgrade-memory-limits-rook branch 2 times, most recently from ee913b8 to 109a3f3 Compare September 15, 2020 07:30

johananl previously approved these changes Sep 15, 2020

View reviewed changes

invidian previously approved these changes Sep 15, 2020

View reviewed changes

surajssd added 2 commits September 15, 2020 16:51

rook: Increase the memory limit of rook operator

35bd295

Add a memory limit override, by increasing it to 512Mi. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

rook: Add lokocfg conversion tests

c422331

Add unit tests that verify if the conversion worked correctly. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

surajssd dismissed stale reviews from invidian and johananl via c422331 September 15, 2020 11:22

surajssd force-pushed the surajssd/upgrade-memory-limits-rook branch from 109a3f3 to c422331 Compare September 15, 2020 11:22

surajssd requested review from johananl and invidian September 15, 2020 12:40

invidian approved these changes Sep 15, 2020

View reviewed changes

surajssd merged commit 2e1ac51 into master Sep 15, 2020

surajssd deleted the surajssd/upgrade-memory-limits-rook branch September 15, 2020 12:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Override memory limits of rook operator to 512Mi #938

Override memory limits of rook operator to 512Mi #938

surajssd commented Sep 10, 2020

johananl commented Sep 10, 2020

surajssd commented Sep 10, 2020

invidian left a comment

johananl Sep 11, 2020

surajssd Sep 11, 2020

invidian Sep 11, 2020 •

edited

Loading

surajssd Sep 11, 2020

invidian Sep 11, 2020

johananl Sep 11, 2020 •

edited

Loading

invidian Sep 11, 2020

surajssd Sep 11, 2020 •

edited

Loading

invidian Sep 11, 2020

surajssd Sep 11, 2020

johananl Sep 11, 2020 •

edited

Loading

invidian Sep 11, 2020

johananl Sep 11, 2020

surajssd Sep 14, 2020

invidian left a comment

invidian Sep 14, 2020

invidian Sep 14, 2020

surajssd Sep 14, 2020

invidian Sep 14, 2020

johananl Sep 15, 2020

invidian commented Sep 14, 2020

surajssd commented Sep 14, 2020

johananl left a comment

invidian left a comment

johananl commented Sep 15, 2020

johananl commented Sep 15, 2020

	func memoryResources(t testing.T, m map[string]string, deploy appsv1.Deployment) {
	func TestMemoryResources(t testing.T, m map[string]string, deploy appsv1.Deployment) {
	m, deploy := manifestsAndDeployment(t)

Override memory limits of rook operator to 512Mi #938

Override memory limits of rook operator to 512Mi #938

Conversation

surajssd commented Sep 10, 2020

johananl commented Sep 10, 2020

surajssd commented Sep 10, 2020

invidian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

invidian Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johananl Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajssd Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johananl Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

invidian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

invidian commented Sep 14, 2020

surajssd commented Sep 14, 2020

johananl left a comment

Choose a reason for hiding this comment

invidian left a comment

Choose a reason for hiding this comment

johananl commented Sep 15, 2020

johananl commented Sep 15, 2020

invidian Sep 11, 2020 •

edited

Loading

johananl Sep 11, 2020 •

edited

Loading

surajssd Sep 11, 2020 •

edited

Loading

johananl Sep 11, 2020 •

edited

Loading