TS-4399 TS-4400 Management API messes up proxy options by danobi · Pull Request #1073 · apache/trafficserver

danobi · 2016-10-03T20:18:42Z

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.

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.

jpeach

I don't think that it is a good idea to have the management API contain an upward dependency on traffic_manager.

One of the problems here is that the introduction of the --bind_std* options changed the way argument passing works without fully propagating the change. I think that a better approach to explore is making traffic_manager call ProxyStateSet. If we can do that then we only have 1 copy of this code (DRY) and the correct dependencies.

This patch centralizes where traffic_server starts to inside CoreAPI's ProxyStateSet. This is good because we reduce the number of places we deal with traffic_server options. Everything is simply handled by the proxy_options_callback. Note: Right now, there is about a ~30 delay between ATS starting up and traffic_ctl commands working. Not sure why.

danobi · 2016-10-14T22:02:29Z

@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 and any traffic_ctl commands working, eg. sudo ./traffic_ctl server stop. I'm still looking into this.

danobi · 2016-10-14T22:07:22Z

cmd/traffic_manager/traffic_manager.cc

        just_started = 0;
        sleep_time   = 0;
      } else {
+        mgmt_log("in ProxyStateSet else branch");


Note to self: remove this line

Yes please :)

jpeach

This is moving in the right direction, but still adds more complexity than it removes.

Proxy options can come from

traffic_manager -tsArgs (explicit)
proxy.config.proxy_binary_opts (explicit)
traffic_manager --bind_foo (implicit)

The goal is to have a single API ProxyStateSet that does the right thing in exactly 1 place (DRY principle). Using ProxyStateSet from traffic_manager is an improvement. Note that all we really need to persist across restarts are (1) and (3). We already have a LocalManager object that can store the data that we need.

So we can keep LocalManager::proxy_options as the persistent proxy options, and have ProxyStateSet generate any additional options needed for this particular launch. The per-launch options from the API flags and from (2) can be passed directly into LocalManager::startProxy(), which can concatenate them (you just need a Vec<char*> here). This won't require any new global data.

jpeach · 2016-10-19T04:12:33Z

cmd/traffic_manager/traffic_manager.cc

-    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);


foo.push_back("--" TM_OPT_BIND_STDERR")

Instead, use proxy_options as persistent proxy options storage. Then have lmgmt->startProxy() take in an argument for one-time flags. This was done to reduce complicated dependencies.

danobi · 2016-10-19T21:14:05Z

How does that look? Still need to figure out that delay issue.

jpeach

This is looking pretty good.

jpeach · 2016-10-20T03:33:36Z

cmd/traffic_manager/traffic_manager.cc

 #include "ts/ink_sock.h"
 #include "ts/ink_args.h"
 #include "ts/ink_syslog.h"
+#include <vector>


Remove this.

jpeach · 2016-10-20T03:33:43Z

cmd/traffic_manager/traffic_manager.cc

  int binding_version      = 0;
  BindingInstance *binding = NULL;

+  std::vector<char*> callback_proxy_options;


Remove this.

jpeach · 2016-10-20T03:33:53Z

cmd/traffic_manager/traffic_manager.cc

+    strcpy(new_proxy_opts, lmgmt->proxy_options);
+    strcat(new_proxy_opts, bind_stdout_opt);
+    strcat(new_proxy_opts, bind_stdout);
+    delete lmgmt->proxy_options;


You can replace all this string code with TextBuffer.

TextBuffer args; if (*bind_stdout) { const char * space = args.empty() ? "" : " "; args.format("%s%s", space, "--" TM_OPT_BIND_STDOUT); } ... lmgmt->proxy_options = args.release();

jpeach · 2016-10-20T03:35:23Z

cmd/traffic_manager/traffic_manager.cc

    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;


At this point lmgmt->proxy_options isn't set, so just copy tsArgs into the text buffer if it is present.

jpeach · 2016-10-20T03:36:06Z

cmd/traffic_manager/traffic_manager.cc

+    strcpy(new_proxy_opts, lmgmt->proxy_options);
+    strcat(new_proxy_opts, bind_stderr_opt);
+    strcat(new_proxy_opts, bind_stderr);
+    delete lmgmt->proxy_options;


You should not mix new/delete and malloc/free here. But this problem goes away with the text buffer.

jpeach · 2016-10-20T03:41:22Z

mgmt/api/CoreAPI.cc

 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;


Remove this.

jpeach · 2016-10-20T03:41:38Z

mgmt/LocalManager.h


  void processEventQueue();
-  bool startProxy();
+  bool startProxy(char *onetime_options);


const char *.

jpeach · 2016-10-20T03:41:46Z

mgmt/LocalManager.cc

 */
 bool
-LocalManager::startProxy()
+LocalManager::startProxy(char *onetime_options)


const char *

jpeach · 2016-10-20T03:42:47Z

mgmt/LocalManager.cc


    real_proxy_options.append(proxy_options, strlen(proxy_options));
+    real_proxy_options.append(" ", strlen(" "));
+    real_proxy_options.append(onetime_options, strlen(onetime_options));


Let's allow onetime_options to be NULL, so

if (onetime_options && *onetime_options) { ... }

jpeach · 2016-10-20T03:44:46Z

cmd/traffic_manager/traffic_manager.cc

        just_started = 0;
        sleep_time   = 0;
      } else {
+        mgmt_log("in ProxyStateSet else branch");


Yes please :)

