Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add logging to appliance init #2284

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DanielXiao
Copy link
Contributor

Write logs of ova-webserver to /var/log/fileserver.

VIC Appliance Checklist:

  • Up to date with master branch
  • Added tests
  • Considered impact to upgrade
  • Tests passing
  • Updated documentation
  • Impact assessment checklist

If this is a feature or change to existing functionality, consider areas of impact with the Impact
Assessment Checklist

Fixes #777

Write logs of ova-webserver to /var/log/fileserver.

if _, err := os.Stat(conf.logPath); os.IsNotExist(err) {
os.MkdirAll(conf.logPath, 0700)
}
Copy link
Member

@YanzhaoLi YanzhaoLi Jan 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also check whether conf.logPath is a directory. Because users may provide a filename path to conf.logPath.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MkdirALL will return Error if it is a file

		if dir.IsDir() {
			return nil
		}
		return &PathError{"mkdir", path, syscall.ENOTDIR}

os.MkdirAll(conf.logPath, 0700)
}
logPath := filepath.Join(conf.logPath, "fileserver.log")
logFile, err := os.OpenFile(logPath, os.O_RDWR|os.O_APPEND|os.O_CREATE, 0600)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to handle closing the logFile, or the trace.Logger.out already help us handle it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't close the file descriptor because the running process will write logs to it until it exits.

Copy link
Contributor

@wjun wjun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we use journal logs in vic appliance vm already, it seems we do not need to introduce another log file?

@DanielXiao DanielXiao requested a review from zjs January 8, 2019 08:09
@DanielXiao
Copy link
Contributor Author

@zjs Do you think it is necessary to write logs to a file?

@zjs
Copy link
Member

zjs commented Jan 8, 2019

I think #777 is about improving the content of the logs, not changing how they're written (although that may be useful too, as a vSphere Administrator attempting to debug may be more used to finding files in /var/log than checking a journal — but in that case I think we'd want to write them both ways).

For example, I believe we don't currently get good logging around the plugin registration process or failures that can occur there.

@DanielXiao
Copy link
Contributor Author

@wjun From logging design principle https://github.com/vmware/vic-product/blob/master/installer/docs/DESIGN.md#logging, the init log should be written to a file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants