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

Support automatic buildrun cleanup #1027

Conversation

raghavbhatnagar96
Copy link
Contributor

@raghavbhatnagar96 raghavbhatnagar96 commented Mar 29, 2022

Based on Ship #28

Core Ship Proposals:

  • Two methods used for automatically deleting buildruns - using limits and TTLs
  • Create two controllers to ensure this functionality
  • Add optional retention parameters in build specifications
  • Delete buildrun if either criteria is met
  • Update build and buildrun README files

Commit Summary:

  • Two controllers have been added - buildrun_ttl_cleanup controller and build_limit_cleanup controller.
    • buildrun_ttl_cleanup controller watches buildruns that are created or updated.
    • build_limit_cleanup controller watches builds that are created or updated and watches buildruns that are updated.
  • Build specifications were extended by adding a retention parameter that has 4 optional fields
    • ttlAfterFailed: buildrun is deleted if the mentioned duration of time has passed and the buildrun has failed.
    • ttlAfterSucceeded: buildrun is deleted if the mentioned duration of time has passed and the buildrun has succeeded.
    • failedLimit: If this limit is exceeded, the oldest failed buildruns are deleted till the limit is satisfied.
    • succeededLimit: If this limit is exceeded, the oldest successful buildruns are deleted till the limit is satisfied.
  • Buildrun specifications were extended by adding a retention parameter that has 2 optional fields
    • ttlAfterFailed: buildrun is deleted if the mentioned duration of time has passed and the buildrun has failed.
    • ttlAfterSucceeded: buildrun is deleted if the mentioned duration of time has passed and the buildrun has succeeded.
  • Tests have been added to ensure that these controllers are working as intended.
  • Documentation has been updated to reflect the changes

Signed-off-by: Pravallika Dindukurthi dindukurthi.pravallika@gmail.com

Changes

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Set a kind label on this PR
  • Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

Introducing support for automatic cleanup by extending build and buildrun specifications. A new optional `retention` section has been introduced in both buildrun and build specifications, that consists of 4 optional fields - `ttlAfterFailed`, `ttlAfterSucceeded`, `failedLimit`, `succeededLimit` in build specifications and 2 optional fields - `ttlAfterFailed`, `ttlAfterSucceeded` - in buildrun specifications.

@openshift-ci openshift-ci bot added the release-note Label for when a PR has specified a release note label Mar 29, 2022
@raghavbhatnagar96 raghavbhatnagar96 added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. labels Mar 29, 2022
@qu1queee qu1queee requested review from SaschaSchwarze0 and qu1queee and removed request for HeavyWombat March 29, 2022 09:39
Copy link
Contributor

@qu1queee qu1queee left a comment

Choose a reason for hiding this comment

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

@raghavbhatnagar96 @Pravalika very nice. @SaschaSchwarze0 and myself pair on this review, pls take a look.

pkg/apis/build/v1alpha1/build_types.go Show resolved Hide resolved
pkg/apis/build/v1alpha1/build_types.go Outdated Show resolved Hide resolved
pkg/reconciler/buildlimitcleanup/controller.go Outdated Show resolved Hide resolved
pkg/reconciler/buildlimitcleanup/controller.go Outdated Show resolved Hide resolved
pkg/reconciler/buildlimitcleanup/controller.go Outdated Show resolved Hide resolved
test/integration/buildrun_cleanup_test.go Outdated Show resolved Hide resolved
test/integration/buildrun_cleanup_test.go Outdated Show resolved Hide resolved
test/integration/buildrun_cleanup_test.go Outdated Show resolved Hide resolved
docs/build.md Outdated Show resolved Hide resolved
docs/buildrun.md Outdated Show resolved Hide resolved
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

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

Two small things.

pkg/reconciler/buildlimitcleanup/controller.go Outdated Show resolved Hide resolved
pkg/reconciler/buildrunttlcleanup/buildrun_ttl_cleanup.go Outdated Show resolved Hide resolved
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

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

And just a few glitches on the integration tests.

test/integration/buildrun_cleanup_test.go Outdated Show resolved Hide resolved
test/integration/buildrun_cleanup_test.go Outdated Show resolved Hide resolved
test/integration/buildrun_cleanup_test.go Outdated Show resolved Hide resolved
test/integration/buildrun_cleanup_test.go Outdated Show resolved Hide resolved
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

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

Thank you @raghavbhatnagar96 and @PravallikaDindukurthi.

I think you should already start to work on unit tests for the predicate function parts that you already extracted into separate code. We can then add them to this PR or add them in a separate one depending on how fast reviews happen.

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 6, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SaschaSchwarze0

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 6, 2022
@adambkaplan adambkaplan added this to In progress in shipwright-dev Apr 6, 2022
@adambkaplan adambkaplan moved this from In progress to Review in progress in shipwright-dev Apr 6, 2022
Copy link
Member

@gabemontero gabemontero left a comment

Choose a reason for hiding this comment

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

API looks OK to me, as do doc.

I minor code debug logging item. I at least won't gate merge on it.

I'll defer tagging until both @otaviof and @adambkaplan take a peek.

@SaschaSchwarze0
Copy link
Member

Thanks @raghavbhatnagar96 for addressing all things. Looking for an lgtm @otaviof @gabemontero.

raghavbhatnagar96 and others added 2 commits April 7, 2022 21:47
…nagement.

Two controllers have been added - buildrun_ttl_cleanup controller and build_limit_cleanup controller.
- buildrun_ttl_cleanup controller watches buildruns that are created or updated.
- build_limit_cleanup controller watches builds that are created or updated and watches buildruns that are updated.

Build specifications were extended by adding a retention parameter that has 4 optional fields
- ttlAfterFailed: buildrun is deleted if the mentioned duration of time has passed and the buildrun has failed.
- ttlAfterSucceeded: buildrun is deleted if the mentioned duration of time has passed and the buildrun has succeeded.
- failedLimit: If this limit is exceeded, the oldest failed buildruns are deleted till the limit is satisfied.
- succeededLimit: If this limit is exceeded, the oldest successful buildruns are deleted till the limit is satisfied.

Tests have been added to ensure that these controllers are working as intended.

Signed-off-by: Pravallika Dindukurthi <dindukurthi.pravallika@gmail.com>
Signed-off-by: Raghav Bhatnagar <raghavbhatnagar96@gmail.com>
Signed-off-by: Pravallika Dindukurthi <dindukurthi.pravallika@gmail.com>
Copy link
Member

@otaviof otaviof left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2022
@openshift-merge-robot openshift-merge-robot merged commit bcff280 into shipwright-io:main Apr 8, 2022
shipwright-dev automation moved this from Review in progress to Done Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Label for when a PR has specified a release note
Projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants