search

IBM / ubiquity

Ubiquity
Apache License 2.0
90 stars 26 forks source link

Fix/ub 624 flex should not write log into flex driver directory #181

Closed hfeish closed 6 years ago

hfeish commented 6 years ago
  1. Handle flex logrotate internal
  2. configurable for FLEX-LOG-DIR in ubiquity-configmap.yml and ubiquity_installer.conf, default is /var/log, customer need to make sure Flex-Log-dir exist in the minors
  3. configurable for FLEX-LOG-ROTATE-MAXSIZE in ubiquity-configmap.yml, default is 50MB Need to work with https://github.com/IBM/ubiquity-k8s/pull/163

This change is Reviewable

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.5%) to 53.242% when pulling 4c140501b22c4414a17e9d204d14ca8dd85ba997 on fix/UB-624_Flex_should_not_write_log_into_flex_driver_directory into aa56b12148495b07454c57452a9bb6c1d9cfe1b8 on dev.

shay-berman commented 6 years ago

Review status: 0 of 3 files reviewed at latest revision, 5 unresolved discussions.

utils/logs/global_logger.go, line 25 at r2 (raw file):

  "path"
  "strings"
  "github.com/natefinch/lumberjack"

send to Alon the new import link please.

utils/logs/global_logger.go, line 60 at r2 (raw file):

  if os.IsNotExist(err) {
      fileDir,_ := path.Split(filePath)
      err := os.MkdirAll(fileDir, 0766)

but what if the directory already exist? (you check above if the file exist but what if the file not exist but the dir exist? you suould handle it as well)

utils/logs/global_logger.go, line 76 at r2 (raw file):

  }

  fileStatSize := int(fileStat.Size())

just do it in one line, instead of 2 lines

utils/logs/global_logger.go, line 79 at r2 (raw file):

  fileStatSize = fileStatSize / 1024 / 1024

  // If log file size bigger than 52428800 (50MB), will use lunberjack to run the logrotate

don't hardcoded in the comment the max size which is configurable.

