Support log file size and number limits #879

jianjuns · 2020-06-27T06:00:51Z

klog does not enforce max log file size limit when the log file is not
specifiec by the --log_file argument. This commit sets klog.MaxSize to
the --log_file_max_size argument value when logging to file is enabled
and --log_file is not provided.
The commit also adds a new argument --log_file_max_num to define the
max number of log files to be kept. The file number limit is enforced by
Antrea code that periodically checks the INFO log files and deletes the
oldest files to keep at most the specified max number of log files.

Fixes: #788

antrea-bot · 2020-06-27T06:01:03Z

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

/test-e2e: to trigger e2e tests.
/skip-e2e: to skip e2e tests.
/test-conformance: to trigger conformance tests.
/skip-conformance: to skip conformance tests.
/test-whole-conformance: to trigger all conformance tests on linux.
/skip-whole-conformance: to skip all conformance tests on linux.
/test-networkpolicy: to trigger networkpolicy tests.
/skip-networkpolicy: to skip networkpolicy tests.
/test-windows-conformance: to trigger windows conformance tests.
/skip-windows-conformance: to skip windows conformance tests.
/test-all: to trigger all tests (except whole conformance).
/skip-all: to skip all tests (except whole conformance).

These commands can only be run by members of the vmware-tanzu organization.

jianjuns · 2020-06-27T06:01:28Z

@antoninbas @tnqn : let me know if you are fine with the approach or not.
I still plan to add some tests.

antoninbas

the approach looks fine to me. I feel like we should be able to write a unit test for this?

antoninbas · 2020-06-29T21:16:36Z

pkg/log/log_file.go

+
+// InitLogFileLimits initializes log file maximum size and maximum number limits based on the
+// command line flags.
+func InitLogFileLimits(fs *pflag.FlagSet) {


I think we should let this function return an error, I don't see any reason not to, and it is itself calling functions that may return errors (even if unlikely).

Yes, we could return errors. Just feel no big harm to let Agent continue to run even at such errors. The errors could happen only when klog implementation changes.

pkg/log/log_file.go

antoninbas · 2020-06-29T21:21:09Z

pkg/log/log_file.go

+}
+
+// StartLogFileNumberMonitor starts monitoring the log files to make sure the
+// number of INFO log files does not exceed the maximum limit, when the log file


I think we should include all verbosity levels, not just INFO. You never know, the WARNING / ERROR files may grow large as well, and I don't see a downside to monitoring their size as well. I don't think we need to be concerned about missing important errors.

So you mean for example <10 INFO files, <10 ERROR files, <10WARNING files?

Changed to limit all severity levels.

pkg/log/log_file.go

jianjuns · 2020-06-29T21:32:09Z

the approach looks fine to me. I feel like we should be able to write a unit test for this?

Great to know. I was thinking to add integration tests. Could consider unit tests too.

antoninbas · 2020-06-29T21:34:38Z

You may be able to use this for unit tests (in-memory filesystem): https://github.com/spf13/afero. It's already an Antrea dependency.

jianjuns · 2020-06-29T21:42:09Z

You may be able to use this for unit tests (in-memory filesystem): https://github.com/spf13/afero. It's already an Antrea dependency.

How can I let klog log to in-memory FS?

antoninbas · 2020-06-29T21:53:24Z

You may be able to use this for unit tests (in-memory filesystem): https://github.com/spf13/afero. It's already an Antrea dependency.

How can I let klog log to in-memory FS?

You're right, it wouldn't be possible if we want to use klog to generate the test logs, which is probably the right thing to do.
Still, do you think this warrants an integration test? I imagine you will create a temporary directory with ioutil.TempDir and use that as the logs directory, which IMO is ok to do in unit tests. What do you think?

jianjuns · 2020-07-02T06:20:19Z

@antoninbas @tnqn : I tested the PR and added unit tests. Could I request a review from you? Hope to get it in 0.8.0 for the DECC request.

tnqn

LGTM, two minor comments on cleanup style.

tnqn · 2020-07-02T10:16:18Z

pkg/log/log_file.go

+	if err != nil {
+		klog.Errorf("Failed to open log directory %s: %v", logDir, err)
+		return
+	}


defer f.Close() here in case of Readdir failure?
The error of deferred call is normally ignored.
https://github.com/kubernetes/kubernetes/blob/a138be8722ebd0ce281029fa747315500a99ffd5/pkg/kubelet/kuberuntime/logs/logs.go#L295
https://github.com/moby/moby/blob/3b4cfa97237a8e1fb5eb985e4a7c0717cd14f5c8/daemon/graphdriver/devmapper/deviceset.go#L556

Yes. I hope to close the file earlier. Changed the code to cover Readdir failure.

tnqn · 2020-07-02T10:41:50Z

pkg/log/log_file_test.go

+	assert.Equal(t, 2, infoLogFileNum, "info log file number after checking")
+	assert.Equal(t, 2, warningLogFileNum, "warning log file number after checking")
+
+	restoreFlagDefaultValues()


Not sure whether there are other exits in the function, but is defer restoreFlagDefaultValues() more safe and go?

Changed to use defer.

tnqn

LGTM

antoninbas

LGTM, I think the commit message / PR description still mention INFO files only

antoninbas · 2020-07-02T18:09:06Z

pkg/log/log_file_test.go

+			} else if strings.Contains(file.Name(), ".log.WARNING.") {
+				warningLogFiles = append(warningLogFiles, file)
+			} else if strings.Contains(file.Name(), ".log.ERROR.") {
+			} else {


would be good to have a comment here stating that the test is purposely not generating any error logs, or remove the empty if statement altogether

Added comments.

antoninbas · 2020-07-02T18:10:00Z

pkg/log/log_file_test.go

+	}
+	defer os.RemoveAll(testLogDir)
+
+	args := []string{"--logtostderr=false", "--log_dir=" + testLogDir, "--log_file_max_size=1", "--log_file_max_num=2"}


