TS-4399: 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 above 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.

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.

We use proxy_options as persistent proxy options storage.  Then
have lmgmt->startProxy() take in an argument for one-time flags.

This closes #1073.

Author: danobi

cmd/traffic_manager/traffic_manager.cc

@@ -37,6 +37,7 @@
 #include "FileManager.h"
 #include "ts/I_Layout.h"
 #include "ts/I_Version.h"
+#include "ts/TextBuffer.h"
 #include "DiagsConfig.h"
 #include "HTTP.h"
 #include "CoreAPI.h"
@@ -610,33 +611,27 @@ main(int argc, const char **argv)
 
   /* 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);
+    lmgmt->proxy_options = 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
-  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);
+  // 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
+  textBuffer args(1024);
+
+  if (*bind_stdout) {
+    const char *space = args.empty() ? "" : " ";
+    args.format("%s%s %s", space, "--" TM_OPT_BIND_STDOUT, 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);
+  if (*bind_stderr) {
+    const char *space = args.empty() ? "" : " ";
+    args.format("%s%s %s", space, "--" TM_OPT_BIND_STDERR, bind_stderr);
   }
 
+  lmgmt->proxy_options = args.release();
+
   if (proxy_port) {
     HttpProxyPort::loadValue(lmgmt->m_proxy_ports, proxy_port);
   }
@@ -812,7 +807,7 @@ 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/LocalManager.cc

@@ -241,8 +241,8 @@ 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                = NULL;
 
   // Calculate proxy_binary from the absolute bin_path
   absolute_proxy_binary = Layout::relative_to(bindir, proxy_binary);
@@ -853,9 +853,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(const char *onetime_options)
 {
   if (proxy_launch_outstanding) {
     return false;
@@ -924,8 +928,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) {
       real_proxy_options.append(" ", strlen(" "));
       real_proxy_options.append(MGMT_OPT, sizeof(MGMT_OPT) - 1);
     }

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(const char *onetime_options);
   void listenForProxy();
   void bindProxyPort(HttpProxyPort &);
   void closeProxyPorts();
@@ -108,7 +108,7 @@ class LocalManager : public BaseManager
   char *absolute_proxy_binary;
   char *proxy_name;
   char *proxy_binary;
-  char *proxy_options;
+  char *proxy_options; // These options should persist across proxy reboots
   char *env_prep;
 
   int process_server_sockfd;

mgmt/api/CoreAPI.cc

@@ -153,7 +153,6 @@ ProxyStateGet()
 TSMgmtError
 ProxyStateSet(TSProxyStateT state, TSCacheClearT clear)
 {
-  int i = 0;
   char tsArgs[MAX_BUF_SIZE];
   char *proxy_options;
 
@@ -184,24 +183,16 @@ 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();
-
-    do {
-      mgmt_sleep_sec(1);
-    } while (i++ < 20 && (lmgmt->proxy_running == 0));
-
-    if (!lmgmt->processRunning()) {
+    if (!lmgmt->startProxy(tsArgs)) {
       goto Lerror;
     }
 
     break;
+
   default:
     goto Lerror;
   }