adds auto-prune per namespace #410

pradeepitm12 · 2021-09-16T13:21:44Z

This adds auto-pruning of taskrun and pipeline run per namespace
configured specifically.
adding unique schedule create a new cronjob in the annotated
namespace.
The default behaviour will persist, on top of it now user can
annotate a namespace with unique pruning configuration.
If user configures a unique schedule value by annotating a ns
a new cron job in that namespace will be created.

Auto purning is now enabled by default for default configurations
from tektonconfig.

Annotations introduced with this pr
- "operator.tekton.dev/prune.skip"
- "operator.tekton.dev/prune.resources"
- "operator.tekton.dev/prune.strategy"
- "operator.tekton.dev/prune.keep"
- "operator.tekton.dev/prune.keep-since"
- "operator.tekton.dev/prune.schedule"

Signed-off-by: Pradeep Kumar [email protected]

Changes

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Release Notes

1) Introduce resource deletion as per the age.

keep-since newly introduced to tektonconfig, this is used to delete resources on the basis of their age.

     user wants to keep resources that are younger than 5 days.
     keep-since: 7200

Note: keep and keep-since are mutually exclusive. ie only one should be configured.

2) Introduce pruning config for each namespace.

With this pr user would be able to configure pruning uniquely for each namespace.
This can be achieved by annotation on the namespace.
The annotations introduced are as follows

"operator.tekton.dev/prune.skip"
- if set as true, this particular namespace will be skipped for pruning
"operator.tekton.dev/prune.resources"
- it can take the comma-separated value "taskrun, pipelinerun" if user wants to prune both resources, or a single resource "pipelinerun". This is similar to what we have in tektonconfig, only diff is it is a comma-separated string at the annotation level.
"operator.tekton.dev/prune.strategy"
- strategy can have two values, "keep" or "keep-since"
- The value for the strategy should be specified with below two annotations.
"operator.tekton.dev/prune.keep"
- value for keep
"operator.tekton.dev/prune.keep-since"
- value for keep-since
"operator.tekton.dev/prune.schedule"
- If the value of the schedule is different than the value specified at the tektonconfig a new cron job in that particular namespace is created.

tekton-robot · 2021-09-16T13:24:49Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/operator/v1alpha1/tektonconfig_validation.go	89.7%	83.9%	-5.8
pkg/reconciler/common/prune.go	52.9%	77.8%	24.8

tekton-robot · 2021-09-16T13:49:08Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/operator/v1alpha1/tektonconfig_validation.go	89.7%	90.3%	0.7
pkg/reconciler/common/prune.go	52.9%	77.8%	24.8

pkg/apis/operator/v1alpha1/tektonconfig_validation.go

pkg/reconciler/openshift/tektonconfig/controller.go

pkg/reconciler/common/prune.go

nikhil-thomas · 2021-09-20T12:05:51Z

pkg/reconciler/common/prune.go

+	var containers []corev1.Container
+
+	cmdArgs := pruneCommand(nsConfig, ns)
+	jobName := SimpleNameGenerator.RestrictLengthWithRandomSuffix("pruner-tkn-" + ns)


let us avoid the namespace suffix. it is redundant data and it doesn't help us with any functional benefits.

Im assuming that this was done to check a cronjob was created by this pruner logic. For that, let us add a label on the CornJob (example tektonconfig.operator.tekton.dev/pruner: true)

This will be more readable. It will also help us filter our query for cronJobs from allnamespaces.

In addition, we will create only 1 pruner CronJob in any given namespace.

nikhil-thomas · 2021-09-20T12:07:12Z

pkg/reconciler/common/prune.go

-	ownerAPIVer      = "operator.tekton.dev/v1alpha1"
-	ownerKind        = "TektonConfig"
+	tektonSA                    = "tekton-pipelines-controller"
+	CronName                    = "resource-pruner"


let us make this tektoncd-resorce-pruner

This could be done. I am only thinking of updates, is there any chance the cronjob name change will create any problem with the updates?

