register stats in traffic_server if running in standalone mode

[zizhong]

Without this the
RecSetRecordInt("proxy.node.config.draining", 1, REC_SOURCE_DEFAULT); on Main.cc:288 will fail,
because

    // Add the record but do not set the 'registered' flag, as this
    // record really hasn't been registered yet.  Also, in order to
    // add the record, we need to have a rec_type, so if the user
    // calls RecSetRecord on a record we haven't registered yet, we
    // should fail out here.
    if ((rec_type == RECT_NULL) || (data_type == RECD_NULL)) {
      err = REC_ERR_FAIL;
      goto Ldone;
    }

from P_RecCore.cc:422.

This bug is breaking the drain feature and it's needed for #2918.

[zwoop]

I don't think this is the right way to do this honestly. I think we should register this setting where this feature gets initialized. Yes, there are some proxy.node metrics in RecordsConfig.cc already, but that really wasn't the way this was supposed to be done afaik.

[zwoop]

Also, we have this:

cmd/traffic_manager/traffic_manager.cc:  RecRegisterStatInt(RECT_NODE, "proxy.node.config.draining", 0, RECP_NON_PERSISTENT);

So if this is not sufficient, we should fix the above. Or at a minimum the above should be removed (we shouldn't register the metric twice).

[zwoop]

@maskit something odd is going on here, can you take a look please?

[maskit]

The metric was originally just a variable but the variable was replaced with the setting on #2962, which means it worked without traffic_manager but now it requires traffic_manager (traffic_manager has to register the setting first on startup).

Can we register all the 6 settings on traffic_server side, or do these need to be registered on traffic_manager side? Only moving the one for draining doesn't make sense.

Also, we should be able to remove "proxy.node.config.restart_required.cop", right?

[zwoop]

[approve ci autest]

[zizhong]

@zwoop @maskit Thanks for the insightful comments.
This is to fix the autest failure since autest only runs traffic_server and this stat is registered in traffic_manager. I feel the latest change makes much more sense. Can you guys take another look? Thank you!

[zizhong]

[approve ci autest]

[zizhong]

     - < Proxy-Connection: keep-alive
           ? ^                   ^    ^
           
     + > Proxy-Connection: Keep-Alive
           ? ^                   ^    ^

Saw this causes autest failed. @zwoop @dragon512 any insights?

[maskit]

I'm OK with this. However, I'd rather register all the metrics consistently on traffic_server side on startup if traffic_server wasn't managed by traffic_manager.

I'm not sure why autest is failing but the diff is really confusing. It looks like 'keep-alive' vs 'Keep-Alive' but they are a request header and a response header.

[zizhong]

@maskit I looked into other stats registered in traffic_manager. They can only be changed via traffic_ctl and traffic_manager. Wouldn't make much sense to register them in traffic_server. This stat is different. It can be also changed by the signal.

[maskit]

@zizhong

They can only be changed via traffic_ctl and traffic_manager.

Right, that's true as far as I know.

Wouldn't make much sense to register them in traffic_server.

I think differently. I see the stats as interfaces of traffic_server for other processes that manages traffic_server. If we registered them in traffic_server, we would be able to manage traffic_server with different tools, such as unit tests and homemade management tools.

Also, this is probably the only point that traffic_server depends on traffic_manager. So if we could make traffic_server their owner the relationship between traffic_server and traffic_manager would be a bit simple, though I don't think it has to be done on this PR because it wouldn't be a small change and I'm not sure whether this is really possible.

[maskit]

@SolidWallOfCode @vmamidi I heard you guys are working on around traffic_manager and RPC. What do you think about this from your point of view?

[SolidWallOfCode]

I agree with @maskit. I think it's a good idea to make traffic_server as self contained as possible. If we get the RPC fixed up to some standard mechanism we can expect the subsequent creation of other management tools that work directly with traffic_server and use only that executable. I am in favor of moving in that direction.

[zizhong]

@SolidWallOfCode @maskit Thanks for the suggestions! I'll make the change accordingly.

[zizhong]

@maskit @zwoop PR updated. can you help to take another look? Thanks!
Once this is merged, the failed test case of #2918 will pass.

[maskit]

Looks good to me.

We could remove them from traffic_manager but I don't think that need to be done on this PR. This fix is the minimal and simplest fix for the draining issue.

fixed