do you think you can use a local const for the max num here and use that in your asserts below?

antoninbas · 2020-07-02T18:12:28Z

pkg/log/log_file_test.go

+	logs.InitLogs()
+	defer restoreFlagDefaultValues()
+
+	// Should generate 3 log files (100K * 30 / 1M).


maybe use consts for 100k, 1M and number of iterations

then we can add an assert like this to document our intent:

require.Greater(logSize * numIters / logFileMaxSize, logFileMaxNum, "test should generate enough logs to exceed --log_file_max_num")

The problem is hard to accurately control the log message size/number, due to the log file/line header and klog flush time (I am not very sure about this but I did see log interval impact the number of log files created). So, I feel such assert does not help much.

Sounds good. You probably want to include that information in the comment though.

Ok. Added some comments to explain this.

antoninbas

LGTM

jianjuns · 2020-07-02T22:06:39Z

/test-all

abhiraut · 2020-07-02T22:11:49Z

pkg/log/log_file.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// Package main under directory cmd parses and validates user input,


incorrect pkg description

Thanks. Fixed.

abhiraut · 2020-07-02T22:15:49Z

pkg/log/log_file.go

+	if err != nil {
+		return
+	}
+	if maxSize > 1024*1024 {


this could be a constant?

Defined a constant.

abhiraut · 2020-07-02T22:16:45Z

pkg/log/log_file.go

+		return
+	}
+	if maxSize > 1024*1024 {
+		klog.Errorf("The specified log file max size %d is too big, ignored", maxSize)


maybe quantify "too big" ? with a number that we allow

pkg/log/log_file_test.go

klog does not enforce max log file size limit when the log file is not specifiec by the --log_file argument. This commit sets klog.MaxSize to the --log_file_max_size argument value when logging to file is enabled and --log_file is not provided. The commit also adds a new argument --log_file_max_num to define the max number (per severity level) of log files to be kept. The file number limit is enforced by Antrea code that periodically checks the log files and deletes the oldest files to keep at most the specified max number of log files. Fixes: antrea-io#788

jianjuns · 2020-07-02T23:02:42Z

/test-all

abhiraut · 2020-07-02T23:04:54Z

pkg/log/log_file.go

+	// Check log file number every 10 mins.
+	logFileCheckInterval = time.Minute * 10
+	// Allowed maximum value for the maximum file size limit.
+	maxMaxSizeMB = 1024 * 100


this was 1024*1024 previously.. decided to reduce?

Yes. Do not really what is the appropriate number. klog default value is 18G.

klog does not enforce max log file size limit when the log file is not specifiec by the --log_file argument. This commit sets klog.MaxSize to the --log_file_max_size argument value when logging to file is enabled and --log_file is not provided. The commit also adds a new argument --log_file_max_num to define the max number (per severity level) of log files to be kept. The file number limit is enforced by Antrea code that periodically checks the log files and deletes the oldest files to keep at most the specified max number of log files. Fixes: antrea-io#788

jianjuns requested review from antoninbas and tnqn June 27, 2020 06:00

vmwclabot added the cla-not-required label Jun 27, 2020

jianjuns force-pushed the log-limit branch 5 times, most recently from 3a1f12c to a155dcd Compare June 29, 2020 20:56

antoninbas previously approved these changes Jun 29, 2020

View reviewed changes

jianjuns dismissed antoninbas’s stale review via 9213a1f July 2, 2020 06:13

jianjuns force-pushed the log-limit branch from a155dcd to 9213a1f Compare July 2, 2020 06:13

jianjuns changed the title ~~WIP: Support log file size and number limits~~ Support log file size and number limits Jul 2, 2020

tnqn reviewed Jul 2, 2020

View reviewed changes

jianjuns force-pushed the log-limit branch from 9213a1f to 082730a Compare July 2, 2020 16:10

tnqn previously approved these changes Jul 2, 2020

View reviewed changes

antoninbas previously approved these changes Jul 2, 2020

View reviewed changes

jianjuns dismissed stale reviews from antoninbas and tnqn via 6511bf6 July 2, 2020 18:24

jianjuns force-pushed the log-limit branch 3 times, most recently from 03b3538 to e63b9b4 Compare July 2, 2020 18:45

antoninbas previously approved these changes Jul 2, 2020

View reviewed changes

jianjuns dismissed antoninbas’s stale review via daba240 July 2, 2020 19:45

jianjuns force-pushed the log-limit branch 4 times, most recently from 6755a23 to c205232 Compare July 2, 2020 21:46

abhiraut reviewed Jul 2, 2020

View reviewed changes

jianjuns force-pushed the log-limit branch from c205232 to 821e400 Compare July 2, 2020 22:38

abhiraut approved these changes Jul 2, 2020

View reviewed changes

jianjuns merged commit cfd6440 into antrea-io:master Jul 2, 2020

jianjuns deleted the log-limit branch August 25, 2020 21:54

Support log file size and number limits #879

Support log file size and number limits #879

Conversation

jianjuns commented Jun 27, 2020 • edited Loading

antrea-bot commented Jun 27, 2020

jianjuns commented Jun 27, 2020

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns commented Jun 29, 2020

antoninbas commented Jun 29, 2020

jianjuns commented Jun 29, 2020

antoninbas commented Jun 29, 2020

jianjuns commented Jul 2, 2020

tnqn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

jianjuns commented Jul 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns commented Jul 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns commented Jun 27, 2020 •

edited

Loading