Issue #8943: Check log retention max days & max builds is set globally

[jonnydotgg]

Summary

build_log_retention.days is being ignored and defaulting to 0 if CONCOURSE_MAX_BUILD_LOGS_TO_RETAIN is set but CONCOURSE_MAX_DAYS_TO_RETAIN_BUILD_LOGS is not.

The same applies the other way round, build_log_retention.builds is ignored and defaults to 0 if CONCOURSE_MAX_DAYS_TO_RETAIN_BUILD_LOGS is set but CONCOURSE_MAX_BUILD_LOGS_TO_RETAIN is not.

Steps to reproduce

1. set CONCOURSE_MAX_BUILD_LOGS_TO_RETAIN: 5 in docker-compose.yml
2. create a pipeline and set build_log_retention.builds to 3 and set build_log_retention.days to 7 (see example below).
3. run the test jobs 10 times.
4. wait for atc.reaper.tick.build-reaper.start log
5. see that only 3 logs are kept, not 7 days

Expected results

Logs should not be reaped until 7 days later.

Actual results

Logs are reaped at 0 days!

Additional context

Example pipeline for testing...

---
jobs:
  - name: job
    build_log_retention:
      builds: 3
      days: 7
    public: true
    plan:
      - task: simple-task
        config:
          platform: linux
          image_resource:
            type: registry-image
            source: { repository: busybox }
          run:
            path: echo
            args: ["Hello world!"]

Triaging info

• Concourse version: v7.9.1
• Browser (if applicable):
• Did this used to work?

[xtremerui]

Hi I think it is the expected behaviour refer to the code here

https://github.com/concourse/concourse/blob/master/atc/atccmd/command.go#L208

Once it is set it will overwrite the config in job level.

[jonnydotgg]

I don't think this is the expected behavior.

It should only override the job values if it's specified and it's less than the job value. However, If you only set one and not the other (for example, set MaxBuildLogsToRetain but not MaxDaysToRetainBuildLogs) then the other (MaxDaysToRetainBuildLogs) will be the default value 0 which will always be less than job value.

See this section here, https://github.com/concourse/concourse/blob/master/atc/gc/build_log_retention_calculator.go#L53-L56, it's check if both MaxBuildLogsToRetain and MaxDaysToRetainBuildLogs are set, however if you only set one the other will be 0, leading to this bug.

The change I've suggested adds separate zero value checks for both MaxBuildLogsToRetain and MaxDaysToRetainBuildLogs to prevent this bug occuring.

[xtremerui]

The concourse-ci/unit test is failing for below tests:

Summarizing 3 Failures:

  [FAIL] BuildLogRetentionCalculator [It] default and job set and max set gives max if lower

  github.com/concourse/concourse/atc/gc/build_log_retention_calculator_test.go:38

  [FAIL] BuildLogCollector when there is a pipeline when the FirstLoggedBuildID has an value when FirstLoggedBuildID == 1 when we install a custom build log retention calculator [It] uses build log calculator

  github.com/concourse/concourse/atc/gc/build_log_collector_test.go:676

  [FAIL] BuildLogRetentionCalculator [It] mix of count and days with max

  github.com/concourse/concourse/atc/gc/build_log_retention_calculator_test.go:50

If you run a local unit test by ginkgo in atc/gc folder, you should see the same failure and detail logs. Those are build log retention behaviors (not necessary correct) protected by the tests. Could you take a look at those tests?