Clean histories for successful and failed jobs separately

[bastjan]

Summary

This PR allows cleaning histories for successful and failed jobs separately.

It introduces two new fields to the job CRDs .spec.successfulJobsHistoryLimit and .spec.failedJobsHistoryLimit. Those fields override the now deprecated .spec.KeepJobs.

It fixes a bug where most job cleanups would result in always cleaning up the last object, due to the misuse of a pointer in a for range loop:

https://github.com/vshn/k8up/blob/df7435b28e9292ef88c582bce9aa64bb69185168/executor/check.go#L115-L117

Fixes #322.

Checklist

• Keep pull requests small so they can be easily reviewed. Sadly no 🙈
• Categorize the PR by setting a good title and adding one of the labels:
  bug, enhancement, documentation, change, breaking,
  as they show up in the changelog
• Update the documentation.
• Update tests.
• Link this PR to related issues.

[ccremer]

This is a first review and I'm not through yet. Overall good, but I don't see much changes in the documentation. Could you double check whether the new variables and behaviour is updated in the docs?

[bastjan]

This is a first review and I'm not through yet. Overall good, but I don't see much changes in the documentation. Could you double check whether the new variables and behaviour is updated in the docs?

Thanks for the review 😊

The documentation is auto generated here: docs/modules/ROOT/pages/references/api-reference.adoc. Any other place needed? 😊

[ccremer]

The documentation is auto generated here: docs/modules/ROOT/pages/references/api-reference.adoc. Any other place needed? blush

Not everything is automated :)
Please do a local preview and go through all pages where documenting the new behaviour could make sense. I recall there's a config reference page where the env variables are documented. There definitely BACKUP_KEEPJOBS needs to be more thoroughly documented/deprecated. You might also find code examples and add the new variables there and then.

[Kidswiss]

@bastjan

Sorry to barge in like that. But this caught my eye:

It fixes a bug where most job cleanups would result in always cleaning up the last object, due to the misuse of a pointer in a for range loop:

It would fix this case, too: #465, right?

Just opened that one a few hours ago :D

[bastjan]

@bastjan

It would fix this case, too: #465, right?

Just opened that one a few hours ago :D

I can't yet prove it, but I do think so yes 😊

[ccremer]

I ran the integration tests locally and get some error output here. It doesn't fail the tests (exit code 0) but I'm not sure if that's something as expected:

=== CONT  Test_Check/TestJobCleanup
    check_integration_test.go:105: 2 deleted Checks found
=== RUN   Test_Check/TestReconciliation
    envsuite_test.go:147: creating namespace 'x7vs977v'
    envsuite_test.go:154: creating resource 'x7vs977v/check-integration-test0'
=== CONT  Test_Check
    logger.go:130: 2021-07-26T18:53:20.936+0200	DEBUG	adding job to the queue	{"check": "x7vs977v/check-integration-test0"}
    logger.go:130: 2021-07-26T18:53:20.948+0200	DEBUG	adding job to the queue	{"check": "x7vs977v/check-integration-test0"}
=== CONT  Test_Check/TestReconciliation
    check_integration_test.go:169: 1 Jobs found
=== CONT  Test_Check
    logger.go:130: 2021-07-26T18:53:22.349+0200	ERROR	could not patch status conditions	{"check": "x7vs977v/check-integration-test0", "error": "Patch \"http://127.0.0.1:36099/apis/backup.appuio.ch/v1alpha1/namespaces/x7vs977v/checks/check-integration-test0/status\": dial tcp 127.0.0.1:36099: connect: connection refused"}
    logger.go:130: 2021-07-26T18:53:22.349+0200	ERROR	cannot create job	{"check": "x7vs977v/check-integration-test0", "repository": "s3:/", "error": "Post \"http://127.0.0.1:36099/apis/batch/v1/namespaces/x7vs977v/jobs\": dial tcp 127.0.0.1:36099: connect: connection refused"}
--- PASS: Test_Check (14.69s)
    --- PASS: Test_Check/TestJobCleanup (7.17s)
    --- PASS: Test_Check/TestReconciliation (1.03s)

[ccremer]

Please have another look at the documentation

• https://github.com/vshn/k8up/blob/7b47bcd9cc4a81576f8b0f95f0b3f4594b457681/docs/modules/ROOT/pages/references/object-specifications.adoc: We make still use of the deprecated KeepJobs field. Probably should remove those and replace with newer example.
• https://github.com/vshn/k8up/blob/7b47bcd9cc4a81576f8b0f95f0b3f4594b457681/docs/modules/ROOT/pages/references/status.adoc: Please check if the status and conditions are up-to-date
• https://github.com/vshn/k8up/blob/improve-monitoring/config/samples/k8up_v1alpha1_schedule.yaml#L41: Update the sample

Basically, a grep -r -i "keepjobs" . and see where it needs updating

[bastjan]

Please have another look at the documentation

• https://github.com/vshn/k8up/blob/7b47bcd9cc4a81576f8b0f95f0b3f4594b457681/docs/modules/ROOT/pages/references/object-specifications.adoc: We make still use of the deprecated KeepJobs field. Probably should remove those and replace with newer example.
• https://github.com/vshn/k8up/blob/7b47bcd9cc4a81576f8b0f95f0b3f4594b457681/docs/modules/ROOT/pages/references/status.adoc: Please check if the status and conditions are up-to-date
• https://github.com/vshn/k8up/blob/improve-monitoring/config/samples/k8up_v1alpha1_schedule.yaml#L41: Update the sample

Basically, a grep -r -i "keepjobs" . and see where it needs updating

Thanks 😊 I'm still working on the documentation👍

[bastjan]

I ran the integration tests locally and get some error output here. It doesn't fail the tests (exit code 0) but I'm not sure if that's something as expected:

I have the same issues on master. Did not yet take a look at the root cause.

[corvus-ch]

Looking good now. I left some remarks. Those are general thoughts and I do not see a need for a change.