Re: [apache/trafficserver] remove bad check for configuration directory override (kill it with fire) (#2090)

[James Peach <jp...@apache.org>]

> On Jun 7, 2017, at 6:15 PM, dragon512 <no...@github.com> wrote:
> 
> @jpeach I don't know about the -D options. I did not test to see if -D works or not ( I would assumed you guy had tested it already).

This patch enables the -D option in the generic option parsing, so I assumed that you needed it :)

> The code removed here is preventing a custom directory for sysconfdir() to be used. (ie the test would fail as trafficserver would exit with message assuming we are using some -D option.

This was added by Sudheer for Yahoo for the "-C config_verify” option. AFAIK no-one except Yahoo uses this, and there are no tests.

> Removing this allowed some testing work to run correctly.

OK so it sounds like it makes something work for you? In general I’m not that confident that setting the config directory is fully working. It worked enough for what Yahoo needed at the time, but it is hard to say more than that.

> At the moment we are still making tweaks to get AuTest to work with layouts that have absolute paths ( ie they ignore TS_ROOT) so they can work correct with the testing system. A number of companies have complied about this issue. This is one step in few PR yet to come to correct this issue. At the moment an intern is looking in to traffic_ctrl and what we need to fix to get this to use the correct paths.

The only case that I’m aware of is that there is no convenient way to tell traffic_ctl which socket to use if you have a custom runtie layout. That doesn’t seem hard to fix, though deciding on the right way to specify the path deserves some though.

> I can see about getting some "test" to validate the -D option works, and if we all agree this is a supported option, see about documenting it

AFAICT this option does nothing, so I don’t understand what started working when you removed it.

Re: [apache/trafficserver] remove bad check for configuration directory override (kill it with fire) (#2090)

[James Peach <jp...@apache.org>]

On 8 Jun 2017, at 10:31, Jason Kenny wrote:

> James,
> to help clarify what we are doing. There currently is an issue in 
> which autest cannot correct test certain builds of ATS. The issue 
> comes from the need to point to a build and copy/hard link bits in to 
> a sandbox directory to run the test in a controlled environment. Todo 
> this we use TS_ROOT and information from traffic_layout to setup 
> directories in the sandbox so when we run the 
> traffic_server/manger/top it will look in the sandbox area for 
> information such as the config files, or as the place to output log 
> files. The problem that has been found at Yahoo/Apple/some other 
> companies is that certain layouts use absolute path ( Honestly some of 
> them use messed up paths...) So for example the layout opt has this 
> layout structure:...mandir:        ${prefix}/share/man
> sysconfdir:    /etc${prefix}
> ...
> I would argue that sysconfdir should be ${prefix}/etc, however that is 
> not the point here.

The reason config.layout exists is because no-one can agree on the same 
locations for everything :)

> The issue is that cases like this prevent us from testing ATS 
> correctly as we point to locations outside the sandbox. GIven the 
> TS_ROOT as it is design is broken in certain cases, the fix for this 
> is to change autest to set exact paths for the path like sysconfdir. 
> This prevents the problems from happening and should allow custom 
> layouts to works. The code removed was some code ( I have no idea why 
> it was there, and from your response seems to be dead code as is) that 
> prevented us from modifying the sysconfdir, as it assumed we should be 
> using it only with some feature/command line option.

The code that was removed was checking that the -D option was not given 
unless the "-C config_verify” was also given. If you look at the way 
the -D option is handled, it sets the “conf_dir” value which is 
*only* used in cmd_verify() in 
“Layout::get()->update_sysconfdir(conf_dir)”. It doesn’t do 
anything at all in the absence of the "-C config_verify” option.

This is why I asked about the removal, because removing the check does 
not fix any of the problems you describe AFAICT.  So I wonder why the 
code was removed, and what problem that removal fixed.

To change the config directory, you can set the PROXY_CONFIG_CONFIG_DIR 
environment variable, which is supported and ought to work in all cases.

> At the moment I have an intern making these changes in AuTest 
> extensions to get it work with the current trafficserver. The only 
> code changes I will try to push are item that are utterly broken from 
> a testability point of view.
> GIven we get this working the second phase I would like to do is make 
> code changes to add some sort of TS_RUN_DIR value or something to 
> allow us to define layout as a run time value.

Experiment with traffic_layout a bit. You can alter most things in the 
layout today.

For example:

$ PROXY_CONFIG_CONFIG_DIR=/one/etc PROXY_CONFIG_LOCAL_STATE_DIR=/two/run 
PROXY_CONFIG_LOG_LOGFILE_DIR=/three/logs /opt/ats/bin/traffic_layout
PREFIX: /opt/ats
BINDIR: /opt/ats/bin
SYSCONFDIR: /one/etc
LIBDIR: /opt/ats/lib
LOGDIR: /three/logs
RUNTIMEDIR: /two/run
PLUGINDIR: /opt/ats/libexec/trafficserver
INCLUDEDIR: /opt/ats/include
SNAPSHOTDIR: /one/etc/snapshots
records.config: /one/etc/records.config
remap.config: /one/etc/remap.config
plugin.config: /one/etc/plugin.config
ssl_multicert.config: /one/etc/ssl_multicert.config
storage.config: /one/etc/storage.config
hosting.config: /one/etc/hosting.config
volume.config: /one/etc/volume.config
ip_allow.config: /one/etc/ip_allow.config

As you correctly note, traffic_ctl probably should learn how to deal 
with a non-default PROXY_CONFIG_LOCAL_STATE_DIR
   
> General idea here is that we could say something like:
> :> traffic_server init ./
> which would make a copy of the config file and state being used. Ideas 
> of having options like --full --data/partial  to control how much to 
> copy or options like --layout to copy data in a different layout 
> structure are --default or --clean to get a clean set of 
> configurations files, etc... are idea that might be useful to have as 
> wellWhen we run trafficserver from the location it would use the 
> layout data. there might be an option we can add to tell 
> traffic_server to use the data in that location ( instead of the 
> default location)
> This would greatly help testing, and would be nice for ops as one can 
> copy the state, make changes and see if an issue or feature works 
> without messing up the default setup by mistake. I am sure other use 
> case could be define to benefit form this as well. However I am not at 
> the point to propose a formal design for this.. but if you have ideas 
> about it.. would love to hear it
> anyways I hope this clarifies the change and the motivation behind it. 
> Also as far as I know we don't use the -C option as well.. but maybe 
> Evil Dave knows if it is used at all. It does sound like a useful 
> item.
> Jason
>
> On ‎Wednesday‎, ‎June‎ ‎7‎, ‎2017‎ 
> ‎10‎:‎18‎:‎29‎ ‎PM‎ ‎CDT, James Peach 
> <jp...@apache.org> wrote:
>
>
>> On Jun 7, 2017, at 6:15 PM, dragon512 <no...@github.com> 
>> wrote:
>>
>> @jpeach I don't know about the -D options. I did not test to see if 
>> -D works or not ( I would assumed you guy had tested it already).
>
> This patch enables the -D option in the generic option parsing, so I 
> assumed that you needed it :)
>
>> The code removed here is preventing a custom directory for 
>> sysconfdir() to be used. (ie the test would fail as trafficserver 
>> would exit with message assuming we are using some -D option.
>
> This was added by Sudheer for Yahoo for the "-C config_verify” 
> option. AFAIK no-one except Yahoo uses this, and there are no tests.
>
>> Removing this allowed some testing work to run correctly.
>
> OK so it sounds like it makes something work for you? In general I’m 
> not that confident that setting the config directory is fully working. 
> It worked enough for what Yahoo needed at the time, but it is hard to 
> say more than that.
>
>> At the moment we are still making tweaks to get AuTest to work with 
>> layouts that have absolute paths ( ie they ignore TS_ROOT) so they 
>> can work correct with the testing system. A number of companies have 
>> complied about this issue. This is one step in few PR yet to come to 
>> correct this issue. At the moment an intern is looking in to 
>> traffic_ctrl and what we need to fix to get this to use the correct 
>> paths.
>
> The only case that I’m aware of is that there is no convenient way 
> to tell traffic_ctl which socket to use if you have a custom runtie 
> layout. That doesn’t seem hard to fix, though deciding on the right 
> way to specify the path deserves some though.
>
>> I can see about getting some "test" to validate the -D option works, 
>> and if we all agree this is a supported option, see about documenting 
>> it
>
> AFAICT this option does nothing, so I don’t understand what started 
> working when you removed it.

Re: [apache/trafficserver] remove bad check for configuration directory override (kill it with fire) (#2090)

[Jason Kenny <jk...@yahoo-inc.com.INVALID>]

James,
to help clarify what we are doing. There currently is an issue in which autest cannot correct test certain builds of ATS. The issue comes from the need to point to a build and copy/hard link bits in to a sandbox directory to run the test in a controlled environment. Todo this we use TS_ROOT and information from traffic_layout to setup directories in the sandbox so when we run the traffic_server/manger/top it will look in the sandbox area for information such as the config files, or as the place to output log files. The problem that has been found at Yahoo/Apple/some other companies is that certain layouts use absolute path ( Honestly some of them use messed up paths...) So for example the layout opt has this layout structure:...mandir:        ${prefix}/share/man
sysconfdir:    /etc${prefix}
...
I would argue that sysconfdir should be ${prefix}/etc, however that is not the point here. The issue is that cases like this prevent us from testing ATS correctly as we point to locations outside the sandbox. GIven the TS_ROOT as it is design is broken in certain cases, the fix for this is to change autest to set exact paths for the path like sysconfdir. This prevents the problems from happening and should allow custom layouts to works. The code removed was some code ( I have no idea why it was there, and from your response seems to be dead code as is) that prevented us from modifying the sysconfdir, as it assumed we should be using it only with some feature/command line option.
At the moment I have an intern making these changes in AuTest extensions to get it work with the current trafficserver. The only code changes I will try to push are item that are utterly broken from a testability point of view. GIven we get this working the second phase I would like to do is make code changes to add some sort of TS_RUN_DIR value or something to allow us to define layout as a run time value. 
General idea here is that we could say something like:
:> traffic_server init ./
which would make a copy of the config file and state being used. Ideas of having options like --full --data/partial  to control how much to copy or options like --layout to copy data in a different layout structure are --default or --clean to get a clean set of configurations files, etc... are idea that might be useful to have as wellWhen we run trafficserver from the location it would use the layout data. there might be an option we can add to tell traffic_server to use the data in that location ( instead of the default location)
This would greatly help testing, and would be nice for ops as one can copy the state, make changes and see if an issue or feature works without messing up the default setup by mistake. I am sure other use case could be define to benefit form this as well. However I am not at the point to propose a formal design for this.. but if you have ideas about it.. would love to hear it
anyways I hope this clarifies the change and the motivation behind it. Also as far as I know we don't use the -C option as well.. but maybe Evil Dave knows if it is used at all. It does sound like a useful item.
Jason

On ‎Wednesday‎, ‎June‎ ‎7‎, ‎2017‎ ‎10‎:‎18‎:‎29‎ ‎PM‎ ‎CDT, James Peach <jp...@apache.org> wrote:


> On Jun 7, 2017, at 6:15 PM, dragon512 <no...@github.com> wrote:
> 
> @jpeach I don't know about the -D options. I did not test to see if -D works or not ( I would assumed you guy had tested it already).

This patch enables the -D option in the generic option parsing, so I assumed that you needed it :)

> The code removed here is preventing a custom directory for sysconfdir() to be used. (ie the test would fail as trafficserver would exit with message assuming we are using some -D option.

This was added by Sudheer for Yahoo for the "-C config_verify” option. AFAIK no-one except Yahoo uses this, and there are no tests.

> Removing this allowed some testing work to run correctly.

OK so it sounds like it makes something work for you? In general I’m not that confident that setting the config directory is fully working. It worked enough for what Yahoo needed at the time, but it is hard to say more than that.

> At the moment we are still making tweaks to get AuTest to work with layouts that have absolute paths ( ie they ignore TS_ROOT) so they can work correct with the testing system. A number of companies have complied about this issue. This is one step in few PR yet to come to correct this issue. At the moment an intern is looking in to traffic_ctrl and what we need to fix to get this to use the correct paths.

The only case that I’m aware of is that there is no convenient way to tell traffic_ctl which socket to use if you have a custom runtie layout. That doesn’t seem hard to fix, though deciding on the right way to specify the path deserves some though.

> I can see about getting some "test" to validate the -D option works, and if we all agree this is a supported option, see about documenting it

AFAICT this option does nothing, so I don’t understand what started working when you removed it.