diff --git a/doc/admin-guide/files/records.yaml.en.rst b/doc/admin-guide/files/records.yaml.en.rst index 043ed594b65..24866ceb2da 100644 --- a/doc/admin-guide/files/records.yaml.en.rst +++ b/doc/admin-guide/files/records.yaml.en.rst @@ -1532,6 +1532,10 @@ Parent Proxy Configuration The amount of time allowed between connection retries to a parent cache that is unavailable. + Once this time has elapsed the parent is selected again as a retry candidate. It is + restored to the pool only if that retry actually succeeds; if the retry fails, the parent + remains unavailable and a further ``retry_time`` must elapse before it is tried again. + .. ts:cv:: CONFIG proxy.config.http.parent_proxy.max_trans_retries INT 2 Limits the number of simultaneous transactions that may retry a parent once the parents diff --git a/src/proxy/http/HttpTransact.cc b/src/proxy/http/HttpTransact.cc index b87412d7b29..4aaa8b81a5b 100644 --- a/src/proxy/http/HttpTransact.cc +++ b/src/proxy/http/HttpTransact.cc @@ -3836,12 +3836,6 @@ HttpTransact::handle_response_from_parent(State *s) TxnDbg(dbg_ctl_http_trans, "(hrfp)"); HTTP_RELEASE_ASSERT(s->current.server == &s->parent_info); - // if this parent was retried from a markdown, then - // notify that the retry has completed. - if (s->parent_result.retry) { - markParentUp(s); - } - simple_or_unavailable_server_retry(s); s->parent_info.state = s->current.state; diff --git a/tests/gold_tests/parent_proxy/parent_retry_availability.test.py b/tests/gold_tests/parent_proxy/parent_retry_availability.test.py new file mode 100644 index 00000000000..51a7b1448d1 --- /dev/null +++ b/tests/gold_tests/parent_proxy/parent_retry_availability.test.py @@ -0,0 +1,26 @@ +""" +Verify the retry path restores a parent only when the retry actually succeeds. +""" +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +Test.Summary = ''' +Both outcomes of a parent retry: a parent that is still up but not serving stays +marked down, and a parent that has recovered is restored to the pool. +''' + +Test.ATSReplayTest(replay_file='replays/parent_retry_failure_stays_down.replay.yaml') +Test.ATSReplayTest(replay_file='replays/parent_retry_success_restores.replay.yaml') diff --git a/tests/gold_tests/parent_proxy/replays/parent_retry_failure_stays_down.replay.yaml b/tests/gold_tests/parent_proxy/replays/parent_retry_failure_stays_down.replay.yaml new file mode 100644 index 00000000000..d2c5fc5bd88 --- /dev/null +++ b/tests/gold_tests/parent_proxy/replays/parent_retry_failure_stays_down.replay.yaml @@ -0,0 +1,215 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# +# A parent that accepts the connection but never sends a response ("up but not +# serving") must stay marked down when a retry probe also fails. +# +# A retried parent must only be restored once a retry actually succeeds. +# markParentUp() zeroes failedAt and failCount, and the markParentDown() that +# follows a failed probe takes its result->retry branch, which leaves +# new_fail_count at 0 and so never re-clears available. Restoring the parent +# before the probe's outcome is known therefore returns it to the pool with a +# clean counter after every retry_time window, no matter how long it stays +# degraded. That is why the restore belongs in the CONNECTION_ALIVE branch of +# HttpTransact::handle_response_from_parent and not at function entry. +# +# The verifier server plays the parent: server-response.delay (4s) exceeds the +# child's transaction_no_activity_timeout_out (2s), so ATS reads zero bytes and +# times out (INACTIVE_TIMEOUT), which enable_parent_timeout_markdowns=1 admits +# as a markdown-eligible failure. +# + +meta: + version: "1.0" + +autest: + description: 'A failed retry probe must not restore a degraded parent' + + server: + name: 'server-degraded' + # Every transaction is abandoned by ATS mid-flight (the response is still + # being held when the timeout fires), so the server may report an incomplete + # transaction cycle. The assertions live in the client statuses and the ATS + # diags log. + return_code: [0, 1] + + client: + name: 'client-degraded' + process_config: + # Each request waits out a 2s ATS timeout, and one waits a further 3s for + # retry_time. The default 5s poll timeout would abandon a transaction + # mid-flight and report no status violation at all. + other_args: '--poll-timeout 30000' + + # Resolve every hostname to loopback. The origin is never actually contacted + # (go_direct=false keeps every request on the parent), but ATS may still run + # an origin DNS lookup on the way to parent selection, and an unresolvable + # name would fail the request before selection runs. + dns: + name: 'dns-degraded' + + ats: + name: 'ts-degraded' + process_config: + enable_cache: false + + records_config: + proxy.config.diags.debug.enabled: 1 + proxy.config.diags.debug.tags: 'parent_select|http_trans' + # NOTE: no_dns_just_forward_to_parent is deliberately NOT set here. It + # routes the request through the parentExists() shortcut in + # HttpTransact::HandleRequest, which tests pRecord::available directly and + # so short-circuits to an error as soon as the parent is marked down -- + # before ParentRoundRobin::selectParent runs. The retry-window logic under + # test lives inside selectParent, so the request has to reach it. + # + # With the cache disabled every request is non-lookupable, and this + # setting (default 1) would then send each one direct to the origin + # instead of the parent -- "request not cacheable, so bypass parent". + proxy.config.http.uncacheable_requests_bypass_parent: 0 + # A read timeout only counts toward markdown when this is enabled. It is + # off by default upstream but enabled in the production config this + # reproduces. + proxy.config.http.parent_proxy.enable_parent_timeout_markdowns: 1 + # One timeout demotes the parent, keeping the test to four requests. + proxy.config.http.parent_proxy.fail_threshold: 1 + # Short enough that a single in-replay delay outlasts the down window. + proxy.config.http.parent_proxy.retry_time: 2 + # No intra-request retry looping; one attempt per request keeps the + # failure accounting one-to-one with the transactions below. + proxy.config.http.parent_proxy.total_connect_attempts: 1 + proxy.config.http.parent_proxy.per_parent_connect_attempts: 1 + # Keep the parent out of the HostStatus map so availability is governed + # purely by retry_time. + proxy.config.http.parent_proxy.self_detect: 0 + # Bound the wait on the silent parent. + proxy.config.http.transaction_no_activity_timeout_out: 2 + proxy.config.url_remap.remap_required: 0 + + # A single parent with go_direct=false: once it is unavailable there is + # nowhere else to go, so selection-time skipping is visible as a 502. + parent_config: + - 'dest_domain=. parent="127.0.0.1:{SERVER_HTTP_PORT}" go_direct=false parent_is_proxy=true' + + log_validation: + diags_log: + contains: + # The retry probe fails, so the parent must be re-marked down rather + # than left available. + - expression: 'Parent retry marked as down 127\.0\.0\.1:\d+' + description: 'The failed retry probe re-marks the parent down' + excludes: + # Note() emitted only by ParentSelectionStrategy::markParentUp. Every + # request in this replay fails, so no retry ever succeeds and the + # parent must never be restored. + - expression: 'http parent proxy 127\.0\.0\.1:\d+ restored with request' + description: 'A failed retry probe must not restore the parent' + +sessions: +- transactions: + # 1: the parent accepts but never responds. ATS times out (504) and the + # failure count reaches fail_threshold, so the parent is marked down as this + # request finishes. + - all: { headers: { fields: [[ uuid, degrade1 ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /degrade1 + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + delay: 4s + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 504 + + # 2: still inside retry_time. The parent is skipped at selection and, with no + # other parent and go_direct=false, ATS returns 502 without touching the leg. + # This confirms the markdown from request 1 actually took effect. + - all: { headers: { fields: [[ uuid, downcheck ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /downcheck + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 502 + + # 3: the retry probe. The client delay pushes this request past retry_time, so + # parent selection picks the parent as a retry candidate. The probe reaches the + # still-silent parent and times out (504). The parent must not be restored: + # markParentUp belongs to the CONNECTION_ALIVE branch, which this never takes. + - all: { headers: { fields: [[ uuid, retryprobe ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /retryprobe + delay: 3s + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + delay: 4s + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 504 + + # 4: the retry probe failed, so the parent must still be down. Selection skips + # it and ATS returns 502 without touching the leg. A 504 here would mean the + # failed probe had restored the parent and it was attempted again. + - all: { headers: { fields: [[ uuid, aftercheck ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /aftercheck + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + delay: 4s + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 502 diff --git a/tests/gold_tests/parent_proxy/replays/parent_retry_success_restores.replay.yaml b/tests/gold_tests/parent_proxy/replays/parent_retry_success_restores.replay.yaml new file mode 100644 index 00000000000..91a8942d53e --- /dev/null +++ b/tests/gold_tests/parent_proxy/replays/parent_retry_success_restores.replay.yaml @@ -0,0 +1,177 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# +# A parent that recovers must be restored to the pool by a successful retry. +# +# This is the positive counterpart to parent_retry_failure_stays_down.replay.yaml, +# which covers the case where the retry probe also fails. Together they pin both +# outcomes of the retry path: restore only on success, stay down on failure. +# +# The parent stops being silent partway through the replay, so the retry probe +# sent after retry_time elapses actually succeeds. markParentUp() must then run +# from the CONNECTION_ALIVE branch of handle_response_from_parent, clearing the +# failure count and returning the parent to the pool. +# + +meta: + version: "1.0" + +autest: + description: 'A successful retry restores a parent that had been marked down' + + server: + name: 'server-recovered' + # The first transaction is abandoned by ATS mid-flight (the response is + # still being held when the timeout fires), so the server may report an + # incomplete transaction cycle. + return_code: [0, 1] + + client: + name: 'client-recovered' + process_config: + # The first request waits out a 2s ATS timeout and the retry probe waits a + # further 3s for retry_time, both well past the 5s default. + other_args: '--poll-timeout 30000' + + # Resolve every hostname to loopback, as in the failing-retry scenario. + dns: + name: 'dns-recovered' + + ats: + name: 'ts-recovered' + process_config: + enable_cache: false + + records_config: + proxy.config.diags.debug.enabled: 1 + proxy.config.diags.debug.tags: 'parent_select|http_trans' + # See parent_retry_failure_stays_down.replay.yaml for why + # no_dns_just_forward_to_parent must stay unset and why this must be 0. + proxy.config.http.uncacheable_requests_bypass_parent: 0 + proxy.config.http.parent_proxy.enable_parent_timeout_markdowns: 1 + proxy.config.http.parent_proxy.fail_threshold: 1 + proxy.config.http.parent_proxy.retry_time: 2 + proxy.config.http.parent_proxy.total_connect_attempts: 1 + proxy.config.http.parent_proxy.per_parent_connect_attempts: 1 + proxy.config.http.parent_proxy.self_detect: 0 + proxy.config.http.transaction_no_activity_timeout_out: 2 + proxy.config.url_remap.remap_required: 0 + + parent_config: + - 'dest_domain=. parent="127.0.0.1:{SERVER_HTTP_PORT}" go_direct=false parent_is_proxy=true' + + log_validation: + diags_log: + contains: + # Note() from ParentSelectionStrategy::markParentUp, emitted only when + # a retry succeeds against a parent with a non-zero failure count. + - expression: 'http parent proxy 127\.0\.0\.1:\d+ restored with request' + description: 'A successful retry restores the parent' + excludes: + # The retry succeeds, so it must never be re-marked down. + - expression: 'Parent retry marked as down' + description: 'A successful retry must not re-mark the parent down' + +sessions: +- transactions: + # 1: the parent accepts but never responds. ATS times out (504) and marks it + # down, since fail_threshold is 1. + - all: { headers: { fields: [[ uuid, degrade1 ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /degrade1 + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + delay: 4s + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 504 + + # 2: still inside retry_time, so the parent is skipped at selection and ATS + # returns 502 without touching the leg. + - all: { headers: { fields: [[ uuid, downcheck ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /downcheck + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 502 + + # 3: the retry probe. The client delay pushes it past retry_time so the parent + # is selected as a retry candidate, and this time the parent responds promptly + # (no server delay). The 200 proves the retry succeeded and the parent was + # restored from the CONNECTION_ALIVE branch. + - all: { headers: { fields: [[ uuid, retryprobe ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /retryprobe + delay: 3s + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 200 + + # 4: the parent is available again, so this request goes straight through + # without waiting for another retry_time window. + - all: { headers: { fields: [[ uuid, aftercheck ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /aftercheck + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 200