From 2133763ceb1d38fa502e5bbf18dfb23318a11a75 Mon Sep 17 00:00:00 2001 From: Pradeep Kumar Date: Tue, 24 May 2022 16:42:51 +0530 Subject: [PATCH] move prune commands to single container Issue : with older apporach pruner was crating one container for one namespace which says 100 container for 100 namespaces Fix : We need to think of a proper appraoch to tackel this but for the time being moving all the prune commands to one container Signed-off-by: Pradeep Kumar --- pkg/reconciler/common/prune.go | 67 +++++++++++++++++------------ pkg/reconciler/common/prune_test.go | 46 ++++++++++---------- 2 files changed, 63 insertions(+), 50 deletions(-) diff --git a/pkg/reconciler/common/prune.go b/pkg/reconciler/common/prune.go index b62cffba9d..7c96bdab8c 100644 --- a/pkg/reconciler/common/prune.go +++ b/pkg/reconciler/common/prune.go @@ -128,7 +128,7 @@ func Prune(ctx context.Context, k kubernetes.Interface, tC *v1alpha1.TektonConfi return err } if pruningNamespaces.commonScheduleNs != nil && len(pruningNamespaces.commonScheduleNs) > 0 { - jobs := pruner.createAllJobContainers(pruningNamespaces.commonScheduleNs) + jobs := getJobContainer(generateAllPruneConfig(pruningNamespaces.commonScheduleNs), pruner.targetNamespace, pruner.tknImage) if err := pruner.checkAndCreate(ctx, "", tC.Spec.Pruner.Schedule, jobs, pruneCronLabel, pruningNamespaces.configChanged); err != nil { logger.Error("failed to create cronjob ", err) } @@ -136,7 +136,7 @@ func Prune(ctx context.Context, k kubernetes.Interface, tC *v1alpha1.TektonConfi if pruningNamespaces.uniqueScheduleNS != nil { for ns, con := range pruningNamespaces.uniqueScheduleNS { - jobs := pruner.createJobContainers(con, ns) + jobs := getJobContainer(generatePruneConfigPerNamespace(con, ns), ns, pruner.tknImage) listOpt := pruneCronNsLabel + "=" + ns if err := pruner.checkAndCreate(ctx, ns, con.config.Schedule, jobs, listOpt, pruningNamespaces.configChanged); err != nil { logger.Error("failed to create cronjob ", err) @@ -240,30 +240,26 @@ func prunableNamespaces(ctx context.Context, k kubernetes.Interface, defaultPrun return prunableNs, nil } -func (pruner *Pruner) createAllJobContainers(nsConfig map[string]*pruneConfigPerNS) []corev1.Container { - var containers []corev1.Container +func generateAllPruneConfig(nsConfig map[string]*pruneConfigPerNS) string { + var cmds string for ns, con := range nsConfig { - jobContainers := pruner.createJobContainers(con, ns) - containers = append(containers, jobContainers...) + cmd := generatePruneConfigPerNamespace(con, ns) + cmds = cmds + " " + cmd } - return containers + return cmds } -func (pruner *Pruner) createJobContainers(nsConfig *pruneConfigPerNS, ns string) []corev1.Container { - var containers []corev1.Container - - cmdArgs := pruneCommand(nsConfig, ns) +func getJobContainer(cmdArgs, ns string, tknImage string) []corev1.Container { + cmd := "function prune() { n=$1; a=$2; resources=$3; old_ifs=\" \"; IFS=\",\"; for r in $resources; do tkn $r delete -n=$n $a -f; done; IFS=$old_ifs; }; echo $conf; for c in $*; do ns=$(echo $c | cut -d \";\" -f 1); args=$(echo $c | cut -d \";\" -f 2); resources=$(echo $c | cut -d \";\" -f 3); prune $ns $args $resources; done;" containerName := SimpleNameGenerator.RestrictLengthWithRandomSuffix("pruner-tkn-" + ns) container := corev1.Container{ Name: containerName, - Image: pruner.tknImage, - Command: []string{"/bin/sh", "-c"}, - Args: []string{cmdArgs}, + Image: tknImage, + Command: []string{"/bin/sh", "-c", cmd}, + Args: []string{"-s", cmdArgs}, TerminationMessagePolicy: "FallbackToLogsOnError", } - containers = append(containers, container) - - return containers + return []corev1.Container{container} } func (pruner *Pruner) checkAndCreate(ctx context.Context, uniquePruneNs, schedule string, pruneContainers []corev1.Container, listOpt string, configChanged bool) error { @@ -344,20 +340,35 @@ func createCronJob(ctx context.Context, kc kubernetes.Interface, cronName, targe return nil } -func pruneCommand(pru *pruneConfigPerNS, ns string) string { - var cmdArgs string - for _, resource := range pru.config.Resources { +// generatePruneConfigPerNamespace takes config `pruneConfigPerNS` per namespace and return strings like +// ns-one;--keep=5;pipelinerun +// ns-two;--keep=3;pipelinerun,taskrun +// ns-three;--keep=2;taskrun +// these configs are passed as space seperated string argument to pod container spec +// for each of this namespaced config below commands are generated +// tkn pipelinerun delete --keep=5 -n=ns-one -f ; +// tkn pipelinerun delete --keep=3 -n=ns-two -f ; +// tkn taskrun delete --keep=3 -n=ns-two -f ; +// tkn taskrun delete --keep=3 -n=ns-three -f ; +func generatePruneConfigPerNamespace(pru *pruneConfigPerNS, ns string) string { + var keepCmd string + if pru.config.Keep != nil { + keepCmd = "--keep=" + fmt.Sprint(*pru.config.Keep) + } + if pru.config.Keep == nil && pru.config.KeepSince != nil { + keepCmd = "--keep-since=" + fmt.Sprint(*pru.config.KeepSince) + } + cmdArgs := ns + ";" + keepCmd + ";" + var resources string + for i, resource := range pru.config.Resources { res := strings.TrimSpace(resource) - var keepCmd string - if pru.config.Keep != nil { - keepCmd = "--keep=" + fmt.Sprint(*pru.config.Keep) - } - if pru.config.Keep == nil && pru.config.KeepSince != nil { - keepCmd = "--keep-since=" + fmt.Sprint(*pru.config.KeepSince) + if i < len(pru.config.Resources)-1 { + resources = resources + res + "," + } else { + resources = resources + res } - cmd := "tkn " + strings.ToLower(res) + " delete " + keepCmd + " -n=" + ns + " -f ; " - cmdArgs = cmdArgs + cmd } + cmdArgs = cmdArgs + resources return cmdArgs } diff --git a/pkg/reconciler/common/prune_test.go b/pkg/reconciler/common/prune_test.go index b7072bd4a7..ee3eb3b57f 100644 --- a/pkg/reconciler/common/prune_test.go +++ b/pkg/reconciler/common/prune_test.go @@ -19,7 +19,6 @@ package common import ( "context" "errors" - "fmt" "os" "strings" "testing" @@ -141,8 +140,8 @@ func TestPruneCommands(t *testing.T) { keep := uint(2) keepsince := uint(300) expected := []string{ - "tkn pipelinerun delete --keep=2 -n=ns -f ; tkn taskrun delete --keep=2 -n=ns -f ; ", - "tkn pipelinerun delete --keep-since=300 -n=ns -f ; tkn taskrun delete --keep-since=300 -n=ns -f ; ", + "ns;--keep=2;pipelinerun,taskrun", + "ns;--keep-since=300;pipelinerun,taskrun", } ns := "ns" configs := []*pruneConfigPerNS{ @@ -165,7 +164,7 @@ func TestPruneCommands(t *testing.T) { } for i, config := range configs { - cmd := pruneCommand(config, ns) + cmd := generatePruneConfigPerNamespace(config, ns) assert.Equal(t, cmd, expected[i]) } } @@ -210,21 +209,11 @@ func TestAnnotationCmd(t *testing.T) { &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "ns-ten", Annotations: annoResourceTrPr}}, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "ns-thirteen", Annotations: annoStrategyKeep}}, ) - expected := map[string]string{ - "one-" + scheduleCommon: "tkn pipelinerun delete --keep=3 -n=ns-one -f ; ", - "two-" + scheduleUnique: "tkn pipelinerun delete --keep=3 -n=ns-two -f ; ", - "four-" + scheduleCommon: "tkn pipelinerun delete --keep=3 -n=ns-four -f ; ", - "seven-" + scheduleCommon: "tkn pipelinerun delete --keep-since=3200 -n=ns-seven -f ; ", - "eight-" + scheduleCommon: "tkn pipelinerun delete --keep=5 -n=ns-eight -f ; ", - "nine-" + scheduleCommon: "tkn taskrun delete --keep=3 -n=ns-nine -f ; ", - "ten-" + scheduleCommon: "tkn taskrun delete --keep=3 -n=ns-ten -f ; tkn pipelinerun delete --keep=3 -n=ns-ten -f ; ", - "ns-thirteen" + scheduleCommon: "tkn pipelinerun delete --keep=50 -n=ns-thirteen -f ; ", - } os.Setenv(JobsTKNImageName, "some") err := Prune(context.TODO(), client, config) if err != nil { - assert.Error(t, err, "unable to get ns list") + assert.Error(t, err, "pruning failed ") } cronjobs, err := client.BatchV1().CronJobs("").List(context.TODO(), metav1.ListOptions{}) if err != nil { @@ -232,18 +221,31 @@ func TestAnnotationCmd(t *testing.T) { } // Only one ns with unique schedule than default if len(cronjobs.Items) != 2 { - assert.Error(t, err, "unable to get ns list") + assert.Error(t, err, "number of cronjobs created is not right") + } + expected := map[string]struct{}{ + "ns-two;--keep=3;pipelinerun": {}, + "ns-eight;--keep=5;pipelinerun": {}, + "ns-four;--keep=3;pipelinerun": {}, + "ns-nine;--keep=3;taskrun": {}, + "ns-one;--keep=3;pipelinerun": {}, + "ns-seven;--keep-since=3200;pipelinerun": {}, + "ns-ten;--keep=3;taskrun,pipelinerun": {}, + "ns-thirteen;--keep=50;pipelinerun": {}, } for _, cronjob := range cronjobs.Items { + assert.Equal(t, len(cronjob.Spec.JobTemplate.Spec.Template.Spec.Containers), 1) for _, container := range cronjob.Spec.JobTemplate.Spec.Template.Spec.Containers { - if _, ok := expected[container.Name[14:len(container.Name)-5]+cronjob.Spec.Schedule]; ok { - if expected[container.Name[14:len(container.Name)-5]+cronjob.Spec.Schedule] != strings.Join(container.Args, " ") { - msg := fmt.Sprintf("expected : %s\n actual : %s \n", expected[container.Name[14:len(container.Name)-5]+cronjob.Spec.Schedule], strings.Join(container.Args, " ")) - assert.Error(t, errors.New("command created is not as expected"), msg) + args := strings.Split(container.Args[1], " ") + if len(args) == 1 { + if _, ok := expected[args[0]]; !ok { + assert.Error(t, err, "expected args not found") } } - if container.Name == "ns-six" { - assert.Error(t, errors.New("Should not be created as Ns have skip prune annotation"), "") + for _, arg := range args[1:] { + if _, ok := expected[arg]; !ok { + assert.Error(t, errors.New("not found"), "expected command not found") + } } } }