Copilot commented on code in PR #9324:
URL: https://github.com/apache/cloudstack/pull/9324#discussion_r3773419530
##########
client/src/main/java/org/apache/cloudstack/ServerDaemon.java:
##########
@@ -333,9 +334,31 @@ private RequestLog createRequestLog() {
log.setAppend(true);
log.setLogTimeZone("GMT");
log.setLogLatency(true);
+ createRotateFile(logPath);
return log;
}
+ private void createRotateFile(File logPath) {
+ String rotatefile = "/etc/logrotate.d/access";
+ String fileContents = logPath.getAbsolutePath() + " {\n"
Review Comment:
Writing a logrotate config into `/etc/logrotate.d` at runtime (and using a
generic filename like `access`) can fail due to permissions and can overwrite
administrator-managed config. CloudStack already ships logrotate configs via
packaging (e.g. `server/conf/cloudstack-management.logrotate.in`), so this is
likely better handled as a packaged config (and/or via a configurable
path/name) rather than being created on server startup.
##########
client/src/main/java/org/apache/cloudstack/ServerDaemon.java:
##########
@@ -333,9 +334,31 @@ private RequestLog createRequestLog() {
log.setAppend(true);
log.setLogTimeZone("GMT");
log.setLogLatency(true);
+ createRotateFile(logPath);
return log;
}
+ private void createRotateFile(File logPath) {
+ String rotatefile = "/etc/logrotate.d/access";
+ String fileContents = logPath.getAbsolutePath() + " {\n"
+ + " copytruncate"
+ + " daily"
+ + " rotate 14"
+ + " compress"
+ + " missingok"
+ + " create 0644 cloud cloud"
+ + "}";
Review Comment:
The generated logrotate config concatenates the closing brace directly after
the last token ("cloud}"), which can make the config invalid for logrotate to
parse. Add a newline (or at least whitespace) before the closing '}' so it is a
separate token.
##########
client/src/main/java/org/apache/cloudstack/ServerDaemon.java:
##########
@@ -333,9 +334,31 @@ private RequestLog createRequestLog() {
log.setAppend(true);
log.setLogTimeZone("GMT");
log.setLogLatency(true);
+ createRotateFile(logPath);
return log;
}
+ private void createRotateFile(File logPath) {
+ String rotatefile = "/etc/logrotate.d/access";
+ String fileContents = logPath.getAbsolutePath() + " {\n"
+ + " copytruncate"
+ + " daily"
+ + " rotate 14"
+ + " compress"
+ + " missingok"
+ + " create 0644 cloud cloud"
+ + "}";
+ File rotateConfigFile = new File(rotatefile);
+ try {
+ FileWriter fw = new FileWriter(rotateConfigFile);
+ fw.write(fileContents);
+ fw.close();
+ } catch (IOException e) {
+ // log but continue without rotate (for now)
+ LOG.warn("no way to rotate access log, continuing as is");
+ }
Review Comment:
This block uses a manual close (which can leak the FileWriter on write
failures) and logs via an undefined logger (LOG), which will not compile. Use
try-with-resources and log using the existing `logger` field, including the
exception details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
