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

time based retention for build log collector #3560

Merged
merged 4 commits into from May 14, 2019

Conversation

@hprotzek
Copy link
Contributor

hprotzek commented Mar 21, 2019

This adds a time based retention to the build log collector. So it's possible to configure retention count or time or both. If both are configured, the highest value wins: e.g. 10 builds, 1 day = will keep more then 10 builds if per day.

Copy link
Member

vito left a comment

Sweet, this makes a lot of sense. Thanks for starting on it! 👍 Left some early feedback and one idea for the pipeline config.

@@ -148,6 +148,9 @@ type RunCommand struct {
DefaultBuildLogsToRetain uint64 `long:"default-build-logs-to-retain" description:"Default build logs to retain, 0 means all"`
MaxBuildLogsToRetain uint64 `long:"max-build-logs-to-retain" description:"Maximum build logs to retain, 0 means not specified. Will override values configured in jobs"`

DefaultBuildLogsDaysToRetain uint64 `long:"default-build-logs-days-to-retain" description:"Default days to retain build logs. 0 means unlimited"`

This comment has been minimized.

Copy link
@vito

vito Mar 25, 2019

Member

The name here feels odd - how about default-days-to-retain-build-logs and max-days-to-retain-build-logs?

@@ -10,6 +10,7 @@ type JobConfig struct {
SerialGroups []string `yaml:"serial_groups,omitempty" json:"serial_groups,omitempty" mapstructure:"serial_groups"`
RawMaxInFlight int `yaml:"max_in_flight,omitempty" json:"max_in_flight,omitempty" mapstructure:"max_in_flight"`
BuildLogsToRetain int `yaml:"build_logs_to_retain,omitempty" json:"build_logs_to_retain,omitempty" mapstructure:"build_logs_to_retain"`
BuildLogsDaysToRetain int `yaml:"build_logs_days_to_retain,omitempty" json:"build_logs_days_to_retain,omitempty" mapstructure:"build_logs_days_to_retain"`

This comment has been minimized.

Copy link
@vito

vito Mar 25, 2019

Member

What do you think of something like this?:

build_log_retention:
  builds: 10
  days: 5

We'd have to keep build_logs_to_retain for backwards-compatibility, but this would make it easier to add more retention policies without polluting the toplevel configuration too much. I could see someone wanting something like this for example:

build_log_retention:
  failed_builds: 1000
  succeeded_builds: 10

Whaddaya think?

@hprotzek

This comment has been minimized.

Copy link
Contributor Author

hprotzek commented Mar 29, 2019

Thanks @vito I will look into it and implement your feedback.

@hprotzek hprotzek force-pushed the hprotzek:time-based-log-retention branch from b3ba6fa to bdfd797 May 10, 2019
hprotzek added 2 commits Mar 20, 2019
Signed-off-by: Holger Protzek <holger.protzek@springernature.com>
Signed-off-by: Holger Protzek <holger.protzek@springernature.com>
@hprotzek hprotzek force-pushed the hprotzek:time-based-log-retention branch 2 times, most recently from 997ce6c to 3b220b0 May 10, 2019
Signed-off-by: Holger Protzek <holger.protzek@springernature.com>
@hprotzek hprotzek force-pushed the hprotzek:time-based-log-retention branch from 3b220b0 to 86f1fa8 May 10, 2019
@hprotzek

This comment has been minimized.

Copy link
Contributor Author

hprotzek commented May 10, 2019

Morning @vito finally I got time to add your suggestions. Build log retention can now be configured:

build_log_retention:
  builds: 10
  days: 5

The old build_logs_to_retain is still supported. When used both, a validation error will be thrown.

Copy link
Member

vito left a comment

Sweet! I left a few minor suggestions. The new approach looks great. Thanks for coming back to it!

@@ -123,6 +130,12 @@ func (br *buildLogCollector) Run(ctx context.Context) error {
}
}

if daysToRetainBuildLogs > 0 {
if build.StartTime().AddDate(0, 0, daysToRetainBuildLogs).After(time.Now()) {

This comment has been minimized.

Copy link
@vito

vito May 13, 2019

Member

Should this be build.FinishTime()? It might matter for builds that take a very long time to run - if it runs for 2 days but I have a policy of 'reap after 1 day' I wouldn't want it to be immediately reaped.

}
}

func (blrc *buildLogRetentionCalculator) BuildLogsToRetain(job db.Job) int {
func (blrc *buildLogRetentionCalculator) BuildLogsToRetain(job db.Job) (int, int) {

This comment has been minimized.

Copy link
@vito

vito May 13, 2019

Member

Maybe this could return atc.BuildLogRetention? It's not clear which int is which, and we already have the handy struct for it.

if job.BuildLogRetention != nil && job.BuildLogsToRetain != 0 {
errorMessages = append(
errorMessages,
identifier+fmt.Sprintf(" can't use both build_log_retention and deprecated build_logs_to_retain"),

This comment has been minimized.

Copy link
@vito

vito May 13, 2019

Member
Suggested change
identifier+fmt.Sprintf(" can't use both build_log_retention and deprecated build_logs_to_retain"),
identifier+fmt.Sprintf(" can't use both build_log_retention and build_logs_to_retain"),

We haven't deprecated it just yet - the validation makes sense but we haven't decided whether we want to deprecate the shorthand form. 🙂

calculate cut off time from EndTime
removed deprecation from validation warning

Signed-off-by: Holger Protzek <holger.protzek@springernature.com>
@hprotzek

This comment has been minimized.

Copy link
Contributor Author

hprotzek commented May 14, 2019

Hi @vito ,I added your suggestions, thanks for the review.

@vito
vito approved these changes May 14, 2019
Copy link
Member

vito left a comment

Tried it out locally and it works!

For my own understanding, days: 2 doesn't mean 'reap after 2 days', it means 'keep anything within 2 days' and fall back on any configured build limit to further retain builds.

So if I have this:

build_log_retention:
  days: 2

This means "keep everything within the past 2 days, reap everything older".

build_log_retention:
  days: 2
  builds: 10

This means "keep everything within the past 2 days, and keep the last 10 builds (regardless of age), reap everything else".

To be honest, I think this makes sense and is actually the most literal reading, but I do find it a bit hard to reason about. We'll have to be super clear in the documentation. 🤔

Thanks!

@vito vito merged commit a737aec into concourse:master May 14, 2019
5 checks passed
5 checks passed
DCO DCO
Details
WIP Ready for review
Details
concourse-ci/testflight Concourse CI build success
Details
concourse-ci/unit Concourse CI build success
Details
concourse-ci/watsjs Concourse CI build success
Details
kcmannem added a commit to concourse/charts that referenced this pull request May 14, 2019
concourse/concourse#3560

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
Co-authored-by: Krishna Mannem <kmannem@pivotal.io>
cirocosta added a commit to concourse/charts that referenced this pull request May 15, 2019
concourse/concourse#3560

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
Co-authored-by: Krishna Mannem <kmannem@pivotal.io>
@cirocosta cirocosta mentioned this pull request May 15, 2019
3 of 3 tasks complete
cirocosta added a commit to concourse/charts that referenced this pull request May 20, 2019
* [stable/concourse] Update flags for next Concourse release

- Adds `CONCOURSE_CLUSTER_NAME` (concourse.web.clusterName)
  - https://github.com/concourse/concourse/pull/3736<Paste>

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>

* [stable/concourse] add generic secret configurations

With the introduction generic caching, we no longer need the
vault-specific cacheing flags, while at the same time, having the need
of providing new ones (generic).

concourse/concourse#3628

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>

* [stable/concourse] add auditing flags

With the introduction of auditing capabilities (see Auditor#3577 [1]),
it's now possible to configure in a per-subsystem basis which API
requests an installation should audit.

This commit adds the necessary support for such flags.

[1]: concourse/concourse#3577

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>

* [stable/concourse] update image version to 5.2.0

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
Co-authored-by: Krishna Mannem <kmannem@pivotal.io>

* [stable/concourse] Add time based build log retention flags

concourse/concourse#3560

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
Co-authored-by: Krishna Mannem <kmannem@pivotal.io>

* [stable/concourse] add support for externalGardenUrl

With the addition of `external-garden-url` to the `concourse worker`
command, one is now able to reference a non-embedded garden server to be
used as the target for container creation.

concourse/concourse#3806

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
amine7536 added a commit to amine7536/charts that referenced this pull request May 21, 2019
* [stable/concourse] Update flags for next Concourse release

- Adds `CONCOURSE_CLUSTER_NAME` (concourse.web.clusterName)
  - https://github.com/concourse/concourse/pull/3736<Paste>

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>

* [stable/concourse] add generic secret configurations

With the introduction generic caching, we no longer need the
vault-specific cacheing flags, while at the same time, having the need
of providing new ones (generic).

concourse/concourse#3628

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>

* [stable/concourse] add auditing flags

With the introduction of auditing capabilities (see Auditor#3577 [1]),
it's now possible to configure in a per-subsystem basis which API
requests an installation should audit.

This commit adds the necessary support for such flags.

[1]: concourse/concourse#3577

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>

* [stable/concourse] update image version to 5.2.0

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
Co-authored-by: Krishna Mannem <kmannem@pivotal.io>

* [stable/concourse] Add time based build log retention flags

concourse/concourse#3560

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
Co-authored-by: Krishna Mannem <kmannem@pivotal.io>

* [stable/concourse] add support for externalGardenUrl

With the addition of `external-garden-url` to the `concourse worker`
command, one is now able to reference a non-embedded garden server to be
used as the target for container creation.

concourse/concourse#3806

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
eyenx added a commit to eyenx/charts that referenced this pull request May 28, 2019
* [stable/concourse] Update flags for next Concourse release

- Adds `CONCOURSE_CLUSTER_NAME` (concourse.web.clusterName)
  - https://github.com/concourse/concourse/pull/3736<Paste>

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>

* [stable/concourse] add generic secret configurations

With the introduction generic caching, we no longer need the
vault-specific cacheing flags, while at the same time, having the need
of providing new ones (generic).

concourse/concourse#3628

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>

* [stable/concourse] add auditing flags

With the introduction of auditing capabilities (see Auditor#3577 [1]),
it's now possible to configure in a per-subsystem basis which API
requests an installation should audit.

This commit adds the necessary support for such flags.

[1]: concourse/concourse#3577

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>

* [stable/concourse] update image version to 5.2.0

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
Co-authored-by: Krishna Mannem <kmannem@pivotal.io>

* [stable/concourse] Add time based build log retention flags

concourse/concourse#3560

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
Co-authored-by: Krishna Mannem <kmannem@pivotal.io>

* [stable/concourse] add support for externalGardenUrl

With the addition of `external-garden-url` to the `concourse worker`
command, one is now able to reference a non-embedded garden server to be
used as the target for container creation.

concourse/concourse#3806

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
runningman84 added a commit to runningman84/charts that referenced this pull request Jun 4, 2019
* [stable/concourse] Update flags for next Concourse release

- Adds `CONCOURSE_CLUSTER_NAME` (concourse.web.clusterName)
  - https://github.com/concourse/concourse/pull/3736<Paste>

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>

* [stable/concourse] add generic secret configurations

With the introduction generic caching, we no longer need the
vault-specific cacheing flags, while at the same time, having the need
of providing new ones (generic).

concourse/concourse#3628

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>

* [stable/concourse] add auditing flags

With the introduction of auditing capabilities (see Auditor#3577 [1]),
it's now possible to configure in a per-subsystem basis which API
requests an installation should audit.

This commit adds the necessary support for such flags.

[1]: concourse/concourse#3577

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>

* [stable/concourse] update image version to 5.2.0

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
Co-authored-by: Krishna Mannem <kmannem@pivotal.io>

* [stable/concourse] Add time based build log retention flags

concourse/concourse#3560

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
Co-authored-by: Krishna Mannem <kmannem@pivotal.io>

* [stable/concourse] add support for externalGardenUrl

With the addition of `external-garden-url` to the `concourse worker`
command, one is now able to reference a non-embedded garden server to be
used as the target for container creation.

concourse/concourse#3806

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
Signed-off-by: Philipp Hellmich <phil@hellmi.de>
rainest added a commit to rainest/charts that referenced this pull request Jun 4, 2019
* [stable/concourse] Update flags for next Concourse release

- Adds `CONCOURSE_CLUSTER_NAME` (concourse.web.clusterName)
  - https://github.com/concourse/concourse/pull/3736<Paste>

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>

* [stable/concourse] add generic secret configurations

With the introduction generic caching, we no longer need the
vault-specific cacheing flags, while at the same time, having the need
of providing new ones (generic).

concourse/concourse#3628

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>

* [stable/concourse] add auditing flags

With the introduction of auditing capabilities (see Auditor#3577 [1]),
it's now possible to configure in a per-subsystem basis which API
requests an installation should audit.

This commit adds the necessary support for such flags.

[1]: concourse/concourse#3577

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>

* [stable/concourse] update image version to 5.2.0

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
Co-authored-by: Krishna Mannem <kmannem@pivotal.io>

* [stable/concourse] Add time based build log retention flags

concourse/concourse#3560

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
Co-authored-by: Krishna Mannem <kmannem@pivotal.io>

* [stable/concourse] add support for externalGardenUrl

With the addition of `external-garden-url` to the `concourse worker`
command, one is now able to reference a non-embedded garden server to be
used as the target for container creation.

concourse/concourse#3806

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
xlucas added a commit to xlucas/charts that referenced this pull request Jun 12, 2019
* [stable/concourse] Update flags for next Concourse release

- Adds `CONCOURSE_CLUSTER_NAME` (concourse.web.clusterName)
  - https://github.com/concourse/concourse/pull/3736<Paste>

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>

* [stable/concourse] add generic secret configurations

With the introduction generic caching, we no longer need the
vault-specific cacheing flags, while at the same time, having the need
of providing new ones (generic).

concourse/concourse#3628

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>

* [stable/concourse] add auditing flags

With the introduction of auditing capabilities (see Auditor#3577 [1]),
it's now possible to configure in a per-subsystem basis which API
requests an installation should audit.

This commit adds the necessary support for such flags.

[1]: concourse/concourse#3577

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>

* [stable/concourse] update image version to 5.2.0

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
Co-authored-by: Krishna Mannem <kmannem@pivotal.io>

* [stable/concourse] Add time based build log retention flags

concourse/concourse#3560

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
Co-authored-by: Krishna Mannem <kmannem@pivotal.io>

* [stable/concourse] add support for externalGardenUrl

With the addition of `external-garden-url` to the `concourse worker`
command, one is now able to reference a non-embedded garden server to be
used as the target for container creation.

concourse/concourse#3806

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
taylorsilva pushed a commit to taylorsilva/concourse-helm that referenced this pull request Oct 2, 2019
* [stable/concourse] Update flags for next Concourse release

- Adds `CONCOURSE_CLUSTER_NAME` (concourse.web.clusterName)
  - https://github.com/concourse/concourse/pull/3736<Paste>

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>

* [stable/concourse] add generic secret configurations

With the introduction generic caching, we no longer need the
vault-specific cacheing flags, while at the same time, having the need
of providing new ones (generic).

concourse/concourse#3628

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>

* [stable/concourse] add auditing flags

With the introduction of auditing capabilities (see Auditor#3577 [1]),
it's now possible to configure in a per-subsystem basis which API
requests an installation should audit.

This commit adds the necessary support for such flags.

[1]: concourse/concourse#3577

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>

* [stable/concourse] update image version to 5.2.0

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
Co-authored-by: Krishna Mannem <kmannem@pivotal.io>

* [stable/concourse] Add time based build log retention flags

concourse/concourse#3560

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
Co-authored-by: Krishna Mannem <kmannem@pivotal.io>

* [stable/concourse] add support for externalGardenUrl

With the addition of `external-garden-url` to the `concourse worker`
command, one is now able to reference a non-embedded garden server to be
used as the target for container creation.

concourse/concourse#3806

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.