Updated logrotate frequency to allow for an override #1814

kdorosh · 2018-06-27T15:43:23Z

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

…ional file/file groups (via wildcard).

baconmania · 2018-06-28T14:59:41Z

...java/com/hubspot/singularity/executor/config/SingularityExecutorLogrotateAdditionalFile.java

        this.filename = filename;
        this.extension = extension;
        this.dateformat = dateformat;
+        this.frequencyOverride = frequencyOverride;
    }

    public String getFilename() {
        return filename;
    }

    public Optional<String> getExtension() {


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.

Updated to deserialize to Guava Optionals in our POJOs

baconmania · 2018-06-29T15:37:05Z

...Executor/src/main/java/com/hubspot/singularity/executor/models/LogrotateTemplateContext.java

@@ -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()


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

…s absent.

ssalinas · 2018-07-02T12:36:35Z

...java/com/hubspot/singularity/executor/config/SingularityExecutorLogrotateAdditionalFile.java

    }

    @JsonCreator
    public SingularityExecutorLogrotateAdditionalFile(@JsonProperty("filename") String filename,
-        @JsonProperty("extension") Optional<String> extension,
-        @JsonProperty("dateformat") Optional<String> dateformat) {
+        @JsonProperty("extension") String extension,


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

ssalinas · 2018-07-02T12:47:39Z

SingularityExecutor/src/main/resources/logrotate.conf.hbs

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

 {{#if extrasFiles}}
 {{#each extrasFiles}}{{{filename}}} {
+  {{#if logrotateFrequencyOverride }}{{{logrotateFrequencyOverride}}}{{/if}}


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)

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

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.

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

…tories and templates.

ssalinas · 2018-07-03T15:08:09Z

...Executor/src/main/java/com/hubspot/singularity/executor/models/LogrotateTemplateContext.java

+   */
+  public List<LogrotateAdditionalFile> getExtrasFilesHourly() {
+    return getAllExtraFiles().stream()
+        .filter(p -> p.getLogrotateFrequencyOverride().equals(SingularityExecutorLogrotateFrequency.HOURLY)).collect(Collectors.toList());


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

ssalinas · 2018-07-03T15:08:38Z

...Executor/src/main/java/com/hubspot/singularity/executor/models/LogrotateTemplateContext.java

    }

    return transformed;
  }

+


nit, extra new line

ssalinas · 2018-07-03T15:08:56Z

...Executor/src/main/java/com/hubspot/singularity/executor/models/LogrotateTemplateContext.java

-          additionalFile.getExtension().or(Strings.emptyToNull(Files.getFileExtension(additionalFile.getFilename()))),
-          dateformat
-      ));
+      if (additionalFile.getLogrotateFrequencyOverride().isPresent() &&


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

… cron schedule.

ssalinas

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

ssalinas · 2018-07-09T17:51:13Z

...r/src/main/java/com/hubspot/singularity/executor/task/SingularityExecutorTaskLogManager.java

    }
  }

+  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()


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.

ssalinas · 2018-07-09T17:53:23Z

...Executor/src/main/java/com/hubspot/singularity/executor/models/LogrotateTemplateContext.java

+        .collect(Collectors.toList());
+  }
+
+  public boolean getIsGlobalLogrotateHourly() {


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 · 2018-07-12T21:44:52Z

🚢

kdorosh · 2018-07-12T22:26:46Z

🚢

pschoenfelder · 2018-07-17T14:19:21Z

🚢

baconmania · 2018-07-17T14:23:42Z

🚢

Updated logrotate frequency to allow for an override by listing addit…

758e89b

…ional file/file groups (via wildcard).

kdorosh changed the base branch from master to hs_staging June 27, 2018 15:44

kdorosh changed the base branch from hs_staging to master June 27, 2018 15:51

Fix checkstyle on .* import for mockito.

4623ebd

kdorosh force-pushed the logrotate_freq_override branch from 2a29d1c to 9c011d4 Compare June 27, 2018 21:33

Update POJOs to use Optional the way it's supposed to be used.

e50f527

kdorosh force-pushed the logrotate_freq_override branch from 1a3654f to e50f527 Compare June 28, 2018 13:41

Really need to figure out how to stop IntelliJ from doing this.

25be9f8

baconmania reviewed Jun 28, 2018

View reviewed changes

Update to deserialize to Guava Optionals.

025c732

kdorosh added the hs_staging label Jun 28, 2018

baconmania reviewed Jun 29, 2018

View reviewed changes

kdorosh added 2 commits June 29, 2018 13:59

Fix handlebars template apply error when logrotateFrequencyOverride i…

8cbb242

…s absent.

Very silly error wrapping an extra Optional and fixing whitespace.

2552650

ssalinas reviewed Jul 2, 2018

View reviewed changes

Jackson can deserialize Optionals to Strings, so put that back.

c94a606

HubSpot deleted a comment from kdorosh Jul 2, 2018

Make parameters final that can be made final.

152eb8c

kdorosh force-pushed the logrotate_freq_override branch from 9cd9ed1 to 152eb8c Compare July 2, 2018 22:13

kdorosh added 2 commits July 3, 2018 11:02

Updated logrotate to support hourly rotates via separate config direc…

bcc0379

…tories and templates.

Consistent spacing.

2631ba5

ssalinas reviewed Jul 3, 2018

View reviewed changes

kdorosh added 6 commits July 3, 2018 11:17

Bug in getAllExtraFiles and some nitpicking formatting.

5092d6b

Add forgotten @provides annotation for new logrotate template.

10b1b98

Fix bug with calling Optional.get on potentially absent global hourly…

76d30af

… cron schedule.

Part two to Optional.get on absent global frequency bug.

1fe3159

No longer calling Optional.get on a findFirst() that may return absent.

6a17308

Proper equality comparison.

d1255c0

ssalinas added the hs_qa label Jul 5, 2018

Prevent duplicated hourly/non-hourly entries in both configs.

79f9fd3

Proper cleanup of second logrotate config.

1d293fb

ssalinas added this to the 0.21.0 milestone Jul 9, 2018

ssalinas reviewed Jul 9, 2018

View reviewed changes

kdorosh and others added 2 commits July 9, 2018 14:25

Rename methods and replace java.util Optional with Guava Optional.

09de273

Don't enforce directory present in config, create if doesn't exist

a8f6a7b

ssalinas added the hs_stable label Jul 16, 2018

ssalinas added 4 commits July 17, 2018 09:14

make these log levels debug instead

c62b6e0

check exists on each before delete

b9053f6

fix this if statement

7ba27b0

Always attempt to remove file if exists

2873512

Missing !

cd13a97

ssalinas merged commit 9910a1e into master Aug 1, 2018

ssalinas deleted the logrotate_freq_override branch August 1, 2018 13:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated logrotate frequency to allow for an override #1814

Updated logrotate frequency to allow for an override #1814

kdorosh commented Jun 27, 2018

baconmania Jun 28, 2018

kdorosh Jun 28, 2018

baconmania Jun 29, 2018

ssalinas Jul 2, 2018

ssalinas Jul 2, 2018

ssalinas Jul 2, 2018

kdorosh Jul 2, 2018

kdorosh Jul 3, 2018

ssalinas Jul 3, 2018

ssalinas Jul 3, 2018

ssalinas Jul 3, 2018

ssalinas left a comment

ssalinas Jul 9, 2018

ssalinas Jul 9, 2018

ssalinas commented Jul 12, 2018

kdorosh commented Jul 12, 2018

pschoenfelder commented Jul 17, 2018

baconmania commented Jul 17, 2018

Updated logrotate frequency to allow for an override #1814

Updated logrotate frequency to allow for an override #1814

Conversation

kdorosh commented Jun 27, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssalinas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssalinas commented Jul 12, 2018

kdorosh commented Jul 12, 2018

pschoenfelder commented Jul 17, 2018

baconmania commented Jul 17, 2018