You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2022/11/05 00:12:46 UTC
[GitHub] [trafficserver] serrislew opened a new pull request, #9181: Adding back set_connect_fail for generic I/O error
serrislew opened a new pull request, #9181:
URL: https://github.com/apache/trafficserver/pull/9181
Without setting the generic I/O error, the server doesn't know it has failed and `t_state.current.server->had_connect_fail()` in `HttpSM::track_connect_fail()` always returns false. As a result, when the server has exhausted its attempts, it fails to reach `mark_host_failure` and does not update that HostDBInfo entry's last_failure timeout field
I am not sure why it was deleted before and do not see its replacement where the server's `connect_result` field is updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] serrislew commented on pull request #9181: Adding back set_connect_fail for generic I/O error
Posted by GitBox <gi...@apache.org>.
serrislew commented on PR #9181:
URL: https://github.com/apache/trafficserver/pull/9181#issuecomment-1306342550
Yes, I will definitely add an autest. The PR is #7580. I spoke to Susan about this and it was initially deleted since NET_EVENT_OPEN should have preemptively set the server connect failure code (EIO) but the failed case I've been testing doesn't reach this event. Instead of setting the EIO as I have it now (since it could possibly override other more specific failures), I will set it before NET_EVENT_OPEN so it applies to all cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] serrislew commented on a diff in pull request #9181: Adding back set_connect_fail for generic I/O error
Posted by GitBox <gi...@apache.org>.
serrislew commented on code in PR #9181:
URL: https://github.com/apache/trafficserver/pull/9181#discussion_r1020596576
##########
tests/gold_tests/dns/replay/server_down.replay.yaml:
##########
@@ -0,0 +1,54 @@
+# 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.
+
+meta:
+ version: "1.0"
+
+sessions:
+- transactions:
+ - client-request:
+ delay: 2s
Review Comment:
The client request delay is to allow the host file to sync so HostDB will include entries and the DelayStart is specific to the test run. I don't think the test run actually needs a delay so I will remove that one and also add comments to the replay file!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] serrislew merged pull request #9181: Adding back set_connect_fail for generic I/O error
Posted by GitBox <gi...@apache.org>.
serrislew merged PR #9181:
URL: https://github.com/apache/trafficserver/pull/9181
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9181: Adding back set_connect_fail for generic I/O error
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #9181:
URL: https://github.com/apache/trafficserver/pull/9181#discussion_r1020584842
##########
tests/gold_tests/dns/replay/server_down.replay.yaml:
##########
@@ -0,0 +1,54 @@
+# 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.
+
+meta:
+ version: "1.0"
+
+sessions:
+- transactions:
+ - client-request:
+ delay: 2s
Review Comment:
Let's add comments explaining the reasoning for the delays in the replay file. In particular I'm currently trying to think through the relationship between this first delay and the DelayStart directives in the test.py file. Shouldn't only one of those be needed?
##########
tests/gold_tests/dns/dns_host_down.test.py:
##########
@@ -0,0 +1,96 @@
+'''
+Verify ATS handles down origin servers with domain cached correctly.
+'''
+# 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.
+from ports import get_port
+import os
+
+Test.Summary = '''
+Verify ATS handles down origin servers with cached domain correctly.
+'''
+
+
+class DownCachedOriginServerTest:
+ replay_file = "replay/server_down.replay.yaml"
+
+ def __init__(self):
+ """Initialize the Test processes for the test runs."""
+ self._dns_port = None
+
+ def _configure_trafficserver(self):
+ """Configure Traffic Server."""
+ self._ts = Test.MakeATSProcess("ts", enable_cache=False)
+
+ self._ts.Disk.remap_config.AddLine(
+ f"map / http://resolve.this.com:{self._server.Variables.http_port}/"
+ )
+
+ self._ts.Disk.records_config.update({
+ 'proxy.config.diags.debug.enabled': 1,
+ 'proxy.config.diags.debug.tags': 'hostdb|dns|http|socket',
+ 'proxy.config.http.connect_attempts_max_retries': 0,
+ 'proxy.config.http.connect_attempts_rr_retries': 0,
+ 'proxy.config.hostdb.fail.timeout': 10,
+ 'proxy.config.dns.resolv_conf': 'NULL',
+ 'proxy.config.hostdb.ttl_mode': 1,
+ 'proxy.config.hostdb.timeout': 2,
+ 'proxy.config.hostdb.lookup_timeout': 2,
+ 'proxy.config.http.transaction_no_activity_timeout_in': 2,
+ 'proxy.config.http.connect_attempts_timeout': 2,
+ 'proxy.config.hostdb.host_file.interval': 1,
+ 'proxy.config.hostdb.host_file.path': os.path.join(Test.TestDirectory, "hosts_file"),
+ })
+
+ # Even when the origin server is down, SM will return a hit-fresh domain from HostDB.
+ # After request has failed, SM should mark the IP as down
+ def _test_host_mark_down(self):
+ tr = Test.AddTestRun()
+ self._server = tr.AddVerifierServerProcess("server", DownCachedOriginServerTest.replay_file)
+ self._configure_trafficserver()
Review Comment:
I suggest moving this to the `__init__` function since it's a Test-level rather than a TestRun-level process.
##########
tests/gold_tests/dns/replay/server_down.replay.yaml:
##########
@@ -0,0 +1,54 @@
+# 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.
+
+meta:
+ version: "1.0"
+
+sessions:
+- transactions:
+ - client-request:
+ delay: 2s
+ method: "GET"
+ version: "1.1"
+ url: /dns/mark/down
+ headers:
+ fields:
+ - [ Host, example.com ]
+ - [ X-Request, request ]
+ - [ uuid, 1 ]
+
+ server-response:
+ status: 200
+
+ proxy-response:
+ status: 502
Review Comment:
Comments explaining the reasoning of these expected return codes would be helpful too.
##########
tests/gold_tests/dns/dns_host_down.test.py:
##########
@@ -0,0 +1,96 @@
+'''
+Verify ATS handles down origin servers with domain cached correctly.
+'''
+# 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.
+from ports import get_port
+import os
+
+Test.Summary = '''
+Verify ATS handles down origin servers with cached domain correctly.
+'''
+
+
+class DownCachedOriginServerTest:
+ replay_file = "replay/server_down.replay.yaml"
+
+ def __init__(self):
+ """Initialize the Test processes for the test runs."""
+ self._dns_port = None
+
+ def _configure_trafficserver(self):
+ """Configure Traffic Server."""
+ self._ts = Test.MakeATSProcess("ts", enable_cache=False)
+
+ self._ts.Disk.remap_config.AddLine(
+ f"map / http://resolve.this.com:{self._server.Variables.http_port}/"
+ )
+
+ self._ts.Disk.records_config.update({
+ 'proxy.config.diags.debug.enabled': 1,
+ 'proxy.config.diags.debug.tags': 'hostdb|dns|http|socket',
+ 'proxy.config.http.connect_attempts_max_retries': 0,
+ 'proxy.config.http.connect_attempts_rr_retries': 0,
+ 'proxy.config.hostdb.fail.timeout': 10,
+ 'proxy.config.dns.resolv_conf': 'NULL',
+ 'proxy.config.hostdb.ttl_mode': 1,
+ 'proxy.config.hostdb.timeout': 2,
+ 'proxy.config.hostdb.lookup_timeout': 2,
+ 'proxy.config.http.transaction_no_activity_timeout_in': 2,
+ 'proxy.config.http.connect_attempts_timeout': 2,
+ 'proxy.config.hostdb.host_file.interval': 1,
+ 'proxy.config.hostdb.host_file.path': os.path.join(Test.TestDirectory, "hosts_file"),
+ })
+
+ # Even when the origin server is down, SM will return a hit-fresh domain from HostDB.
+ # After request has failed, SM should mark the IP as down
+ def _test_host_mark_down(self):
+ tr = Test.AddTestRun()
+ self._server = tr.AddVerifierServerProcess("server", DownCachedOriginServerTest.replay_file)
+ self._configure_trafficserver()
+
+ tr.Processes.Default.StartBefore(self._server)
+ tr.Processes.Default.StartBefore(self._ts)
+
+ tr.DelayStart = 1
+ tr.AddVerifierClientProcess(
+ "client-1",
+ DownCachedOriginServerTest.replay_file,
+ http_ports=[self._ts.Variables.port],
+ other_args='--keys 1')
+
+ tr.NotRunningAfter = self._server
+
+ # After host has been marked down from previous test, HostDB should not return
+ # the host as available and DNS lookup should fail.
Review Comment:
Any diags.log entries we should verify with the test via ContainsExpression? (traffic.out is debug-level, and it's generally good to avoid depending upon those since they may change more regularly.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] serrislew commented on pull request #9181: Adding back set_connect_fail for generic I/O error
Posted by GitBox <gi...@apache.org>.
serrislew commented on PR #9181:
URL: https://github.com/apache/trafficserver/pull/9181#issuecomment-1310736814
[approve ci]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #9181: Adding back set_connect_fail for generic I/O error
Posted by GitBox <gi...@apache.org>.
zwoop commented on PR #9181:
URL: https://github.com/apache/trafficserver/pull/9181#issuecomment-1319296903
Cherry-picked to v9.2.x
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org