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

Updated logrotate frequency to allow for an override #1814

Merged
merged 26 commits into from
Aug 1, 2018

Conversation

kdorosh
Copy link
Contributor

@kdorosh kdorosh commented Jun 27, 2018

To use this feature add additional file/file groups (via wildcard) and times to AdditionalLogrotateFile.

@kdorosh kdorosh changed the base branch from master to hs_staging June 27, 2018 15:44
@kdorosh kdorosh changed the base branch from hs_staging to master June 27, 2018 15:51
this.filename = filename;
this.extension = extension;
this.dateformat = dateformat;
this.frequencyOverride = frequencyOverride;
}

public String getFilename() {
return filename;
}

public Optional<String> getExtension() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Each of the getters in a POJO like this should have the same return type as the private member variables they expose. In this case, I'd recommend having extension, dateformat, and frequencyOverride be Optional<>s (in both their member variable types and their getter types).

Our Jackson YAML deserialization should automatically take care of setting a missing field to an empty Optional<> in the resulting Java object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to deserialize to Guava Optionals in our POJOs

@@ -88,7 +88,8 @@ public String getCompressExt() {
new LogrotateAdditionalFile(
taskDefinition.getTaskDirectoryPath().resolve(additionalFile.getFilename()).toString(),
additionalFile.getExtension().or(Strings.emptyToNull(Files.getFileExtension(additionalFile.getFilename()))),
dateformat
dateformat,
additionalFile.getLogrotateFrequencyOverride()
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll want to be consistent on the indent here before we merge up to master.

}

@JsonCreator
public SingularityExecutorLogrotateAdditionalFile(@JsonProperty("filename") String filename,
@JsonProperty("extension") Optional<String> extension,
@JsonProperty("dateformat") Optional<String> dateformat) {
@JsonProperty("extension") String extension,
Copy link
Member

Choose a reason for hiding this comment

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

Curious why you removed the optionals from the arguments. Both cases should work for that particular usage but there are a few things to keep in mind:

  • Jackson can handle the optionals just fine when reading from json and a missing key will be an absent optional
  • Not relevant for this case, but always be on the lookout for other uses of a constructor that would cause something like this to be a breaking change. For this particular case, these POJOs are just used to model something read from json. But for other cases, releasing a new version with a changed constructor would break other users

@@ -36,6 +36,7 @@ notifempty

{{#if extrasFiles}}
{{#each extrasFiles}}{{{filename}}} {
{{#if logrotateFrequencyOverride }}{{{logrotateFrequencyOverride}}}{{/if}}
Copy link
Member

Choose a reason for hiding this comment

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

We should double check if this will 'just work'. Not all versions of logrotate support hourly by default (Many are normally run on a daily cron, not hourly). If you look in SingularityExecutorTaskLogManager.writeLogrotateFile you'll notice that we additionally write an hourly cron for the specific logrotate conf file that needs it. We will likely need an additional check in that method to see if an additional file has an hourly rotate and possibly a separate cron file if so (the current cron uses the -f option to support hourly and force a logrotate run for those versions that do not support it)

Copy link
Member

Choose a reason for hiding this comment

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

Additional note, enough OSs likely support hourly logrotate now that we could do away with this extra check if we verify that it still works as intended

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 have discussed this at length and determined that it's ok to put the logrotate config files at /etc/logrotate.d/hourly. The Mesos slave running our SingularityExecutor needs to support hourly cron jobs at this directory. We have already internally updated our puppet deploy to enforce this config on mesos slaves.

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 have decided to support older logrotate versions and added code to Singularity to force a rotation hourly using the -f flag and a cron schedule. This implementation writes config files to two directories:

  • /etc/logrotate.d
  • /etc/logrotate.d/hourly

@HubSpot HubSpot deleted a comment from kdorosh Jul 2, 2018
*/
public List<LogrotateAdditionalFile> getExtrasFilesHourly() {
return getAllExtraFiles().stream()
.filter(p -> p.getLogrotateFrequencyOverride().equals(SingularityExecutorLogrotateFrequency.HOURLY)).collect(Collectors.toList());
Copy link
Member

Choose a reason for hiding this comment

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

nit on formatting, put the collect call on a new line to make it more readable

}

return transformed;
}


Copy link
Member

Choose a reason for hiding this comment

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

nit, extra new line

additionalFile.getExtension().or(Strings.emptyToNull(Files.getFileExtension(additionalFile.getFilename()))),
dateformat
));
if (additionalFile.getLogrotateFrequencyOverride().isPresent() &&
Copy link
Member

Choose a reason for hiding this comment

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

method name is getAll, did you meant to filter to hourly here still?

@ssalinas ssalinas added the hs_qa label Jul 5, 2018
@ssalinas ssalinas added this to the 0.21.0 milestone Jul 9, 2018
Copy link
Member

@ssalinas ssalinas left a comment

Choose a reason for hiding this comment

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

Two last nit-picky things before we get this on hs_stable

}
}

private Optional<SingularityExecutorLogrotateFrequency> getAdditionalHourlyFile() {
// Get the cron schedule for an additional file with an HOURLY schedule, if there is any
java.util.Optional<SingularityExecutorLogrotateAdditionalFile> additionalHourlyFile = configuration.getLogrotateAdditionalFiles()
Copy link
Member

Choose a reason for hiding this comment

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

nit, while util.Optional is the newer/nicer. It's probably easier for us to stick with guava optional until we do a consolidated swap across the whole repo.

.collect(Collectors.toList());
}

public boolean getIsGlobalLogrotateHourly() {
Copy link
Member

Choose a reason for hiding this comment

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

For booleans, the standard is to use the is prefix by itself (isGlobalLogrotateHourly). A number of libraries like reflections (and the the handlebars templater in this case) go by that standard.

@ssalinas
Copy link
Member

🚢

1 similar comment
@kdorosh
Copy link
Contributor Author

kdorosh commented Jul 12, 2018

🚢

@pschoenfelder
Copy link
Contributor

🚢

@baconmania
Copy link
Contributor

🚢

@ssalinas ssalinas merged commit 9910a1e into master Aug 1, 2018
@ssalinas ssalinas deleted the logrotate_freq_override branch August 1, 2018 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants