Adding Test for getShardedRules call #4449

alanprot · 2021-08-26T02:29:41Z

What this PR does:
During some investigation I could see that we don't have any test coverage on the Ruler#GetRules method at all - specially the getShardedRules call.

New interfaces were create in order to make possible to mock the rulers Clients and ClientPool.

The issue i was investigating is that even with shuffle sharding enabled, we call all rulers on GetRulers Call. This is because we are not using the subring to get the replicationSet. See:

cortex/pkg/ruler/ruler.go

Line 744 in b4daa22

rulers, err := r.ring.GetReplicationSetForOperation(RingOp)

This is specially problematic as we throw 5xx if a single ruler is unhealthy (as replicationFactor=1). We can see in the tests that even rulers in "LEAVING" state are considered unhealthy causing "too many unhealthy instances in the ring" errors.

I will follow up with the improvement PR as soon this one is merged (i did not want to do refactor + fix in the same PR to make it easier to read).

PS: Im not sure if I need to add a CHANGELOG entry if the PR only include more tests.

Which issue(s) this PR fixes:

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pkg/ruler/ruler_test.go

jeromeinsf · 2021-08-26T03:41:38Z

I guess

Tests updated

alanprot · 2021-08-26T18:09:38Z

The next change will be something like this: alanprot@f23f10f

This is the issue i wanted to fix.

bboreham · 2021-08-26T19:07:56Z

PS: Im not sure if I need to add a CHANGELOG entry if the PR only include more tests.

No - CHANGELOG is for a Cortex admin to see what affects them in the new version.

bboreham

Broadly good; I made some comments.
The test is now really long, so if it can be broken down to make it easier to follow that would be good.

bboreham · 2021-08-26T19:14:05Z

pkg/ruler/ruler.go

+	if clientPool == nil {
+		clientPool = newRulerClientPool(cfg.ClientTLSConfig, logger, reg)
+	}


why do we need this?

We don't! :D I just wanted to make sure that was not nil -> So added a default.

cortex/pkg/alertmanager/distributor.go

Lines 46 to 48 in 95a407f

if alertmanagerClientsPool == nil {

alertmanagerClientsPool = newAlertmanagerClientsPool(client.NewRingServiceDiscovery(alertmanagersRing), cfg, logger, reg)

}

But i can remove NP.

The test is now really long, so if it can be broken down to make it easier to follow that would be good.

I know... but if I split into 2 tests i will have basically the same test cases + lots of common code on setting up and everything... what do you think?

The test is now really long, so if it can be broken down to make it easier to follow that would be good.

Ok.. I did it! haha
I created a brand new test case -> it indeed seems easier to read.

You referred to NewDistributor(), but the difference is that function is exported; it makes sense that callers from another package can pass nil to get the default.
newRuler() is local to this package, and you never call it with nil, so these three lines are not required.

bboreham · 2021-08-26T19:16:10Z

pkg/ruler/ruler_test.go

-	user1Group2 := &rulespb.RuleGroupDesc{User: user1, Namespace: "namespace", Name: "second"}
-	user2Group1 := &rulespb.RuleGroupDesc{User: user2, Namespace: "namespace", Name: "first"}
-	user3Group1 := &rulespb.RuleGroupDesc{User: user3, Namespace: "namespace", Name: "first"}
+	user1Group1 := &rulespb.RuleGroupDesc{User: user1, Namespace: "namespace", Name: "first", Interval: 10 * time.Second}


Do we actually wait 10 seconds? Might be worth commenting why this specific value, or having it as a constant.

We dont... we just need something > 0 as if is 0 we will get a "Divided by Zero" error when loading the rules. Do you think it worth extract to a const anyway?

pkg/ruler/ruler_test.go

alanprot · 2021-08-31T17:17:07Z

Hi @bboreham
I updated the PR and created a separated test... Can u take a look now?

Thanks

bboreham

Thanks for updating. One small comment but can fix it up later if necessary.

bboreham · 2021-09-01T14:56:20Z

pkg/ruler/ruler.go

+	if clientPool == nil {
+		clientPool = newRulerClientPool(cfg.ClientTLSConfig, logger, reg)
+	}


You referred to NewDistributor(), but the difference is that function is exported; it makes sense that callers from another package can pass nil to get the default.
newRuler() is local to this package, and you never call it with nil, so these three lines are not required.

Signed-off-by: Alan Protasio <[email protected]>

alanprot · 2021-09-01T16:54:49Z

Thanks for updating. One small comment but can fix it up later if necessary.

That make sense! Removed!

I rebase the change and resolved the conflicts as well.

Thanks a lot!

alanprot · 2021-09-02T17:15:48Z

Thanks @bboreham :D

Signed-off-by: Alan Protasio <[email protected]> Signed-off-by: Alvin Lin <[email protected]>

pull-request-size bot added the size/L label Aug 26, 2021

alanprot force-pushed the test/GetRules branch from bb76c5a to 10ffb4b Compare August 26, 2021 02:29

alanprot changed the title ~~Improving Test Coverage for getShardedRules call~~ Adding Test for getShardedRules call Aug 26, 2021

alvinlin123 reviewed Aug 26, 2021

View reviewed changes

pkg/ruler/ruler_test.go Outdated Show resolved Hide resolved

alvinlin123 approved these changes Aug 26, 2021

View reviewed changes

alanprot force-pushed the test/GetRules branch 9 times, most recently from b80ca34 to a90e35c Compare August 26, 2021 17:48

bboreham reviewed Aug 26, 2021

View reviewed changes

alanprot force-pushed the test/GetRules branch 7 times, most recently from 4242b91 to f62510b Compare August 27, 2021 03:07

bboreham approved these changes Sep 1, 2021

View reviewed changes

Adding Test for getShardedRules call

16e1017

Signed-off-by: Alan Protasio <[email protected]>

alanprot force-pushed the test/GetRules branch from f62510b to 16e1017 Compare September 1, 2021 16:42

bboreham merged commit 03f926b into cortexproject:master Sep 2, 2021

alvinlin123 pushed a commit to ac1214/cortex that referenced this pull request Jan 14, 2022

Adding Test for getShardedRules call (cortexproject#4449)

5db90fd

Signed-off-by: Alan Protasio <[email protected]> Signed-off-by: Alvin Lin <[email protected]>

	if alertmanagerClientsPool == nil {
	alertmanagerClientsPool = newAlertmanagerClientsPool(client.NewRingServiceDiscovery(alertmanagersRing), cfg, logger, reg)
	}

Adding Test for getShardedRules call #4449

Adding Test for getShardedRules call #4449

Uh oh!

Conversation

alanprot commented Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jeromeinsf commented Aug 26, 2021

Uh oh!

alanprot commented Aug 26, 2021

Uh oh!

bboreham commented Aug 26, 2021

Uh oh!

bboreham left a comment

Choose a reason for hiding this comment

Uh oh!

bboreham Aug 26, 2021

Choose a reason for hiding this comment

Uh oh!

alanprot Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanprot Aug 26, 2021

Choose a reason for hiding this comment

Uh oh!

alanprot Aug 26, 2021

Choose a reason for hiding this comment

Uh oh!

bboreham Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

bboreham Aug 26, 2021

Choose a reason for hiding this comment

Uh oh!

alanprot Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alanprot commented Aug 31, 2021

Uh oh!

bboreham left a comment

Choose a reason for hiding this comment

Uh oh!

bboreham Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

alanprot commented Sep 1, 2021

Uh oh!

alanprot commented Sep 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alanprot commented Aug 26, 2021 •

edited

Loading

alanprot Aug 26, 2021 •

edited

Loading

alanprot Aug 26, 2021 •

edited

Loading