Do we put annotation on the cronjob we create ? If yes, then we could/should probably not rely on the name, and thus changing it now wouldn't be a problem.
That said, where is this name use : inside openshift-pipelines (then it's fine to not have tektoncd prefix I would guess), or in any annotated namespace (then, yes, we should prefix it because we are not pruning all resources, just tektoncd ones).

earlier label/annotation was not there.
We will be adding a label this time. #410 (comment)

pkg/reconciler/common/prune.go

nikhil-thomas · 2021-09-20T12:21:31Z

pkg/reconciler/common/prune.go

@@ -98,11 +222,11 @@ func createCronJob(k kubernetes.Interface, ctx context.Context, pru v1alpha1.Pru
 			APIVersion: "batch/v1beta1",
 		},
 		ObjectMeta: v1.ObjectMeta{
-			Name:            CronName,
+			Name:            CronName + "-" + targetNs,


https://github.com/tektoncd/operator/pull/410/files#r712104343

vdemeester

Is keep: 2 a default we will/would ship ? because if yes, it might be a tidy bit too small.
keep vs keepSince : I tend to agree that initially both "should" not be used together, but it could make sense though : I want to keep 100 pipelineruns and those since 4 weeks, and clean those who do not fit – If I have less than 100 in 4 weeks, I clean those after 4 weeks, if I have more than 100 in 4 weeks, I clean the 100 ; or the opposite, we would have to decide there.

pkg/apis/operator/v1alpha1/tektonconfig_validation.go

vdemeester · 2021-09-21T08:45:07Z

pkg/reconciler/common/prune.go

-	ownerAPIVer      = "operator.tekton.dev/v1alpha1"
-	ownerKind        = "TektonConfig"
+	tektonSA                    = "tekton-pipelines-controller"
+	CronName                    = "resource-pruner"


Do we put annotation on the cronjob we create ? If yes, then we could/should probably not rely on the name, and thus changing it now wouldn't be a problem.
That said, where is this name use : inside openshift-pipelines (then it's fine to not have tektoncd prefix I would guess), or in any annotated namespace (then, yes, we should prefix it because we are not pruning all resources, just tektoncd ones).

vdemeester

I think we should remove the operator.tekton.dev/prune.resources.pipelineNames and operator.tekton.dev/prune.resources.taskNames from this feature / PR for now.

The feature is not present in the cluster-wide CR
It doesn't scale well, and we could probably give the user a better experience. On top of my head, we go with an annotation or a label, that if present on a TaskRun/PipelineRun, we would skip it — something like operator.tekton.dev/prune.ignore or something similar, and because we propagate annotation from pipeline to pipelinerun or task to taskrun, a Task or Pipeline could have this annotation too, to make all execution of it not be pruned.

pkg/reconciler/common/prune.go

pkg/reconciler/shared/tektonconfig/controller.go

vdemeester · 2021-09-21T09:34:17Z

/hold

pkg/reconciler/shared/tektonconfig/controller.go

tekton-robot · 2021-09-30T18:18:47Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/operator/v1alpha1/tektonconfig_validation.go	90.0%	88.2%	-1.8
pkg/reconciler/common/prune.go	52.9%	72.3%	19.4

pkg/reconciler/shared/tektonconfig/controller.go

nikhil-thomas · 2021-10-07T08:43:00Z

pkg/reconciler/shared/tektonconfig/tektonconfig.go

@@ -118,7 +118,7 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, tc *v1alpha1.TektonConfi
 		}
 	}

-	if err := common.Prune(r.kubeClientSet, ctx, tc); err != nil {
+	if err := common.Prune(ctx, r.kubeClientSet, tc); err != nil {
 		logger.Error(err)


we have to report this error in TektonConfig Status.
eg:

operator/pkg/reconciler/shared/tektonconfig/tektonconfig.go

Line 116 in f2113b6

tc.Status.MarkComponentNotReady(fmt.Sprintf("TektonTrigger: %s", err.Error()))

as it is not a resource like Triggers or Pipeline of operator, should we put tekton-resource-pruner
tc.Status.MarkComponentNotReady(fmt.Sprintf("tekton-resource-pruner: %s", err.Error()))

pkg/reconciler/common/prune.go

tekton-robot · 2021-10-07T10:03:44Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/operator/v1alpha1/tektonconfig_validation.go	90.0%	88.2%	-1.8
pkg/reconciler/common/prune.go	52.9%	74.3%	21.4

nikhil-thomas · 2021-10-07T14:06:43Z

/lgtm

This adds auto-pruning of taskrun and pipeline run per namespace configured specifically. adding unique schedule create a new cronjob in the annotated namespace. The default behaviour will persist, on top of it now user can annotate a namespace with unique pruning configuration. If user configures a unique schedule value by annotating a ns a new cron job in that namespace will be created. Auto purning is now enabled by default for default configurations from tektonconfig. Annotations introduced with this pr - "operator.tekton.dev/prune.skip" - "operator.tekton.dev/prune.resources" - "operator.tekton.dev/prune.strategy" - "operator.tekton.dev/prune.keep" - "operator.tekton.dev/prune.keep-since" - "operator.tekton.dev/prune.schedule" Signed-off-by: Pradeep Kumar <[email protected]>

vdemeester · 2021-10-07T14:46:58Z

/hold cancel
@pradeepitm12 needs a rebase

vdemeester · 2021-10-07T14:47:04Z

/hold cancel
@pradeepitm12 needs a rebase

pradeepitm12 · 2021-10-07T14:51:10Z

/hold cancel @pradeepitm12 needs a rebase

rebase Done

tekton-robot · 2021-10-07T14:54:40Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/operator/v1alpha1/tektonconfig_validation.go	90.0%	88.2%	-1.8
pkg/reconciler/common/prune.go	52.9%	74.3%	21.4

pradeepitm12 · 2021-10-07T15:09:33Z

@nikhil-thomas @vdemeester
rebase Done.

nikhil-thomas · 2021-10-08T05:35:36Z

/lgtm

Starting in Tekton Operator v0.58.0, TektonConfig objects must set a value for `keep` or `keepSince` if the pruner is enabled [1]. If Shipwright bootstraps the TektonConfig object and does not set a value for `.spec.pruner.keep` (or `.keepSince`), it risks failing to reconcile the TektonConfig instance or worse [2]. Fixing by setting the `keep` value to match the Tekton operator default. Fixes shipwright-io#231 [1] tektoncd/operator#410 [2] tektoncd/operator#2450

Starting in Tekton Operator v0.58.0, TektonConfig objects must set a value for `keep` or `keepSince` if the pruner is enabled [1]. If Shipwright bootstraps the TektonConfig object and does not set a value for `.spec.pruner.keep` (or `.keepSince`), it risks failing to reconcile the TektonConfig instance or worse [2]. Fixing by setting the `keep` value to match the Tekton operator default. Fixes shipwright-io#231 [1] tektoncd/operator#410 [2] tektoncd/operator#2450 Signed-off-by: Adam Kaplan <[email protected]>

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Sep 16, 2021

tekton-robot requested review from piyush-garg and sm43 September 16, 2021 13:21

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 16, 2021

pradeepitm12 force-pushed the advance-auto-pruning branch from 4bbafef to 1e75476 Compare September 16, 2021 13:45

pradeepitm12 changed the title ~~WIP: adds auto-prune per namespace~~ adds auto-prune per namespace Sep 20, 2021

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 20, 2021

nikhil-thomas reviewed Sep 20, 2021

View reviewed changes

pkg/apis/operator/v1alpha1/tektonconfig_validation.go Show resolved Hide resolved

nikhil-thomas reviewed Sep 20, 2021

View reviewed changes

pkg/reconciler/openshift/tektonconfig/controller.go Show resolved Hide resolved

nikhil-thomas reviewed Sep 20, 2021

View reviewed changes

pkg/reconciler/common/prune.go Outdated Show resolved Hide resolved