danobi · 2016-10-28T22:04:02Z

I think the delay issue is either:

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
Some kind of quirk with running lmgmt->listenForProxy()

This change was made because the ProxyStateSet() call was sleeping in the same thread as traffic_manager was running, so the condition in the do/while loop could never be updated by traffic_manager. The solution is to differentiate API calls and local calls to CoreAPI by using the coreapi_sleep flag.

danobi · 2016-10-31T16:38:42Z

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.

jpeach · 2016-11-01T03:22:57Z

mgmt/api/CoreAPI.cc


    lmgmt->run_proxy = true;
    lmgmt->listenForProxy();
+    lmgmt->startProxy(tsArgs);


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.

return lmgmt->startProxy(tsArgs) ? TS_ERR_OKAY : TS_ERR_FAIL;

That's a good observation, I'm kicking myself for not seeing it before.

jpeach · 2016-11-01T03:24:51Z

mgmt/LocalManager.cc


-    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)) {


Prefer strstr(...) == 0. It's just that little bit more readable.

jpeach · 2016-11-01T03:26:09Z

mgmt/LocalManager.cc

  proxy_launch_outstanding  = false;
  mgmt_shutdown_outstanding = MGMT_PENDING_NONE;
  proxy_running             = 0;
+  coreapi_sleep             = true;


As mentioned in the other comment, I think we can get away without this.

We no longer need to sleep inside ProxyStateSet because we're directly calling lmgmt->startProxy instead of setting the run_proxy flag and waiting for the TM thread to kick off the lmgmt->startProxy call.

jpeach · 2016-11-03T04:50:45Z

[approve ci]

atsci · 2016-11-03T05:04:32Z

FreeBSD build failed! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1157/ for details.

atsci · 2016-11-03T05:07:37Z

Linux build failed! See https://ci.trafficserver.apache.org/job/Github-Linux/1050/ for details.

jpeach · 2016-11-04T05:12:58Z

@danobi Since this covers 2 JIRAs, please designate one of them for the fix, and mark the other as a duplicate.

danobi · 2016-11-04T16:38:09Z

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.

SolidWallOfCode · 2016-11-04T18:16:41Z

mgmt/LocalManager.cc


-    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) {


Couldn't you just check real_proxy_options here? If onetime_options aren't already in there, they won't be added anyway.

real_proxy_options is the mysterious Vec class. AFAICT, I suppose we could, but it would require more allocation.

danobi added 2 commits October 3, 2016 14:30

Fix memory leaks

3365b20

zwoop added the Manager label Oct 4, 2016

zwoop added this to the 7.1.0 milestone Oct 4, 2016

zwoop assigned jpeach Oct 4, 2016

jpeach requested changes Oct 5, 2016

View reviewed changes

danobi commented Oct 14, 2016

View reviewed changes

jpeach requested changes Oct 19, 2016

View reviewed changes

Remove callback between traffic_manager and LocalManager

af59ccb

Instead, use proxy_options as persistent proxy options storage. Then have lmgmt->startProxy() take in an argument for one-time flags. This was done to reduce complicated dependencies.

jpeach requested changes Oct 20, 2016

View reviewed changes

Code cleanup

79b2b70

danobi added 3 commits October 31, 2016 11:33

clang-format

0fe5db3

Code cleanup

10c10af

jpeach requested changes Nov 1, 2016

View reviewed changes

danobi added 2 commits November 2, 2016 14:14

Remove ProxyStateSet sleeping

5a989cb

We no longer need to sleep inside ProxyStateSet because we're directly calling lmgmt->startProxy instead of setting the run_proxy flag and waiting for the TM thread to kick off the lmgmt->startProxy call.

Code formatting cleanup

3619a85

Fix build warning

9e09c30

SolidWallOfCode reviewed Nov 4, 2016

View reviewed changes

jpeach closed this in eac31a4 Nov 18, 2016

zwoop modified the milestone: 7.1.0 May 4, 2017

zwoop unassigned jpeach May 4, 2017

bneradt pushed a commit to bneradt/trafficserver that referenced this pull request Jan 23, 2026

Merge ASF 10.1.x into Edge 10.0.x apache#1073

ee01073

Conversation

danobi commented Oct 3, 2016

Uh oh!

jpeach left a comment

Choose a reason for hiding this comment

Uh oh!

danobi commented Oct 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpeach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danobi commented Oct 19, 2016

Uh oh!

jpeach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danobi commented Oct 28, 2016

Uh oh!

danobi commented Oct 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danobi Nov 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpeach commented Nov 3, 2016

Uh oh!

atsci commented Nov 3, 2016

Uh oh!

atsci commented Nov 3, 2016

Uh oh!

jpeach commented Nov 4, 2016

Uh oh!

danobi commented Nov 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

danobi commented Oct 14, 2016 •

edited

Loading

danobi Nov 2, 2016 •

edited

Loading