distributor: don't return 5xx on a single ingester outage #4388

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

Merged

alvinlin123 merged 4 commits into cortexproject:master from alanprot:5xx_repro

Jan 25, 2022

Member

alanprot commented Jul 29, 2021

What this PR does:
Fixes cases where a single ingester outage cause 5XXs.

Which issue(s) this PR fixes:
Fixes #4381

Checklist

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

pull-request-size bot added the size/L label

alanprot force-pushed the 5xx_repro branch 2 times, most recently from 08ef41b to c606386 Compare

July 29, 2021 18:27

Member Author

alanprot commented Jul 30, 2021

Just leaving this as a draft now to see if make sense...

@pracucci @bboreham @pstibrany @tomwilkie any thoughts?

Thanks

bboreham mentioned this pull request

Cortex return 5xx due a single ingester outage #4381

Closed

2 tasks

alanprot force-pushed the 5xx_repro branch 4 times, most recently from 64a8407 to a294a99 Compare

August 25, 2021 22:28

alanprot changed the title ~~Returning quorum error~~ Immediately return error on 4xx - DoBatch Operations

alanprot force-pushed the 5xx_repro branch 2 times, most recently from 7266c6c to 996fc93 Compare

August 25, 2021 23:03

alanprot marked this pull request as ready for review

August 26, 2021 01:42

Contributor

pracucci commented Aug 26, 2021

Let's keep talking in the issue until we reach a consensus on the logic. I left a comment to explain why I'm not sure returning 4xx on first 4xx received may be correct:
#4381 (comment)

alanprot force-pushed the 5xx_repro branch 3 times, most recently from 625957f to 2c3ccce Compare

October 4, 2021 03:18

alanprot changed the title ~~Immediately return error on 4xx - DoBatch Operations~~ Cortex return 5xx due a single ingester outage

bboreham changed the title ~~Cortex return 5xx due a single ingester outage~~ distributor: don't return 5xx on a single ingester outage

bboreham reviewed

View reviewed changes

Contributor

bboreham left a comment

I couldn't really follow the central logic. I suspect it does work, but it does so because of an intricate interplay. Can the code explain itself better or be less intricate?

pkg/ring/batch.go Outdated

