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

Remove APIEnableRulesBackup config and use Ring.ReplicationFactor ins… #5901

Merged

Conversation

emanlodovice
Copy link
Contributor

@emanlodovice emanlodovice commented Apr 25, 2024

…tead

What this PR does:
This PR removes the -experimental.ruler.api-enable-rules-backup config because it only makes sense if ruler.ring.replication-factor is set to a value greater than 1. So instead of having the a dedicated config we just infer that rules backup is enabled if ruler.ring.replication-factor is greater than 1.

Checklist

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

Signed-off-by: Emmanuel Lodovice <[email protected]>
CHANGELOG.md Outdated Show resolved Hide resolved
pkg/ruler/ruler.go Show resolved Hide resolved
@emanlodovice emanlodovice force-pushed the remove-api-enable-rules-backup branch from e8aba16 to 21ace57 Compare April 26, 2024 09:21
@yeya24
Copy link
Contributor

yeya24 commented Apr 26, 2024

Test failed related to minimize spread token, unrelated but worth taking a look. @alanprot

--- FAIL: TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy (34.36s)
    --- FAIL: TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy/num_instances_=_60,_num_zones_=_3,_stable_sharding_=_true (0.10s)
        ring_test.go:2446: random generator seed: 1714124452728006355
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x16f8a95]

goroutine 679 [running]:
testing.tRunner.func1.2({0x18fb9e0, 0x36fb220})
	/usr/local/go/src/testing/testing.go:1545 +0x3f7
testing.tRunner.func1()
	/usr/local/go/src/testing/testing.go:1548 +0x716
panic({0x18fb9e0?, 0x36fb220?})
	/usr/local/go/src/runtime/panic.go:[92](https://github.com/cortexproject/cortex/actions/runs/8846062636/job/24291178688?pr=5901#step:6:93)0 +0x270
github.com/cortexproject/cortex/pkg/ring.(*MinimizeSpreadTokenGenerator).GenerateTokens(0xc002ece750, 0xc0001df580, {0xc000015f65, 0xb}, {0xc000015f7a, 0x6}, 0x80, 0x1)
	/__w/cortex/cortex/pkg/ring/token_generator.go:131 +0xab5
github.com/cortexproject/cortex/pkg/ring.TestRing_ShuffleShardWithLookback_CorrectnessWithFuzzy.func1(0xc005be5380)
	/__w/cortex/cortex/pkg/ring/ring_test.go:2495 +0xe83
testing.tRunner(0xc005be5380, 0xc002bc9500)
	/usr/local/go/src/testing/testing.go:1595 +0x262
created by testing.(*T).Run in goroutine 6[94](https://github.com/cortexproject/cortex/actions/runs/8846062636/job/24291178688?pr=5901#step:6:95)
	/usr/local/go/src/testing/testing.go:1648 +0x846
FAIL	github.com/cortexproject/cortex/pkg/ring	738.167s

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Love this change. Great work. Thanks @emanlodovice!

@yeya24 yeya24 merged commit 1b005d3 into cortexproject:master Apr 26, 2024
16 checks passed
Kramer0x0 pushed a commit to Kramer0x0/cortex that referenced this pull request Apr 26, 2024
cortexproject#5901)

* Remove APIEnableRulesBackup config and use Ring.ReplicationFactor instead

Signed-off-by: Emmanuel Lodovice <[email protected]>

* Update changelog

Signed-off-by: Emmanuel Lodovice <[email protected]>

* Update change log

Signed-off-by: Emmanuel Lodovice <[email protected]>

* Consider AZ awareness disabled if only 1 zone is present in list rules

Signed-off-by: Emmanuel Lodovice <[email protected]>

---------

Signed-off-by: Emmanuel Lodovice <[email protected]>
yeya24 pushed a commit to yeya24/cortex that referenced this pull request Apr 27, 2024
cortexproject#5901)

* Remove APIEnableRulesBackup config and use Ring.ReplicationFactor instead

Signed-off-by: Emmanuel Lodovice <[email protected]>

* Update changelog

Signed-off-by: Emmanuel Lodovice <[email protected]>

* Update change log

Signed-off-by: Emmanuel Lodovice <[email protected]>

* Consider AZ awareness disabled if only 1 zone is present in list rules

Signed-off-by: Emmanuel Lodovice <[email protected]>

---------

Signed-off-by: Emmanuel Lodovice <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants