Re: [RFC PATCH] trace2: don't overload target directories

> On 7/29/2019 6:20 PM, Josh Steadmon wrote:
> > trace2 can write files into a target directory. With heavy usage, this
> > directory can fill up with files, causing difficulty for
> > trace-processing systems.
> > 
> > When trace2 would write a file to a target directory, first check
> > whether or not the directory is overloaded. A directory is overloaded if
> > there is a sentinel file declaring an overload, or if the number of
> > files exceeds a threshold. If the latter, create a sentinel file to
> > speed up later overload checks.
> > 
> > The file count threshold is currently set to 1M files, but this can be
> > overridden for testing with GIT_TRACE2_TEST_OVERLOAD_FILE_COUNT.
> 
> 1 million seems like a LOT, and the environment variable seems to be only
> for testing.
> 
> * If the variable is only for testing, then it should start with GIT_TEST_
> 
> * Are we sure 1 million is the right number? I would imagine even 10,000
>   starting to be a problem. How would a user adjust this value if they
>   are having problems before 1,000,000?

> > The assumption is that a separate trace-processing system is dealing
> > with the generated traces; once it processes and removes the sentinel
> > file, it should be safe to generate new trace files again.
> 
> This matches the model that you (Google) are using for collecting logs.
> I'll trust your expertise here in how backed up these logs become. I
> imagine that someone working without a network connection for a long
> time would be likely to run into this problem.
>
> [snip]
> 
> > +test_expect_success "don't overload target directory" '
> > +	GIT_TRACE2_TEST_OVERLOAD_FILE_COUNT=100 &&
> 
> For testing, does this need to be 100? Could it be 5?

> > +	test_when_finished "rm -r trace_target_dir" &&
> > +	mkdir trace_target_dir &&
> > +	test_seq $GIT_TRACE2_TEST_OVERLOAD_FILE_COUNT | sed "s#^#trace_target_dir/#" | sort > expected_filenames.txt &&
> > +	xargs touch < expected_filenames.txt &&
> 
> nit: no space between redirection and filename.

> > +	ls trace_target_dir | sed "s#^#trace_target_dir/#" > first_ls_output.txt &&
> > +	test_cmp expected_filenames.txt first_ls_output.txt &&
> > +	GIT_TRACE2="$(pwd)/trace_target_dir" test-tool trace2 001return 0 &&
> > +	echo "trace_target_dir/git-trace2-overload" >> expected_filenames.txt &&
> > +	ls trace_target_dir | sed "s#^#trace_target_dir/#" > second_ls_output.txt &&
> > +	test_cmp expected_filenames.txt second_ls_output.txt
> > +'
> 
> [snip]
> 
> > +/*
> > + * Check to make sure we're not overloading the target directory with too many
> > + * files. First check for the presence of a sentinel file, then check file
> > + * count. If we are overloaded, create the sentinel file if it doesn't already
> > + * exist.
> > + *
> > + * We expect that some trace processing system is gradually collecting files
> > + * from the target directory; after it removes the sentinel file we'll start
> > + * writing traces again.
> > + */
> > +static int tr2_dst_overloaded(const char *tgt_prefix)
> > +{
> > +	int file_count = 0, overload_file_count = 0;
> > +	char *test_threshold_val;
> > +	DIR *dirp;
> > +	struct strbuf path = STRBUF_INIT, sentinel_path = STRBUF_INIT;
> > +	struct stat statbuf;
> > +
> > +	strbuf_addstr(&path, tgt_prefix);
> > +	if (!is_dir_sep(path.buf[path.len - 1])) {
> > +		strbuf_addch(&path, '/');
> > +	}
> > +
> > +	/* check sentinel */
> > +	strbuf_addstr(&sentinel_path, path.buf);
> > +	strbuf_addstr(&sentinel_path, OVERLOAD_SENTINEL_NAME);
> > +	if (!stat(sentinel_path.buf, &statbuf)) {
> > +		strbuf_release(&path);
> > +		return 1;
> > +	}
> > +
> > +	/* check if we're overriding the threshold (e.g., for testing) */
> > +	test_threshold_val = getenv("GIT_TRACE2_TEST_OVERLOAD_FILE_COUNT");
> > +	if (test_threshold_val)
> > +		overload_file_count = atoi(test_threshold_val);
> > +	if (overload_file_count <= 0)
> > +		overload_file_count = OVERLOAD_FILE_COUNT;
> > +
> > +
> > +	/* check file count */
> > +	dirp = opendir(path.buf);
> > +	while (file_count < overload_file_count && dirp && readdir(dirp))
> > +		file_count++;
> > +	if (dirp)
> > +		closedir(dirp);
> > +
> > +	if (file_count >= overload_file_count) {
> > +		creat(sentinel_path.buf, S_IRUSR | S_IWUSR);
> > +		/* TODO: Write a target-specific message? */
> 
> Perhaps leave the TODO out of the code? I did see it in your commit message.

> > +		strbuf_release(&path);
> > +		return 1;
> > +	}
> > +
> > +	strbuf_release(&path);
> > +	return 0;
> > +}
> > +
> >  static int tr2_dst_try_auto_path(struct tr2_dst *dst, const char *tgt_prefix)
> >  {
> >  	int fd;
> > @@ -50,6 +121,16 @@ static int tr2_dst_try_auto_path(struct tr2_dst *dst, const char *tgt_prefix)
> >  	strbuf_addstr(&path, sid);
> >  	base_path_len = path.len;
> >  
> > +	if (tr2_dst_overloaded(tgt_prefix)) {
> > +		strbuf_release(&path);
> > +		if (tr2_dst_want_warning())
> > +			warning("trace2: not opening %s trace file due to too "
> > +				"many files in target directory %s",
> > +				tr2_sysenv_display_name(dst->sysenv_var),
> > +				tgt_prefix);
> > +		return 0;
> > +	}
> > +
> >  	for (attempt_count = 0; attempt_count < MAX_AUTO_ATTEMPTS; attempt_count++) {
> >  		if (attempt_count > 0) {
> >  			strbuf_setlen(&path, base_path_len);
> > 
> 
> Overall, this looks correct and the test is very clear. Seems to be a helpful feature!
> 
> I only have the nits mentioned above.