Comment on lines 137 to 138

    
              		verifyIfShouldReturn := func() {

              			if sampleTrackers[i].remaining.Dec() == 0 {

Contributor

bboreham Nov 10, 2021

I would pass i as a parameter, to avoid any problem with closure binding.

pkg/ring/batch.go Outdated

    
              			if b.rpcsFailed.Inc() == 1 {

              				b.err <- err

              			}

              		} else {

              			if sampleTrackers[i].succeeded.Inc() != int32(sampleTrackers[i].minSuccess) {

              				verifyIfShouldReturn()

Contributor

bboreham Nov 10, 2021

This can call b.rpcsFailed.Inc(). Why are we incrementing the Failed number on the no-error branch?

Member Author

alanprot Nov 11, 2021

We only do that when there is no remaining ingesters to call anymore - So even though the last ingester succeeded, we need to return an error. (in other words, because there is cases where we return an error even though the ingesters call succeeded).

lets consider the following scenario:

First Request: 4xx
Second Request: 5xx (no quorum yet)
Third Request: 2xx (in this case we did not reach maxFailures for 4xx nor 5xx)

In this particular scenario we can see that even though we have 2 errors (4xx + 5xx) we did not returned any error yet (as we had no quorum). What the verifyIfShouldReturn is doing is basically checking that there is no remaining calls to do for this particular sample and id not, return an error (pick any of the 4xx or 5xx). Its important to note that this function is called only if we did not reach the minSuccess needed to return a success.

Ps:
b.rpcsFailed.Inc() is here only to make sure we only return one error for the whole batch.

pkg/distributor/distributor_test.go Outdated

    
              	// Using 489 just to make sure we are not hitting the &limits

              	// Simulating 2 4xx and 1 5xx -> Should return 4xx

              	ingesters[0].failResp.Store(httpgrpc.Errorf(489, "Throttling"))

Contributor

bboreham Nov 10, 2021

Is this a typo for 429, or am I missing some context?

Member Author

alanprot Nov 11, 2021

its a typo :D

pkg/distributor/distributor_test.go Outdated

    
              	ingesters[1].failResp.Store(httpgrpc.Errorf(500, "InternalServerError"))

              	ingesters[2].failResp.Store(httpgrpc.Errorf(489, "Throttling"))

              	for i := 0; i < 1000; i++ {

Contributor

bboreham Nov 10, 2021

Why 1000?

Member Author

alanprot Nov 11, 2021

I tried many different implementations and many of them failed due race conditions... so this tests is testing multiples times just to make sure we dont have a race condition problem

pkg/distributor/distributor_test.go Outdated

    
              	ingesters[1].failResp.Store(httpgrpc.Errorf(489, "Throttling"))

              	ingesters[2].failResp.Store(httpgrpc.Errorf(500, "InternalServerError"))

              	for i := 0; i < 10000; i++ {

Contributor

bboreham Nov 10, 2021

Why 10000?

Member Author

alanprot Nov 11, 2021

Ditto

pkg/distributor/distributor_test.go Outdated

    
              	require.NoError(t, err)

              	// Give time to the ring get updated with the KV value

              	time.Sleep(5 * time.Second)

Contributor

bboreham Nov 10, 2021

This is very long compared to sleeps in other tests - is there another way to ensure propagation?

Member Author

alanprot Nov 11, 2021

Yeah.. probably i can...

pkg/distributor/distributor_test.go Outdated

    
              	ingesters[1].happy.Store(true)

              	ingesters[2].happy.Store(true)

              	err := r.KVClient.CAS(context.Background(), ring.IngesterRingKey, func(in interface{}) (interface{}, bool, error) {

Contributor

bboreham Nov 10, 2021

Why are we only doing a CAS on one ingester?

Member Author

alanprot Nov 11, 2021

CAS return the whole ring here (in this case that has 3 ingesters):

r := in.(*ring.Desc)

And im just changing the state of ingester2 to "LEFT" -> So after this operation the ring will still have 3 ingester but ingester2 will be in left state.

pkg/ring/batch.go Outdated

    
            @@ -118,15 +134,28 @@ func (b *batchTracker) record(sampleTrackers []*itemTracker, err error) {
          
              	// The use of atomic increments here guarantees only a single sendSamples

              	// goroutine will write to either channel.

              	for i := range sampleTrackers {

              		verifyIfShouldReturn := func() {

Contributor

bboreham Nov 10, 2021

verify to me suggests a function without side-effects; this one has three.

Member Author

alanprot Nov 11, 2021

yeah..
i agree that this logic is hard to understand.. lemme try to rewrite in another way.

alanprot force-pushed the 5xx_repro branch 2 times, most recently from 4399ab1 to 49478f9 Compare

November 11, 2021 19:37

alanprot marked this pull request as draft

November 11, 2021 20:08

Member Author

alanprot commented Nov 11, 2021 •

edited

Loading

I converted to draft as seems that the batch.go file was moved to another package. I just rewrote the solution in maybe a clearer way to see the feedbacks. If this is the way to go i can do the change in the dskit package.

alanprot added 2 commits

January 18, 2022 22:30


          Returning quorum error

68b534b

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


          refactor

9fe7552

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

alanprot force-pushed the 5xx_repro branch from 49478f9 to f28b910 Compare

January 19, 2022 06:42

alanprot marked this pull request as ready for review

January 19, 2022 06:43

alanprot force-pushed the 5xx_repro branch 2 times, most recently from 0d6754f to a7efd0d Compare

January 19, 2022 07:06

Member Author

alanprot commented Jan 19, 2022

Hi...

I was going to do this change in the DSKit but as this code is back in cortex I rebased it here (I can still do there if someone think it is worth).

@bboreham do you think with the comments now the code is a little more clear?


          rebase

9944ca1

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

alanprot force-pushed the 5xx_repro branch from a7efd0d to 9944ca1 Compare

January 19, 2022 07:37

bboreham reviewed

View reviewed changes

Contributor

bboreham left a comment

Yes it does look better. I am down to complaining about comments now :-)

Take this comment, which you didn't change:

	// If we succeed, decrement each sample's pending count by one.

I think this sentence should have been removed long ago. The following sentence is more true:

           If we reach
	// the required number of successful puts on this sample, then decrement the
	// number of pending samples by one.

And it is still true, but you have added a similar comment on line 155. Maybe we don't need this one any more.

  If we successfully push all samples to
	// min success instances, wake up the waiting rpc so it can return early.

This is still true; perhaps could move to line 155.

	// Similarly, track the number of errors, and if it exceeds maxFailures
	// shortcut the waiting rpc.

This bit is more complicated now: the comment on line 144 is closer.

	// The use of atomic increments here guarantees only a single sendSamples
	// goroutine will write to either channel.

Still true, but you now have more atomics so perhaps clarify it is specifically rpcsFailed and rpcsPending that are referred to.

Separately, in the test I would like a comment explaining the 1000 and 10000 "magic numbers".

pkg/ring/batch.go Outdated

-              				b.err <- err
+              			// We count the error by error family (4xx and 5xx)
+              			errCount := sampleTrackers[i].recordError(err)
+              			// We should return an error if we reach the maxFailure (quorum) on a give error family OR

Contributor

bboreham Jan 24, 2022

s/give/given/

Member Author

alanprot Jan 25, 2022

Updated! :D

alanprot force-pushed the 5xx_repro branch 5 times, most recently from dab8dfb to 8a2b76d Compare

January 25, 2022 04:27


          Comments

1937b48

alanprot force-pushed the 5xx_repro branch from 8a2b76d to 1937b48 Compare

January 25, 2022 05:25

bboreham approved these changes

View reviewed changes

Contributor

bboreham left a comment

Approved!

Member Author

alanprot commented Jan 25, 2022

Thanks a lot @bboreham :D

alvinlin123 approved these changes

View reviewed changes

alvinlin123 merged commit 16c83f5 into cortexproject:master

This was referenced Jul 11, 2022

DoBatch preference to 4xx if error danielblando/cortex#10

Closed

DoBatch preference to 4xx if error #4783

Merged

dimitarvdimitrov mentioned this pull request

Add error class tracking to ring.DoBatch grafana/dskit#201

Merged

2 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels