You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficserver.apache.org by danobi <gi...@git.apache.org> on 2016/10/03 20:18:48 UTC
[GitHub] trafficserver pull request #1073: TS-4399 TS-4400 Management API messes up p...
GitHub user danobi opened a pull request:
https://github.com/apache/trafficserver/pull/1073
TS-4399 TS-4400 Management API messes up proxy options
TS-4399: Management API breaks diagnostic log rotation
TS-4400: TSProxyStateSet persist cache clearing across restart
The two issues are related in that they both deal with the
management API not correctly handling proxy flags.
For TS-4399, it was because the management API was not aware
of traffic_manager setting extra proxy options. This was fixed
by providing CoreAPI a callback to get extra proxy options from
traffic_manager.
For TS-4400, it was because the management API was not properly
clearing optional flags between proxy reboots. This was fixed
by resetting the proxy options before each reboot.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/danobi/trafficserver TS-4399_TS-4400
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/1073.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 #1073
----
commit 9f28180ae4bebfb529a8a7f3bf6758cb44a3c57a
Author: Daniel Xu <dl...@yahoo.com>
Date: 2016-10-03T19:05:57Z
TS-4399 TS-4400 Management API messes up proxy options
TS-4399: Management API breaks diagnostic log rotation
TS-4400: TSProxyStateSet persist cache clearing across restart
The two issues are related in that they both deal with the
management API not correctly handling proxy flags.
For TS-4399, it was because the management API was not aware
of traffic_manager setting extra proxy options. This was fixed
by providing CoreAPI a callback to get extra proxy options from
traffic_manager.
For TS-4400, it was because the management API was not properly
clearing optional flags between proxy reboots. This was fixed
by resetting the proxy options before each reboot.
commit 3365b2048e7bf0660faccf573ffd198857d03d25
Author: Daniel Xu <dl...@yahoo.com>
Date: 2016-10-03T20:14:24Z
Fix memory leaks
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1073: TS-4399 TS-4400 Management API messes up p...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1073#discussion_r84208238
--- Diff: cmd/traffic_manager/traffic_manager.cc ---
@@ -611,31 +614,35 @@ main(int argc, const char **argv)
/* Update cmd line overrides/environmental overrides/etc */
if (tsArgs) { /* Passed command line args for proxy */
+ char* new_proxy_opts = new char[strlen(lmgmt->proxy_options) + strlen(tsArgs) + 1];
+ strcpy(new_proxy_opts, lmgmt->proxy_options);
+ strcat(new_proxy_opts, tsArgs);
ats_free(lmgmt->proxy_options);
- lmgmt->proxy_options = tsArgs;
- mgmt_log("[main] Traffic Server Args: '%s'\n", lmgmt->proxy_options);
+ lmgmt->proxy_options = new_proxy_opts;
}
- // we must pass in bind_stdout and bind_stderr values to TS
- // we do it so TS is able to create BaseLogFiles for each value
+ // TS needs to be started up with the same outputlog bindings each time,
+ // so we append the outputlog location to the persistent proxy options
+ //
+ // TS needs them to be able to create BaseLogFiles for each value
if (*bind_stdout != 0) {
- size_t l = strlen(lmgmt->proxy_options);
- size_t n = 3 /* " --" */
- + sizeof(TM_OPT_BIND_STDOUT) /* nul accounted for here */
- + 1 /* space */
- + strlen(bind_stdout);
- lmgmt->proxy_options = static_cast<char *>(ats_realloc(lmgmt->proxy_options, n + l));
- snprintf(lmgmt->proxy_options + l, n, " --%s %s", TM_OPT_BIND_STDOUT, bind_stdout);
+ const char *bind_stdout_opt = " --" TM_OPT_BIND_STDOUT " ";
+ char *new_proxy_opts = new char[strlen(lmgmt->proxy_options) + strlen(bind_stdout_opt) + strlen(bind_stdout) + 1];
+ strcpy(new_proxy_opts, lmgmt->proxy_options);
+ strcat(new_proxy_opts, bind_stdout_opt);
+ strcat(new_proxy_opts, bind_stdout);
+ delete lmgmt->proxy_options;
+ lmgmt->proxy_options = new_proxy_opts;
}
if (*bind_stderr != 0) {
- size_t l = strlen(lmgmt->proxy_options);
- size_t n = 3 /* space dash dash */
- + sizeof(TM_OPT_BIND_STDERR) /* nul accounted for here */
- + 1 /* space */
- + strlen(bind_stderr);
- lmgmt->proxy_options = static_cast<char *>(ats_realloc(lmgmt->proxy_options, n + l));
- snprintf(lmgmt->proxy_options + l, n, " --%s %s", TM_OPT_BIND_STDERR, bind_stderr);
+ const char *bind_stderr_opt = " --" TM_OPT_BIND_STDERR " ";
+ char *new_proxy_opts = new char[strlen(lmgmt->proxy_options) + strlen(bind_stderr_opt) + strlen(bind_stderr) + 1];
+ strcpy(new_proxy_opts, lmgmt->proxy_options);
+ strcat(new_proxy_opts, bind_stderr_opt);
+ strcat(new_proxy_opts, bind_stderr);
+ delete lmgmt->proxy_options;
--- End diff --
You should not mix new/delete and malloc/free here. But this problem goes away with the text buffer.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1073: TS-4399 TS-4400 Management API messes up p...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1073#discussion_r84208742
--- Diff: cmd/traffic_manager/traffic_manager.cc ---
@@ -835,10 +824,11 @@ main(int argc, const char **argv)
} else {
sleep_time = 1;
}
- if (lmgmt->startProxy()) {
+ if (ProxyStateSet(TS_PROXY_ON, TS_CACHE_CLEAR_NONE) == TS_ERR_OKAY) {
just_started = 0;
sleep_time = 0;
} else {
+ mgmt_log("in ProxyStateSet else branch");
--- End diff --
Yes please :)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1073: TS-4399 TS-4400 Management API messes up p...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1073#discussion_r85870603
--- Diff: mgmt/api/CoreAPI.cc ---
@@ -184,21 +184,19 @@ ProxyStateSet(TSProxyStateT state, TSCacheClearT clear)
ink_strlcat(tsArgs, " -k", sizeof(tsArgs));
}
- if (strlen(tsArgs) > 0) { /* Passed command line args for proxy */
- ats_free(lmgmt->proxy_options);
- lmgmt->proxy_options = ats_strdup(tsArgs);
- mgmt_log("[ProxyStateSet] Traffic Server Args: '%s'\n", lmgmt->proxy_options);
- }
+ mgmt_log("[ProxyStateSet] Traffic Server Args: '%s %s'\n", lmgmt->proxy_options ? lmgmt->proxy_options : "", tsArgs);
lmgmt->run_proxy = true;
lmgmt->listenForProxy();
+ lmgmt->startProxy(tsArgs);
--- End diff --
Do you actually need to do the sleeping stuff here? Since you now call ``LocalManager:: startProxy()`` directly, you have the return value to know that it succeeded. I don't think that the contract for this API needs to include waiting for a message.
```C
return lmgmt->startProxy(tsArgs) ? TS_ERR_OKAY : TS_ERR_FAIL;
```
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1073: TS-4399 TS-4400 Management API messes up proxy op...
Posted by danobi <gi...@git.apache.org>.
Github user danobi commented on the issue:
https://github.com/apache/trafficserver/pull/1073
I couldn't figure out where the dup option was so I changed TS-4400 to a subtask of TS-4399.
Also, can you kick off the CI again? I think I fixed the build.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1073: TS-4399 TS-4400 Management API messes up proxy op...
Posted by danobi <gi...@git.apache.org>.
Github user danobi commented on the issue:
https://github.com/apache/trafficserver/pull/1073
I think the delay issue is either:
1) The `mgmt_sleep_sec(1)` loop in `ProxyStateSet` b/c the main `traffic_manager` loop needs to run for the `lmgmt->proxy_running` flag to flip
2) Some kind of quirk with running `lmgmt->listenForProxy()`
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1073: TS-4399 TS-4400 Management API messes up proxy op...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/trafficserver/pull/1073
@danobi Since this covers 2 JIRAs, please designate one of them for the fix, and mark the other as a duplicate.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1073: TS-4399 TS-4400 Management API messes up p...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1073#discussion_r83994912
--- Diff: cmd/traffic_manager/traffic_manager.cc ---
@@ -610,32 +616,37 @@ main(int argc, const char **argv)
RecLocalStart(configFiles);
/* Update cmd line overrides/environmental overrides/etc */
- if (tsArgs) { /* Passed command line args for proxy */
- ats_free(lmgmt->proxy_options);
- lmgmt->proxy_options = tsArgs;
- mgmt_log("[main] Traffic Server Args: '%s'\n", lmgmt->proxy_options);
- }
+ if (tsArgs) /* Passed command line args for proxy */
+ callback_proxy_options.push_back(ats_strdup(tsArgs));
- // we must pass in bind_stdout and bind_stderr values to TS
- // we do it so TS is able to create BaseLogFiles for each value
+ // we must give bind_stdout and bind_stderr values to callback_proxy_options
+ // so LocalManager can start up TS with the correct options
+ //
+ // TS needs them to be able to create BaseLogFiles for each value
if (*bind_stdout != 0) {
- size_t l = strlen(lmgmt->proxy_options);
- size_t n = 3 /* " --" */
- + sizeof(TM_OPT_BIND_STDOUT) /* nul accounted for here */
- + 1 /* space */
- + strlen(bind_stdout);
- lmgmt->proxy_options = static_cast<char *>(ats_realloc(lmgmt->proxy_options, n + l));
- snprintf(lmgmt->proxy_options + l, n, " --%s %s", TM_OPT_BIND_STDOUT, bind_stdout);
+ char *bind_stdout_opt = new char[strlen("--") + strlen(TM_OPT_BIND_STDOUT)];
+ strcpy(bind_stdout_opt, "--");
+ strcat(bind_stdout_opt, TM_OPT_BIND_STDOUT);
+ callback_proxy_options.push_back(bind_stdout_opt);
+ callback_proxy_options.push_back(ats_strdup(bind_stdout));
}
if (*bind_stderr != 0) {
- size_t l = strlen(lmgmt->proxy_options);
- size_t n = 3 /* space dash dash */
- + sizeof(TM_OPT_BIND_STDERR) /* nul accounted for here */
- + 1 /* space */
- + strlen(bind_stderr);
- lmgmt->proxy_options = static_cast<char *>(ats_realloc(lmgmt->proxy_options, n + l));
- snprintf(lmgmt->proxy_options + l, n, " --%s %s", TM_OPT_BIND_STDERR, bind_stderr);
+ char *bind_stderr_opt = new char[strlen("--") + strlen(TM_OPT_BIND_STDERR)];
+ strcpy(bind_stderr_opt, "--");
+ strcat(bind_stderr_opt, TM_OPT_BIND_STDERR);
--- End diff --
```C
foo.push_back("--" TM_OPT_BIND_STDERR")
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1073: TS-4399 TS-4400 Management API messes up proxy op...
Posted by danobi <gi...@git.apache.org>.
Github user danobi commented on the issue:
https://github.com/apache/trafficserver/pull/1073
The actual issue was suspected issue #1, where there was a sleeping deadlock scenario. See commit message for details.
Other than that most recent change, I think the patch is ready for final review.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1073: TS-4399 TS-4400 Management API messes up p...
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1073#discussion_r86604224
--- Diff: mgmt/LocalManager.cc ---
@@ -902,8 +906,13 @@ LocalManager::startProxy()
Vec<char> real_proxy_options;
real_proxy_options.append(proxy_options, strlen(proxy_options));
+ if (onetime_options && *onetime_options) {
+ real_proxy_options.append(" ", strlen(" "));
+ real_proxy_options.append(onetime_options, strlen(onetime_options));
+ }
- if (!strstr(proxy_options, MGMT_OPT)) { // Make sure we're starting the proxy in mgmt mode
+ // Make sure we're starting the proxy in mgmt mode
+ if (strstr(proxy_options, MGMT_OPT) == 0 && strstr(onetime_options, MGMT_OPT) == 0) {
--- End diff --
Couldn't you just check `real_proxy_options` here? If `onetime_options` aren't already in there, they won't be added anyway.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1073: TS-4399 TS-4400 Management API messes up p...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1073#discussion_r85870674
--- Diff: mgmt/LocalManager.cc ---
@@ -902,8 +907,13 @@ LocalManager::startProxy()
Vec<char> real_proxy_options;
real_proxy_options.append(proxy_options, strlen(proxy_options));
+ if (onetime_options && *onetime_options) {
+ real_proxy_options.append(" ", strlen(" "));
+ real_proxy_options.append(onetime_options, strlen(onetime_options));
+ }
- if (!strstr(proxy_options, MGMT_OPT)) { // Make sure we're starting the proxy in mgmt mode
+ // Make sure we're starting the proxy in mgmt mode
+ if (!strstr(proxy_options, MGMT_OPT) && !strstr(onetime_options, MGMT_OPT)) {
--- End diff --
Prefer ``strstr(...) == 0``. It's just that little bit more readable.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1073: TS-4399 TS-4400 Management API messes up p...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1073#discussion_r84208541
--- Diff: mgmt/api/CoreAPI.cc ---
@@ -53,6 +54,9 @@
// global variable
CallbackTable *local_event_callbacks;
+// Used to get any proxy options that traffic_manager might want to tack on
+std::function<void(std::vector<char*>&)> proxy_options_callback;
--- End diff --
Remove 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1073: TS-4399 TS-4400 Management API messes up p...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1073#discussion_r84208555
--- Diff: mgmt/LocalManager.h ---
@@ -74,7 +74,7 @@ class LocalManager : public BaseManager
void signalAlarm(int alarm_id, const char *desc = NULL, const char *ip = NULL);
void processEventQueue();
- bool startProxy();
+ bool startProxy(char *onetime_options);
--- End diff --
``const char *``.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1073: TS-4399 TS-4400 Management API messes up p...
Posted by danobi <gi...@git.apache.org>.
Github user danobi commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1073#discussion_r83509798
--- Diff: cmd/traffic_manager/traffic_manager.cc ---
@@ -835,10 +824,11 @@ main(int argc, const char **argv)
} else {
sleep_time = 1;
}
- if (lmgmt->startProxy()) {
+ if (ProxyStateSet(TS_PROXY_ON, TS_CACHE_CLEAR_NONE) == TS_ERR_OKAY) {
just_started = 0;
sleep_time = 0;
} else {
+ mgmt_log("in ProxyStateSet else branch");
--- End diff --
Note to self: remove this line
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1073: TS-4399 TS-4400 Management API messes up p...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1073#discussion_r84208102
--- Diff: cmd/traffic_manager/traffic_manager.cc ---
@@ -27,6 +27,7 @@
#include "ts/ink_sock.h"
#include "ts/ink_args.h"
#include "ts/ink_syslog.h"
+#include <vector>
--- End diff --
Remove 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1073: TS-4399 TS-4400 Management API messes up proxy op...
Posted by danobi <gi...@git.apache.org>.
Github user danobi commented on the issue:
https://github.com/apache/trafficserver/pull/1073
@jpeach, is this what you were thinking?
Also, between commit 82037 and 3365b, `traffic_ctl` stopped working immediately after ATS starts up. There is ~30s gap between startup any `traffic_ctl` commands working, eg. `sudo ./traffic_ctl server stop`. I'm still looking into 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1073: TS-4399 TS-4400 Management API messes up p...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1073#discussion_r84208323
--- Diff: mgmt/LocalManager.cc ---
@@ -239,8 +239,9 @@ LocalManager::LocalManager(bool proxy_on) : BaseManager(), run_proxy(proxy_on),
process_server_timeout_msecs = REC_readInteger("proxy.config.lm.pserver_timeout_msecs", &found);
proxy_name = REC_readString("proxy.config.proxy_name", &found);
proxy_binary = REC_readString("proxy.config.proxy_binary", &found);
- proxy_options = REC_readString("proxy.config.proxy_binary_opts", &found);
env_prep = REC_readString("proxy.config.env_prep", &found);
+ proxy_options = new char[1];
+ strcpy(proxy_options, ""); // so later we can always `delete proxy_options` without worrying
--- End diff --
You don't need this. It is safe for ``proxy_options`` to be NULL.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1073: TS-4399 TS-4400 Management API messes up proxy op...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1073
FreeBSD build *failed*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1157/ for details.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1073: TS-4399 TS-4400 Management API messes up p...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1073#discussion_r84208632
--- Diff: mgmt/LocalManager.cc ---
@@ -902,8 +907,11 @@ LocalManager::startProxy()
Vec<char> real_proxy_options;
real_proxy_options.append(proxy_options, strlen(proxy_options));
+ real_proxy_options.append(" ", strlen(" "));
+ real_proxy_options.append(onetime_options, strlen(onetime_options));
--- End diff --
Let's allow ``onetime_options`` to be NULL, so
```C
if (onetime_options && *onetime_options) {
...
}
```
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1073: TS-4399 TS-4400 Management API messes up proxy op...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/trafficserver/pull/1073
[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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1073: TS-4399 TS-4400 Management API messes up p...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1073#discussion_r84208107
--- Diff: cmd/traffic_manager/traffic_manager.cc ---
@@ -445,6 +446,8 @@ main(int argc, const char **argv)
int binding_version = 0;
BindingInstance *binding = NULL;
+ std::vector<char*> callback_proxy_options;
--- End diff --
Remove 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1073: TS-4399 TS-4400 Management API messes up p...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1073#discussion_r84208455
--- Diff: mgmt/api/CoreAPI.cc ---
@@ -41,6 +41,7 @@
#include "ts/Diags.h"
#include "ts/ink_hash_table.h"
#include "ExpandingArray.h"
+#include <vector>
--- End diff --
Remove 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1073: TS-4399 TS-4400 Management API messes up p...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach closed the pull request at:
https://github.com/apache/trafficserver/pull/1073
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1073: TS-4399 TS-4400 Management API messes up p...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1073#discussion_r85870712
--- Diff: mgmt/LocalManager.cc ---
@@ -189,6 +189,7 @@ LocalManager::LocalManager(bool proxy_on) : BaseManager(), run_proxy(proxy_on),
proxy_launch_outstanding = false;
mgmt_shutdown_outstanding = MGMT_PENDING_NONE;
proxy_running = 0;
+ coreapi_sleep = true;
--- End diff --
As mentioned in the other comment, I think we can get away without 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1073: TS-4399 TS-4400 Management API messes up p...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1073#discussion_r84208189
--- Diff: cmd/traffic_manager/traffic_manager.cc ---
@@ -611,31 +614,35 @@ main(int argc, const char **argv)
/* Update cmd line overrides/environmental overrides/etc */
if (tsArgs) { /* Passed command line args for proxy */
+ char* new_proxy_opts = new char[strlen(lmgmt->proxy_options) + strlen(tsArgs) + 1];
+ strcpy(new_proxy_opts, lmgmt->proxy_options);
+ strcat(new_proxy_opts, tsArgs);
ats_free(lmgmt->proxy_options);
- lmgmt->proxy_options = tsArgs;
- mgmt_log("[main] Traffic Server Args: '%s'\n", lmgmt->proxy_options);
+ lmgmt->proxy_options = new_proxy_opts;
--- End diff --
At this point ``lmgmt->proxy_options`` isn't set, so just copy ``tsArgs`` into the text buffer if it is present.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1073: TS-4399 TS-4400 Management API messes up proxy op...
Posted by danobi <gi...@git.apache.org>.
Github user danobi commented on the issue:
https://github.com/apache/trafficserver/pull/1073
How does that look? Still need to figure out that delay issue.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1073: TS-4399 TS-4400 Management API messes up p...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1073#discussion_r84208247
--- Diff: cmd/traffic_manager/traffic_manager.cc ---
@@ -844,6 +852,11 @@ main(int argc, const char **argv)
}
}
+ // Delete the proxy options we saved for CoreAPI.
+ // This probably isn't needed b/c the process dies here...
+ for (auto it = callback_proxy_options.begin(); it < callback_proxy_options.end(); ++it)
+ free(*it);
+
--- End diff --
Remove 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1073: TS-4399 TS-4400 Management API messes up p...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1073#discussion_r84208118
--- Diff: cmd/traffic_manager/traffic_manager.cc ---
@@ -611,31 +614,35 @@ main(int argc, const char **argv)
/* Update cmd line overrides/environmental overrides/etc */
if (tsArgs) { /* Passed command line args for proxy */
+ char* new_proxy_opts = new char[strlen(lmgmt->proxy_options) + strlen(tsArgs) + 1];
+ strcpy(new_proxy_opts, lmgmt->proxy_options);
+ strcat(new_proxy_opts, tsArgs);
ats_free(lmgmt->proxy_options);
- lmgmt->proxy_options = tsArgs;
- mgmt_log("[main] Traffic Server Args: '%s'\n", lmgmt->proxy_options);
+ lmgmt->proxy_options = new_proxy_opts;
}
- // we must pass in bind_stdout and bind_stderr values to TS
- // we do it so TS is able to create BaseLogFiles for each value
+ // TS needs to be started up with the same outputlog bindings each time,
+ // so we append the outputlog location to the persistent proxy options
+ //
+ // TS needs them to be able to create BaseLogFiles for each value
if (*bind_stdout != 0) {
- size_t l = strlen(lmgmt->proxy_options);
- size_t n = 3 /* " --" */
- + sizeof(TM_OPT_BIND_STDOUT) /* nul accounted for here */
- + 1 /* space */
- + strlen(bind_stdout);
- lmgmt->proxy_options = static_cast<char *>(ats_realloc(lmgmt->proxy_options, n + l));
- snprintf(lmgmt->proxy_options + l, n, " --%s %s", TM_OPT_BIND_STDOUT, bind_stdout);
+ const char *bind_stdout_opt = " --" TM_OPT_BIND_STDOUT " ";
+ char *new_proxy_opts = new char[strlen(lmgmt->proxy_options) + strlen(bind_stdout_opt) + strlen(bind_stdout) + 1];
+ strcpy(new_proxy_opts, lmgmt->proxy_options);
+ strcat(new_proxy_opts, bind_stdout_opt);
+ strcat(new_proxy_opts, bind_stdout);
+ delete lmgmt->proxy_options;
--- End diff --
You can replace all this string code with ``TextBuffer``.
```C
TextBuffer args;
if (*bind_stdout) {
const char * space = args.empty() ? "" : " ";
args.format("%s%s", space, "--" TM_OPT_BIND_STDOUT);
}
...
lmgmt->proxy_options = args.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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1073: TS-4399 TS-4400 Management API messes up p...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1073#discussion_r84208567
--- Diff: mgmt/LocalManager.cc ---
@@ -831,9 +832,13 @@ LocalManager::processEventQueue()
/*
* startProxy()
* Function fires up a proxy process.
+ *
+ * Args:
+ * onetime_options: one time options that traffic_server should be started with (ie
+ * these options do not persist across reboots)
*/
bool
-LocalManager::startProxy()
+LocalManager::startProxy(char *onetime_options)
--- End diff --
``const char *``
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1073: TS-4399 TS-4400 Management API messes up proxy op...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1073
Linux build *failed*! See https://ci.trafficserver.apache.org/job/Github-Linux/1050/ for details.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---