utils/logs/global_logger.go, line 83 at r2 (raw file):

      initLogger(level, io.MultiWriter(logFile))
  } else {
      initLogger(level, &lumberjack.Logger{

what happened if 10 flex runs at the same time? How this lumberhjack.Logger log rotate handle it? please make sure we have no issue with concurrency

Comments from Reviewable

shay-berman commented 6 years ago

Review status: 0 of 3 files reviewed at latest revision, 5 unresolved discussions.

utils/logs/global_logger.go, line 83 at r2 (raw file):

Previously, shay-berman wrote…
what happened if 10 flex runs at the same time? How this lumberhjack.Logger log rotate handle it? please make sure we have no issue with concurrency

Fie, did you tested it?

Comments from Reviewable

hfeish commented 6 years ago

Review status: 0 of 3 files reviewed at latest revision, 5 unresolved discussions.

utils/logs/global_logger.go, line 25 at r2 (raw file):

Previously, shay-berman wrote…
send to Alon the new import link please.

https://github.com/natefinch/lumberjack I had sent this link in my email thread "Legal cycle for package lumberjack"

utils/logs/global_logger.go, line 60 at r2 (raw file):

Previously, shay-berman wrote…
but what if the directory already exist? (you check above if the file exist but what if the file not exist but the dir exist? you suould handle it as well)

MkdirAll will do nothing if the dir exist but file not exist, so we can ignore this

utils/logs/global_logger.go, line 76 at r2 (raw file):

Previously, shay-berman wrote…
just do it in one line, instead of 2 lines

changed

utils/logs/global_logger.go, line 79 at r2 (raw file):

Previously, shay-berman wrote…
don't hardcoded in the comment the max size which is configurable.

done

utils/logs/global_logger.go, line 83 at r2 (raw file):

Previously, shay-berman wrote…
Fie, did you tested it?

https://wiki.xiv.ibm.com/display/HostSide/Flex+log+file+refactor I had some tests about this and add some comments under the page, please take a look, thanks!

Comments from Reviewable

shay-berman commented 6 years ago

Review status: 0 of 4 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

utils/logs/global_logger.go, line 60 at r2 (raw file):

Previously, hfeish (Fei Huang) wrote…
MkdirAll will do nothing if the dir exist but file not exist, so we can ignore this

ok please make sure you have UT for this

utils/logs/global_logger.go, line 83 at r2 (raw file):

Previously, hfeish (Fei Huang) wrote…
https://wiki.xiv.ibm.com/display/HostSide/Flex+log+file+refactor I had some tests about this and add some comments under the page, please take a look, thanks!

whats happened if one flex rotate the file while other one write to it?

Comments from Reviewable

hfeish commented 6 years ago

Review status: 0 of 5 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

utils/logs/global_logger.go, line 60 at r2 (raw file):

Previously, shay-berman wrote…
ok please make sure you have UT for this

Add UT in the utils/logs/global_logger_test.go

utils/logs/global_logger.go, line 83 at r2 (raw file):

Previously, shay-berman wrote…
whats happened if one flex rotate the file while other one write to it?

When we call InitFileLogger, it will init a logger, and before that we need to compare the log file size, if it < rotatesize, it will init a logger with io.MultiWriter, and if it > rotatesize, it will init a lumberjack.logger, and this logger will rotate the log file, it will close the current logfile and renamed and a new log file created with original name. Thus the filename you give logger is always the same file name. And since we rotate with 50MB default size, there is no much chance to do the logrotate every day

Comments from Reviewable

shay-berman commented 6 years ago

Review status: 0 of 5 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

utils/logs/global_logger.go, line 83 at r2 (raw file):

Previously, hfeish (Fei Huang) wrote…
When we call InitFileLogger, it will init a logger, and before that we need to compare the log file size, if it < rotatesize, it will init a logger with io.MultiWriter, and if it > rotatesize, it will init a lumberjack.logger, and this logger will rotate the log file, it will close the current logfile and renamed and a new log file created with original name. Thus the filename you give logger is always the same file name. And since we rotate with 50MB default size, there is no much chance to do the logrotate every day

but please make sure flex handle log rotating while other flex also do the log rotating. This is not a rare scenario.

utils/logs/global_logger_test.go, line 3 at r3 (raw file):

package logs_test

import (

what is this test file please mention who trigger this test and some detail how to run it. Do we need to add it into CI? provide more detail here please.

Comments from Reviewable

hfeish commented 6 years ago

Review status: 0 of 5 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

utils/logs/global_logger.go, line 83 at r2 (raw file):

Previously, shay-berman wrote…
but please make sure flex handle log rotating while other flex also do the log rotating. This is not a rare scenario.

I tried to attach 5 pods at same time, and it works and no error in log file, I think it meet the multi flex, right

utils/logs/global_logger_test.go, line 3 at r3 (raw file):

Previously, shay-berman wrote…
what is this test file please mention who trigger this test and some detail how to run it. Do we need to add it into CI? provide more detail here please.

this is example test, dev trigger it with "go test -v", we don't need to add it into CI, because this test only prove the API works as expect just like Unit test.

Comments from Reviewable

shay-berman commented 6 years ago

Review status: 0 of 5 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

utils/logs/global_logger_test.go, line 3 at r3 (raw file):

Previously, hfeish (Fei Huang) wrote…
this is example test, dev trigger it with "go test -v", we don't need to add it into CI, because this test only prove the API works as expect just like Unit test.

how does "go test -v" trigger this file? its not a testing or ginkgo file. What do I miss here? Please add doc string in the beginning of the file that explain this test file. Do you plan to trigger it during the scripts/run_units.sh ?

Comments from Reviewable

hfeish commented 6 years ago

Review status: 0 of 5 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

utils/logs/global_logger_test.go, line 3 at r3 (raw file):

Previously, shay-berman wrote…
how does "go test -v" trigger this file? its not a testing or ginkgo file. What do I miss here? Please add doc string in the beginning of the file that explain this test file. Do you plan to trigger it during the scripts/run_units.sh ?

This example test file existed, so I add a new example test in this file. Add doc string . I don't plan to trigger it during the scripts/run_units.sh

Comments from Reviewable

shay-berman commented 6 years ago

LGTM

Review status: 0 of 5 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

utils/logs/global_logger_test.go, line 3 at r3 (raw file):

Previously, hfeish (Fei Huang) wrote…
This example test file existed, so I add a new example test in this file. Add doc string . I don't plan to trigger it during the scripts/run_units.sh

ok so if you feel OK with this testing, then go a head and merge to dev.

Comments from Reviewable