[GitHub] trafficserver issue #1335: Deadlock in HostDB
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/issues/1335 @zwoop Where you able to come up with a reproducible case for this? I'm going to try and take a look this week-- it'd be easier if I had a repro method :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1456: Add TCP accept metric which tracks the total numb...
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/1456 It seems like we should have some sort of layered approach, where we keep track of tcp (l4) http (L5?) and TLS (L7). We have metrics for some of this it sounds, but IIRC they are a bit patchy. So I'm in favor of a raw "tcp accepted", and IMO the http incoming_connections should probably be 1/2 agnostic (if we want an http_stream_accepted-- we can do that too). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1223: Add docs for the hostdb cache metrics
Github user jacksontj closed the pull request at: https://github.com/apache/trafficserver/pull/1223 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1223: Add docs for the hostdb cache metrics
GitHub user jacksontj opened a pull request: https://github.com/apache/trafficserver/pull/1223 Add docs for the hostdb cache metrics You can merge this pull request into a Git repository by running: $ git pull https://github.com/jacksontj/trafficserver hostdb_docs Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/1223.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1223 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1186: TestCase fix: Fix retry counter
Github user jacksontj closed the pull request at: https://github.com/apache/trafficserver/pull/1186 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1186: TestCase fix: Fix retry counter
GitHub user jacksontj opened a pull request: https://github.com/apache/trafficserver/pull/1186 TestCase fix: Fix retry counter ++count in python adds nothing to count, it does not incr the count variable. This simply fixes that to increment so it doesn't retru forever You can merge this pull request into a Git repository by running: $ git pull https://github.com/jacksontj/trafficserver master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/1186.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1186 commit 412cc00ad5473bf6dda153f4ceb88a736a42c53b Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-11-03T16:26:06Z TestCase fix: Fix retry counter ++count in python adds nothing to count, it does not incr the count variable. This simply fixes that to increment so it doesn't retru forever --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1168: Tsqa-- switch to using build that exist
Github user jacksontj closed the pull request at: https://github.com/apache/trafficserver/pull/1168 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1168: Tsqa-- switch to using build that exist
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/1168 @zwoop Hopefully all labelled correctly? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1168: Tsqa-- switch to using build that exist
GitHub user jacksontj opened a pull request: https://github.com/apache/trafficserver/pull/1168 Tsqa-- switch to using build that exist Now that trafficserver support exposing what features it has through `traffic_layout` we don't need to build ATS for every run of TSQA (yay!). Once the support is merged into apache/trafficserver-qa#5 we can merge this in and start running tests against `TS_ROOT`s that already exist. This changes the command from `make test` to something like `make test TSQA_ATS_ROOT=~/workspace/trafficserver/` (or wherever your env is). Single tests can still be run, but tsqa will still require the `TSQA_ATS_ROOT` environment variable, so a one-liner to do that would look like: `TSQA_ATS_ROOT=~/workspace/trafficserver/ nosetests tests/test_example.py` You can merge this pull request into a Git repository by running: $ git pull https://github.com/jacksontj/trafficserver tsqa Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/1168.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1168 commit 479060426966d2b4de23b35b7d28ddb3e4b64344 Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-11-01T03:56:17Z PoC TSQA running against existing builds commit 291a3b6056d3f5dc29474a6432e81926e897ed03 Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-11-01T04:16:50Z Fix broken test-- invalid python commit 032dd51345067334e1f52e23dbed690b536d2d40 Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-11-01T04:28:16Z Fix broken tests commit 3d49b2966b87b445c24a3352d4007e3e3149f73e Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-11-01T15:23:05Z Add timeouts, don't want tests to run forever commit 3bb533f2417cebfc9c31f86eab130db2968d3703 Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-11-01T15:23:37Z Don't re-run regression tests as of now the CI already runs the regression tests, so there is no need to re-run them in tsqa. At some point in the future we may consolidate these-- but for now we'll leave them separate --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1079: TS-4578: Skip HostDB lookup for all address liter...
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/1079 If the HttpSM skips hostdb not only does it skip the DNS lookup (which as @jpeach mentioned is already skipped) this will skip all `last_failure` detection. So, it seems that the assumption is that if the destination is localhost its okay to not check -- which IMO is wrong because its just another endpoint to talk to, so we want to have consistent behavior. IMO we should remove this special casing for localhost all together. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1156: TS-4968: Log a warning if connect_attempts...
Github user jacksontj closed the pull request at: https://github.com/apache/trafficserver/pull/1156 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1156: TS-4968: Log a warning if connect_attempts...
GitHub user jacksontj opened a pull request: https://github.com/apache/trafficserver/pull/1156 TS-4968: Log a warning if connect_attempts_rr_retries is >= connect_attempts_max_retries Fix formatting and compiler warning You can merge this pull request into a Git repository by running: $ git pull https://github.com/jacksontj/trafficserver TS-4968 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/1156.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1156 commit f2e66119c2a88cf7b0918b602ce6637edcd03664 Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-10-27T00:31:09Z TS-4968: Log a warning if connect_attempts_rr_retries is >= connect_attempts_max_retries Fix formatting and compiler warning --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1105: TS-4968: Log a warning if connect_attempts...
Github user jacksontj closed the pull request at: https://github.com/apache/trafficserver/pull/1105 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #947: TS-4796 Change UnixNetHandler to always bub...
Github user jacksontj closed the pull request at: https://github.com/apache/trafficserver/pull/947 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #947: TS-4796 Change UnixNetHandler to always bubble up ...
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/947 An update to summarize updates from today: After looking into it the core issue with the crash I was seeing is that the read/write side of the VIOs where being called regardless of which side the error came in on. Really we should handle the error on the appropriate side (in or out)-- so instead of allowing us to do the read (for example) when read OR error, this PR changes it so that we only call that routine if it was on the read side. Then within that read handler we can check if there was an error. Secondly, instead of trying to unset the error state in the handler, I'm simply just setting it every time we get into the read/write blocks to the appropriate values. So, the PR is now updated (with the patch I've tested) and ready for merge! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1109: TS-4970: Crash in INKVConnInternal when handle_ev...
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/1109 Closing the PR-- as we'll take care of this in Jira. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1108: TS-4970: Crash in INKVConnInternal when handle_ev...
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/1108 Closing the PR-- as we'll take care of this in Jira. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1108: TS-4970: Crash in INKVConnInternal when ha...
Github user jacksontj closed the pull request at: https://github.com/apache/trafficserver/pull/1108 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #947: TS-4796 Change UnixNetHandler to always bubble up ...
Github user jacksontj commented on the issue:
https://github.com/apache/trafficserver/pull/947
After doing some testing with this patch, I see crashes where write_to_net
is being called with a null vc lock.
```
(gdb) list
416 }
417 while ((vc = write_ready_list.dequeue())) {
418 if (vc->closed)
419 close_UnixNetVConnection(vc, trigger_event->ethread);
420 else if ((vc->write.enabled && vc->write.triggered) ||
vc->write.error)
421 write_to_net(this, vc, trigger_event->ethread);
422 else if (!vc->write.enabled) {
423 write_ready_list.remove(vc);
```
Seems that vc->write.error forces write_to_net to be called, but nothing is
checking that the vc is non-null.
Specifically:
```
(gdb) p lock
$1 = {m = {m_ptr = 0x0}, lock_acquired = 157}
(gdb) list
382 write_to_net_io(NetHandler *nh, UnixNetVConnection *vc, EThread *thread)
383 {
384 NetState *s = >write;
385 ProxyMutex *mutex = thread->mutex;
386
387 MUTEX_TRY_LOCK_FOR(lock, s->vio.mutex, thread, s->vio._cont); // <--
this line, specifically the s->vio.mutex
388
389 if (!lock.is_locked() || lock.get_mutex() != s->vio.mutex.m_ptr) {
390 write_reschedule(nh, vc);
391 return;
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1109: TS-4970: Crash in INKVConnInternal when handle_ev...
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/1109 After doing some more looking, the `m_deleted` flag is just marking the VConn as "we should delete this" and that combined with `m_deletable` lets it reschedule the delete in the future when it can. The fundamental problem I'm seeing is it is double freed under some conditions-- since all events trigger a delete. This is fixed in 7.x with TS-4590 -- so I've updated this PR to workaround the issue in a simpler mechanism that more closely mirrors what is happening on 7.x --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1109: TS-4970: Crash in INKVConnInternal when handle_ev...
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/1109 @shinrich I did look a bit more into the other backport-- and it seems that the outcome would be quite similar-- as the `handle_event` method is still using this `m_deleted` flag to determine if the struct was deleted, so it ends up being a superset of this change. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1108: TS-4970: Crash in INKVConnInternal when handle_ev...
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/1108 @jpeach yes, although since 5.2.x drops support in the next month-- its probably not work backporting to the 5.2.x branch-- there is another PR for 6.2.x. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1109: TS-4970: Crash in INKVConnInternal when handle_ev...
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/1109 @shinrich we could-- this patch seems to be working fine for our build though-- seemed like a less intrusive patch to an LTS release. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1108: TS-4970: Crash in INKVConnInternal when ha...
Github user jacksontj commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1108#discussion_r83342298
--- Diff: proxy/InkAPI.cc ---
@@ -1053,15 +1053,14 @@ int
INKVConnInternal::handle_event(int event, void *edata)
{
handle_event_count(event);
- if (m_deleted) {
-if (m_deletable) {
- this->mutex = NULL;
- m_read_vio.set_continuation(NULL);
- m_write_vio.set_continuation(NULL);
- INKVConnAllocator.free(this);
-}
- } else {
+ // If the VConn isn't deleted, call the handler
+ if (!m_deleted) {
return m_event_func((TSCont) this, (TSEvent) event, edata);
+ } else {
+// if the VConn is deleted, and we are in DEBUG mode-- we should assert
+// because this means that the VConn was cleaned up before all the
callbacks
+// (timeouts, etc.) where canceled.
+ink_assert("event on deleted INKVConnInternal");
--- End diff --
I think it might also be worthwhile to put a log line (warning?) here for
non-debug builds. Thoughts?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1108: TS-4970: Crash in INKVConnInternal when handle_ev...
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/1108 @jpeach The issue is that an event shows up after the object was destroyed() the destroy() mechanism sets the deleted flag-- previously what was happening is that if an event showed up after it was `destroy()`d then it would attempt to re-destroy it. This change is simply to not re-destroy it, but rather do nothing. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1108: TS-4970: Crash in INKVConnInternal when ha...
Github user jacksontj commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1108#discussion_r83455610
--- Diff: proxy/InkAPI.cc ---
@@ -1053,15 +1053,14 @@ int
INKVConnInternal::handle_event(int event, void *edata)
{
handle_event_count(event);
- if (m_deleted) {
-if (m_deletable) {
- this->mutex = NULL;
- m_read_vio.set_continuation(NULL);
- m_write_vio.set_continuation(NULL);
- INKVConnAllocator.free(this);
-}
- } else {
+ // If the VConn isn't deleted, call the handler
+ if (!m_deleted) {
return m_event_func((TSCont) this, (TSEvent) event, edata);
+ } else {
--- End diff --
@igalic This handler shouldn't be called after the VConn was destroyed. The
else exists soely to get the assert in there (for debug builds)-- because if we
hit that `else` path there is a bug-- but we don't want ATS to crash on it if
possible.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1109: TS-4970: Crash in INKVConnInternal when handle_ev...
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/1109 @jpeach TBH I'm not sure, I figured the PR would be good at least to get the CI to run on it-- I'm not sure what our official process is now that we do github for *some* things --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1106: Add prr proxy_request_retries log field
Github user jacksontj closed the pull request at: https://github.com/apache/trafficserver/pull/1106 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1106: Add prr proxy_request_retries log field
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/1106 Seems that this does already exists from TS-4148, just couldn't find it ;) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1109: TS-4970: Crash in INKVConnInternal when ha...
GitHub user jacksontj opened a pull request: https://github.com/apache/trafficserver/pull/1109 TS-4970: Crash in INKVConnInternal when handle_event is called after destroy() You can merge this pull request into a Git repository by running: $ git pull https://github.com/jacksontj/trafficserver TS-4970_ATS6 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/1109.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1109 commit d9998523404d2f3dcc5061e7636d4dfe5d81882d Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-10-14T00:23:50Z TS-4970: Crash in INKVConnInternal when handle_event is called after destroy() --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1108: TS-4970: Crash in INKVConnInternal when ha...
GitHub user jacksontj opened a pull request: https://github.com/apache/trafficserver/pull/1108 TS-4970: Crash in INKVConnInternal when handle_event is called after destroy() You can merge this pull request into a Git repository by running: $ git pull https://github.com/jacksontj/trafficserver TS-4970_ATS5 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/1108.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1108 commit 1210ad96278ccb29a0614b3234a2ed3743c3f6f6 Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-10-14T00:22:42Z TS-4970: Crash in INKVConnInternal when handle_event is called after destroy() --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1106: Add prr proxy_request_retries log field
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/1106 I'm not sure about the name (currently `prr`), anyone have opinions on the name? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1106: Add prr proxy_request_retries log field
GitHub user jacksontj opened a pull request: https://github.com/apache/trafficserver/pull/1106 Add prr proxy_request_retries log field You can merge this pull request into a Git repository by running: $ git pull https://github.com/jacksontj/trafficserver TS-4969 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/1106.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1106 commit 453e5c77902bf244b39b242939e934323178dc0d Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-10-13T19:11:29Z Add prr proxy_request_retries log field --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1105: TS-4968: Log a warning if connect_attempts...
GitHub user jacksontj opened a pull request: https://github.com/apache/trafficserver/pull/1105 TS-4968: Log a warning if connect_attempts_rr_retries is >= connect_attempts_max_retries You can merge this pull request into a Git repository by running: $ git pull https://github.com/jacksontj/trafficserver TS-4968 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/1105.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1105 commit 4ca4ca26e22864f415595f5be3a364e9f881ec99 Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-10-13T18:53:20Z TS-4968: Log a warning if connect_attempts_rr_retries is >= connect_attempts_max_retries --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1095: TS-4956: Memory leaks in hostdb test
Github user jacksontj commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1095#discussion_r82878003 --- Diff: iocore/hostdb/test_RefCountCache.cc --- @@ -135,8 +135,6 @@ testRefcounting() cache->put(1, to_delete); ret |= to_delete->refcount() != 1; cache->erase(1); - ret |= to_delete->refcount() != 0; --- End diff -- Since the memory is actually getting freed we should figure out a way to test that it was in fact deleted. An alternative is maybe to have it update a global map of key -> bool for things that where deleted? then we could simply do a get and ensure its not in the map. (something to replace the test instead of dropping it. Or maybe a global atomic for number of active ExampleStruct objects --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1095: TS-4956: Memory leaks in hostdb test
Github user jacksontj commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1095#discussion_r82878134
--- Diff: iocore/hostdb/test_RefCountCache.cc ---
@@ -166,6 +164,8 @@ testRefcounting()
ret |= tmpAfter.get()->idx != 1;
printf("ret=%d ref=%d\n", ret, tmp->refcount());
+ delete cache;
--- End diff --
we probably want to verify somehow that the destructor happened
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1070: TS-4509: Add `outstanding_bytes` to VConne...
Github user jacksontj closed the pull request at: https://github.com/apache/trafficserver/pull/1070 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1088: TS-4915: Crash from hostdb in PriorityQueueLess
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/1088 Re-pasting comments from IRC-- for easier retrieval. The pop you remove does seem extraneous-- and there are test cases for some of the refcountcache stuff (https://github.com/apache/trafficserver/blob/master/iocore/hostdb/test_RefCountCache.cc) should be easy enough to make a test case for evictions). As for the priority queue, I ran into a couple issues of it not doing what I wanted either. Thankfully that has some decent tests-- so assuming you can figure out the sequence of events that causes the problem-- adding a test case is quite simple (https://github.com/apache/trafficserver/blob/master/lib/ts/test_PriorityQueue.cc) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1070: TS-4509: Add `outstanding_bytes` to VConnection
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/1070 Definitely going to squash :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1035: TS-4866: Makes traffic_cop killing optional
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/1035 +1 on metric for this :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #947: TS-4796 Change UnixNetHandler to always bubble up ...
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/947 @zwoop ping again :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1070: TS-4509 Add `outstanding_bytes` to VConnec...
Github user jacksontj commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1070#discussion_r81797531 --- Diff: proxy/http/HttpSM.cc --- @@ -5310,9 +5310,6 @@ HttpSM::handle_post_failure() tunnel.deallocate_buffers(); tunnel.reset(); // Server died -vc_table.cleanup_entry(server_entry); -server_entry = NULL; -server_session= NULL; --- End diff -- We should get a new session-- but to do the "is retryable" check, I need access to the FD-- which is problematic if we release the session-- since it get cleared/closed etc. With these lines removed it seems that we aren't leaking sessions (from testing locally and in our staging env). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1070: TS-4509 Add `outstanding_bytes` to VConnection
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/1070 @jpeach I've updated the PR with your feedback-- although I'm not sure how to mark it as "done"-- its still showing that you have changes requested which is odd :/ --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1070: TS-4509 Add `outstanding_bytes` to VConnec...
GitHub user jacksontj opened a pull request: https://github.com/apache/trafficserver/pull/1070 TS-4509 Add `outstanding_bytes` to VConnection With this we can better check request retryability. This (in addition to not releasing the sessions immediately on error) means that if the request is retryable we can simply check if the number of bytes queued is the same as the number of bytes we've asked to write. If these match, then we can be sure we didn't send any ACKd packets-- meaning we are completely safe to retry. You can merge this pull request into a Git repository by running: $ git pull https://github.com/jacksontj/trafficserver TS-3959 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/1070.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1070 commit 760565954c2b7a9bb747d8e399a6b412b27466f0 Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-10-03T15:16:28Z TS-4509 Add `outstanding_bytes` to VConnection With this we can better check request retryability. This (in addition to not releasing the sessions immediately on error) means that if the request is retryable we can simply check if the number of bytes queued is the same as the number of bytes we've asked to write. If these match, then we can be sure we didn't send any ACKd packets-- meaning we are completely safe to retry. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #947: TS-4796 Change UnixNetHandler to always bubble up ...
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/947 @zwoop ping :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #947: TS-4796 Change UnixNetHandler to always bubble up ...
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/947 Sure, I ran it in a test env for a few days with no issues, I also have a test case for the RST behavior which is passing as well. Probably worth running on docs just to see if there is anything else, but I don't expect there to be any problems-- as we are only adding handling to a subset of cases which are currently ignored. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1020: TS-4867: Only schedule the serializer afte...
Github user jacksontj closed the pull request at: https://github.com/apache/trafficserver/pull/1020 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1020: TS-4867: Only schedule the serializer afte...
GitHub user jacksontj opened a pull request: https://github.com/apache/trafficserver/pull/1020 TS-4867: Only schedule the serializer after we have completely finish⦠â¦ed initializing Otherwise there is a race between initialization and the eventProcessor You can merge this pull request into a Git repository by running: $ git pull https://github.com/jacksontj/trafficserver TS-4867 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/1020.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1020 commit 64cf7597b670339215595e869d0ee7e4748e31c1 Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-09-14T18:12:47Z TS-4867: Only schedule the serializer after we have completely finished initializing Otherwise there is a race between initialization and the eventProcessor --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1004: TS-4641 Changes default for proxy.config.cache.ho...
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/1004 IIRC the plan was to leave this enabled by default? From what I remember of the conversations at the last summit-- the main problem with the current one is that its terrible and causes problems. This new one doesn't have a noticeable performance cost at all, so I'm not sure why we'd want to disable it by default. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #947: TS-4796 Change UnixNetHandler to always bub...
Github user jacksontj commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/947#discussion_r77858252
--- Diff: iocore/net/UnixNet.cc ---
@@ -525,7 +531,17 @@ NetHandler::mainNetEvent(int event, Event *e)
close_UnixNetVConnection(vc, trigger_event->ethread);
else if (vc->read.enabled && vc->read.triggered)
vc->net_read_io(this, trigger_event->ethread);
-else if (!vc->read.enabled) {
+else if (vc->read.error) {
+ int err = 0, errlen = sizeof(int);
+ if (getsockopt(vc->con.fd, SOL_SOCKET, SO_ERROR, , (socklen_t
*)) == -1) {
+err = errno;
+ }
+ if (err != EAGAIN && err != EINTR) {
+vc->readSignalError(this, err);
--- End diff --
That makes sense :) I've cleaned that up (left it as a separate commit for
review-- although before merging I'll squash it). I did test that its still
fixing my bug-- and it is, so I think we are set :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #947: TS-4796 Change UnixNetHandler to always bub...
Github user jacksontj commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/947#discussion_r77699771
--- Diff: iocore/net/UnixNet.cc ---
@@ -542,7 +555,14 @@ NetHandler::mainNetEvent(int event, Event *e)
close_UnixNetVConnection(vc, trigger_event->ethread);
else if (vc->write.enabled && vc->write.triggered)
write_to_net(this, vc, trigger_event->ethread);
-else if (!vc->write.enabled) {
+else if (vc->write.error) {
+ int err = 0, errlen = sizeof(int);
+ if (getsockopt(vc->con.fd, SOL_SOCKET, SO_ERROR, , (socklen_t
*)) == -1) {
+err = errno;
+ }
+ if (err != EAGAIN && err != EINTR)
+vc->writeSignalError(this, err);
+} else if (!vc->write.enabled) {
--- End diff --
Done :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #947: TS-4796 Change UnixNetHandler to always bubble up ...
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/947 That all sounds reasonable :) I just pushed a new commit here which does effectively what you are suggesting-- just both on the read and write side (as well as the few other little changes to make it work). I tested it and this covers my use-case (since we are still bubbling the error up when we aren't enabled) and should continue functioning the same for all the rest of the cases (since left them alone). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #947: TS-4796 Change UnixNetHandler to always bubble up ...
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/947 The behavior I see on master (without this patch) is that ATS doesn't close the session when getting the RST. From digging that UnixNetHandler gets an EPOLLERR -- which attempts to add it to the write_ready queue, but since the vc isn't enabled for writing the HTTPTunnel isn't ever called to do the write. So from your last 2 comments it sounds like a more correct approach would be to immediately schedule a read/write to the socket -- to determine if there was in fact an error. It also sounds like you are suggesting that inactivity cop should get these sessions? In my tests I see that these sessions are either killed by the next attempt to write to the closed socket (when the origin sends more bytes later) or when the transaction hits the max inactivity timeout. Neither of these behaviors are what we want-- since I already got an RST from the client. So it seems that we need to somehow force-schedule a read/write in these error conditions-- do you have any pointers on how to do that? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #947: TS-4796 Change UnixNetHandler to always bub...
GitHub user jacksontj opened a pull request: https://github.com/apache/trafficserver/pull/947 TS-4796 Change UnixNetHandler to always bubble up epoll errors to the VConnection Before if the vcon wasn't read or write enabled errors would be swallowed. This leads to a variety of issues where the socket errors aren't dealt with immediately. You can merge this pull request into a Git repository by running: $ git pull https://github.com/jacksontj/trafficserver TS-4796 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/947.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #947 commit 5b032bae9fe1d30bffa79a5ce8da12dabf7468be Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-08-31T20:02:53Z TS-4796 Change UnixNetHandler to always bubble up epoll errors to the VConnection Before if the vcon wasn't read or write enabled errors would be swallowed. This leads to a variety of issues where the socket errors aren't dealt with immediately. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #351: TS-4042: Add feature to buffer request body before...
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/351 @bryancall it'd be nice if we could have some way to "re-enable" the bytes-- such that a plugin could either buffer everything or stream (since the "buffer everything" case is a possible use-case. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #841: TS-4720 correctly check if requests have a ...
Github user jacksontj closed the pull request at: https://github.com/apache/trafficserver/pull/841 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #841: TS-4720 correctly check if requests have a body
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/841 I haven't added tests to the synthetic server before-- but it is a relatively easy case to reproduce. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #836: TS-4710 make srv_enabled transaction overri...
Github user jacksontj closed the pull request at: https://github.com/apache/trafficserver/pull/836 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #841: TS-4720 correctly check if requests have a ...
GitHub user jacksontj opened a pull request: https://github.com/apache/trafficserver/pull/841 TS-4720 correctly check if requests have a body HttpTransact defaults content length to `-1`, meaning that if the request has no content length header it will be `-1`. These checks weren't taking that into consideration -- meaning client aborts during requests with no content length (GET for example) would leave the origin session open until another timeout kicked in. You can merge this pull request into a Git repository by running: $ git pull https://github.com/jacksontj/trafficserver TS-4720 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/841.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #841 commit a075556602d77d79f5a02e2a36053abfa92703f0 Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-08-05T00:55:59Z TS-4720 correctly check if requests have a body HttpTransact defaults content length to `-1`, meaning that if the request has no content length header it will be `-1`. These checks weren't taking that into consideration -- meaning client aborts during requests with no content length (GET for example) would leave the origin session open until another timeout kicked in. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #836: TS-4710 make srv_enabled transaction overrideable
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/836 [approve ci] --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #836: TS-4710 make srv_enabled transaction overrideable
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/836 @zwoop fixed, i forgot to add the new string to that test case. Added, and the test is passing locally. Its a bit of a pain to add overrideable config-- as it has to be added in ~5 places :/ --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #836: TS-4710
GitHub user jacksontj opened a pull request: https://github.com/apache/trafficserver/pull/836 TS-4710 You can merge this pull request into a Git repository by running: $ git pull https://github.com/jacksontj/trafficserver TS-4710 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/836.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #836 commit c295fdcc375b778af4a000b20e7cbef10d5e627a Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-08-02T01:44:54Z TS-4710 document srv_enabled as overrideable commit 36999f9073e01499c520181d0efe9ea7ccca289d Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-08-02T01:45:17Z TS-4710 make `proxy.config.srv_enabled` transaction overrideable --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #827: Ts 4693
Github user jacksontj closed the pull request at: https://github.com/apache/trafficserver/pull/827 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #827: Ts 4693
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/827 I can squash the 2 test commits, I like to keep tests in separate commits from the code-- easier to backport :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #827: Ts 4693
GitHub user jacksontj opened a pull request: https://github.com/apache/trafficserver/pull/827 Ts 4693 You can merge this pull request into a Git repository by running: $ git pull https://github.com/jacksontj/trafficserver TS-4693 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/827.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #827 commit 832f04bcdf85ce6e04168ec48c8216e21a61840a Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-07-27T05:34:40Z TS-4693 fix priority selection for SRV records The "is_alive" check was backwards, meaning that instead of excluding dead hosts we were excluding healthy hosts. In the failure mode where all reals are dead select_best_srv picks a host at random since we believe all reals to be dead. commit 0065f0e1619037a7e4727eb161f01f6c65b1d3d0 Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-07-21T16:15:56Z Fix test_hostdb SRV testing Add better parsing for the proxy property-- so it can be used for http and https commit e5c7b435f9433e653f8a7183fd2cbf20b42dcca9 Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-07-27T05:50:29Z Re-enable tests for SRV priority and weight --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #768: TS-4615 set SRV scheme based on next_hop_sc...
Github user jacksontj closed the pull request at: https://github.com/apache/trafficserver/pull/768 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #820: TS-4622
Github user jacksontj closed the pull request at: https://github.com/apache/trafficserver/pull/820 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #820: TS-4622
GitHub user jacksontj opened a pull request: https://github.com/apache/trafficserver/pull/820 TS-4622 Cleanup of #773 You can merge this pull request into a Git repository by running: $ git pull https://github.com/jacksontj/trafficserver TS-4622 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/820.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #820 commit 13a70c1b48c95d3306dafa90194459ba6c8bd5ed Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-07-21T15:31:29Z Revert "TS-4622 Use port from SRV response when connecting to origin (#773)" This reverts commit d91457b8cf27beff88d32fb33a37423fb656b01a. Github UI squashed the tests and the code change-- we want them separate commit 1eebb832faade9ae876b7834b3e9842bb8178c4f Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-06-30T17:46:26Z TS-4622 use port from SRV response for origin connections This closes #773 commit 7ffa6cbc9a64ecfe6c73875aa9669bb3e6bcb26e Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-07-21T15:21:25Z Add some initial SRV tsqa tests --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #773: TS-4622 Use port from SRV response when con...
Github user jacksontj closed the pull request at: https://github.com/apache/trafficserver/pull/773 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #812: TS-4688 handle DNS compression labels in SR...
Github user jacksontj closed the pull request at: https://github.com/apache/trafficserver/pull/812 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #812: TS-4688 handle DNS compression labels in SR...
Github user jacksontj commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/812#discussion_r71542146
--- Diff: iocore/dns/DNS.cc ---
@@ -1514,24 +1514,24 @@ dns_process(DNSHandler *handler, HostEnt *buf, int
len)
cp += dn_skipname(cp, eom);
here = cp; /* hack */
SRV *srv = >srv_hosts.hosts[num_srv];
-int r= ink_ns_name_ntop(srv_off + SRV_SERVER, srv->host,
MAXDNAME);
-if (r <= 0) {
- /* FIXME: is this really an error? or just a continue; */
+
+// expand the name
+n = ink_dn_expand((u_char *)h, eom, srv_off + SRV_SERVER, (u_char
*)srv->host, MAXDNAME);
+if (n < 0) {
++error;
- goto Lerror;
+ break;
}
Debug("dns_srv", "Discovered SRV record [from NS lookup] with
cost:%d weight:%d port:%d with host:%s",
ink_get16(srv_off + SRV_COST), ink_get16(srv_off +
SRV_WEIGHT), ink_get16(srv_off + SRV_PORT), srv->host);
-srv->port= ink_get16(srv_off + SRV_PORT);
-srv->priority= ink_get16(srv_off + SRV_COST);
-srv->weight = ink_get16(srv_off + SRV_WEIGHT);
-srv->host_len= r;
-srv->host[r - 1] = '\0';
-srv->key = makeHostHash(srv->host);
+srv->port = ink_get16(srv_off + SRV_PORT);
+srv->priority = ink_get16(srv_off + SRV_COST);
+srv->weight = ink_get16(srv_off + SRV_WEIGHT);
+srv->host_len = ::strlen(srv->host) + 1;
--- End diff --
Sadly no, `n` is the length of the compressed string, not the expanded
string-- meaning I have to calculate the len of the expanded string.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #773: TS-4622 Use port from SRV response when connecting...
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/773 @jpeach I did some testing of my own and I can't reproduce any problems with `TsHttpTxnServerAddrSet()` at all. TSHttpTxnServerAddrSet() is supposed to be called before the DNS lookup. If called then the core bypasses the entire OS_DNS stuff which includes this port setting block in HttpTransact. I tested calling the API on both the `TS_EVENT_HTTP_READ_REQUEST_HDR` and `TS_EVENT_HTTP_OS_DNS` hooks-- with no problem at all. You mentioned in a previous comment that it was broken in your testing, can you share the code you were using to see it broken? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #773: TS-4622 Use port from SRV response when con...
Github user jacksontj commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/773#discussion_r71536060
--- Diff: proxy/http/HttpTransact.cc ---
@@ -1783,7 +1783,13 @@ HttpTransact::OSDNSLookup(State *s)
// update some state variables with hostdb information that has
// been provided.
ats_ip_copy(>server_info.dst_addr, s->host_db_info.ip());
- s->server_info.dst_addr.port() =
htons(s->hdr_info.client_request.port_get()); // now we can set the port.
+ // TODO ideally only if ports aren't defined in remap
+ // If the SRV response has a port number, we should honor it. Otherwise
we do the port defined in remap
+ if (s->dns_info.srv_port) {
+s->server_info.dst_addr.port() = htons(s->dns_info.srv_port);
+ } else {
+s->server_info.dst_addr.port() =
htons(s->hdr_info.client_request.port_get()); // now we can set the port.
+ }
--- End diff --
I'll change the conditional-- that definitely makes sense.
From looking at the API callout code-- it does seem like its already broken
:/
It seems that `TSHttpTxnServerAddrSet()` is called before the DNS lookup--
and if so it bypasses the [dns lookup|
https://github.com/apache/trafficserver/blob/master/proxy/http/HttpSM.cc#L7179].
So, I would think in this section of HttpTransact we would want to skip all
port setting if `api_server_addr_set` is true.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #812: TS-4688 handle DNS compression labels in SRV respo...
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/812 Yes, and we actually already do that for seemingly all the other types-- but not for SRV :/ --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #773: TS-4622 Use port from SRV response when connecting...
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/773 @jpeach `HttpTransact::OSDNSLookup` seems to be the correct place-- as it is called immediately after the DNS lookup is complete-- and is what sets the port numbers for the destination. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #809: TS-4674: Remove useless assert statement
Github user jacksontj closed the pull request at: https://github.com/apache/trafficserver/pull/809 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #812: TS-4688 handle DNS compression labels in SR...
GitHub user jacksontj opened a pull request: https://github.com/apache/trafficserver/pull/812 TS-4688 handle DNS compression labels in SRV responses You can merge this pull request into a Git repository by running: $ git pull https://github.com/jacksontj/trafficserver TS-4688 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/812.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #812 commit 7957efb8effea4533bbb3d8ff18ed5109e8c4877 Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-07-19T15:39:31Z TS-4688 handle DNS compression labels in SRV responses --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #811: TS-4684 Leaked references to HostDBInfos fr...
Github user jacksontj closed the pull request at: https://github.com/apache/trafficserver/pull/811 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #811: TS-4684 Leaked references to HostDBInfos fr...
GitHub user jacksontj opened a pull request: https://github.com/apache/trafficserver/pull/811 TS-4684 Leaked references to HostDBInfos from HttpTransact You can merge this pull request into a Git repository by running: $ git pull https://github.com/jacksontj/trafficserver TS-4684 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/811.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #811 commit ab3b5007ac231cb3ddb3f452eea8a00ed42d04cc Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-07-19T00:11:01Z TS-4684 Leaked references to HostDBInfos from HttpTransact --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #773: TS-4622 User port from SRV response when connectin...
Github user jacksontj commented on the issue: https://github.com/apache/trafficserver/pull/773 Yea, I'll have to look a bit more. Ideally its as close to that DNS lookup as possible, that way if some plugin wants to overwrite it I've done the change in core before they can (so that we won't be clobbering eachother). Have to look into it this week. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #809: TS-4674: Remove useless assert statement
GitHub user jacksontj opened a pull request: https://github.com/apache/trafficserver/pull/809 TS-4674: Remove useless assert statement Now that we have clean allocations (instead of clobbering existing things) there is no need to have this assertion. In practice this assertion is actually incorrect, because in the case where we want to extend the lifetime of a stale record (since the response we got was broken) we'll fail this assert. You can merge this pull request into a Git repository by running: $ git pull https://github.com/jacksontj/trafficserver TS-4674 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/809.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #809 commit 44f9dcddab6cfe755e1b6dd87093762c44e7e7b9 Author: Thomas Jackson <jacksontj...@gmail.com> Date: 2016-07-18T16:13:03Z TS-4674: Remove useless assert statement Now that we have clean allocations (instead of clobbering existing things) there is no need to have this assertion. In practice this assertion is actually incorrect, because in the case where we want to extend the lifetime of a stale record (since the response we got was broken) we'll fail this assert. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
