feat: add log retention for database logger #8622

erikwilson · 2023-12-29T19:51:45Z

Description

Adds log retention for database logger.

Test Plan

Automated tests added.

Manual testing:

Deploy / start a cluster with this change.
Deploy an hpsearch experiment configured with a log retention of 0 days.
Run the log cleanup command to ensure the logs are removed.
Modify the server configuration to use a short duration or cron expression for log cleanup.
Restart the server.
Start another hpsearch experiment with default configuration.
Ensure logs are retained between cleanup cycles.
Update experiment to have log retention of 0 days.
Ensure that logs are removed on next cleanup cycle.

Commentary (optional)

Checklist

Changes have been manually QA'd
User-facing API changes need the "User-facing API Change" label.
Release notes should be added as a separate file under docs/release-notes/.
See Release Note for details.
Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

DET-9995

netlify · 2023-12-29T19:51:50Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`ee1e140`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/6604ab72610d1a0008b899fd
😎 Deploy Preview	https://deploy-preview-8622--determined-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

master/pkg/model/logging_config.go

master/internal/api_master.go

master/internal/core.go

master/pkg/model/logging_config.go

master/internal/rm/agentrm/agent.go

master/internal/db/postgres_tasks.go

erikwilson · 2024-01-09T22:03:58Z

For reference we had a discussion about the golang duration & postgres interval stuff, and decided to keep duration as days. This is because golang duration parsing does not support units larger than hours, we would need to create or find a wrapper for postgres support, and postgres interval math is complex and could be problematic. Here is a blog post with some more info:
https://blog.bossylobster.com/2021/10/go-duration-postgresql-interval.html

codecov · 2024-01-09T22:53:19Z

Codecov Report

Attention: Patch coverage is 56.15385% with 57 lines in your changes are missing coverage. Please review.

Project coverage is 47.17%. Comparing base (8f5de35) to head (ee1e140).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8622      +/-   ##
==========================================
+ Coverage   47.14%   47.17%   +0.03%     
==========================================
  Files        1150     1152       +2     
  Lines      141671   141773     +102     
  Branches     2415     2417       +2     
==========================================
+ Hits        66785    66888     +103     
+ Misses      74696    74695       -1     
  Partials      190      190

Flag	Coverage Δ
backend	`43.00% <59.45%> (+0.14%)`	⬆️
harness	`64.31% <36.84%> (-0.02%)`	⬇️
web	`38.96% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
master/internal/checkpoint_gc.go	`66.30% <100.00%> (+0.37%)`	⬆️
master/internal/command/command.go	`67.96% <100.00%> (+0.13%)`	⬆️
master/internal/config/config.go	`73.57% <100.00%> (-0.22%)`	⬇️
master/internal/core_experiment.go	`61.50% <100.00%> (+0.34%)`	⬆️
master/internal/db/postgres_tasks.go	`68.86% <100.00%> (+0.11%)`	⬆️
master/internal/trial.go	`41.58% <100.00%> (+0.11%)`	⬆️
master/pkg/model/task.go	`15.26% <ø> (ø)`
master/pkg/schemas/expconf/experiment_config.go	`64.21% <ø> (ø)`
master/pkg/schemas/zgen_schemas.go	`1.24% <ø> (ø)`
master/pkg/tasks/task.go	`6.07% <ø> (ø)`
... and 9 more

... and 5 files with indirect coverage changes

stoksc

talked offline but leaving this review still just incase i lost something

stoksc · 2024-01-22T14:36:58Z

.circleci/devcluster/custom-agent-version.devcluster.yaml

@@ -34,7 +34,7 @@ stages:
            set -ex
            mkdir -p /tmp/det
            cat >/tmp/det/agent.yaml <<EOF
-            master_host: 127.0.0.1
+            master_host: host.docker.internal


don't forget to revert this when you land?

master/internal/api_master.go

stoksc · 2024-01-22T14:44:20Z

master/internal/checkpoint_gc.go

+		StartTime:        time.Now().UTC(),
+		JobID:            &jobID,
+		LogVersion:       model.CurrentTaskLogVersion,
+		LogRetentionDays: taskSpec.LogRetentionDays,


can you leave a really good comment on the field about how nulls work, etc?

i'm coming back to this review after a while and thought: what happens if the admin wants to retain most of the logs for a lot shorter time and sets the cluster wide retention time to a week from a month, and what does this field mean, then?

I tried to add some comments to this method:
https://github.com/determined-ai/determined/pull/8622/files#diff-0aff34e1ab2c0076df8608480773216fd6b78a64d46d3f485aee6eb7586659faR87-R89

Please let me know if you think it needs more clarification or comments.

master/internal/core.go

stoksc · 2024-01-22T15:12:05Z

master/internal/db/postgres_tasks.go

+	r, err := db.sql.Exec(fmt.Sprintf(`
+WITH log_retention_tasks AS (
+	SELECT task_id, end_time, COALESCE(log_retention_days, %d) AS current_retention_days FROM tasks
+	WHERE task_id IN (SELECT DISTINCT task_id FROM task_logs)


is this a minor optimization to not try to delete from tasks without logs, or more than that?

correct, it seems like this would help if we have a database with a very large number of tasks but fewer task logs, which is likely to be the situation once retention is enabled.

this might be a good candidate for scale testing & optimization tho.

master/internal/db/postgres_tasks.go

master/internal/logretention/logretention.go

master/pkg/model/logging_config.go

wes-turner · 2024-02-03T01:09:44Z

docs/reference/training/experiment-config-reference.rst

+
+.. code:: yaml
+
+   log_retention_days: 90


It introduces a bit of complexity to have differing syntax for whether you're setting the default retention policy in the master or setting the retention policy for this one particular experiment. I think either syntax works for me, I just don't want to have to remember both and keep them straight.

Checking in, is this the desired choice? For comparison, log_policies has both the same syntax and same name in both master config and experiment config.

The names are somewhat unfortunate but due to collisions with log/logging naming in the configs this seems like the best compromise. The server can also take a schedule and default number of days for retention, while the experiment can only configure the number of days and not the schedule.

docs/reference/deploy/master-config-reference.rst

hkumar92 · 2024-02-20T22:14:35Z

@erikwilson this does not support the CLI or WebUI modifications of retention on individual experiments right? Is there a reason that wasn't done?

erikwilson · 2024-02-20T22:18:26Z

@erikwilson this does not support the CLI or WebUI modifications of retention on individual experiments right? Is there a reason that wasn't done?

This does contain some of the CLI modifications, the rest of the changes can be added in another PR. For the most part we have tried to separate backend and WebUI PRs, as they have historically been done by different teams and developers.

hkumar92 · 2024-02-20T22:28:20Z

Thanks! One concern here is whether the CLI/WebUI/SDK can modify the retention periods for experiments once submitted? If not, I'd rather have a simpler interface where all you can configure for an experiment is whether it is subject to log deletion or not (and we can decide if we need to give users more control later).

jgongd · 2024-03-26T15:20:52Z

harness/determined/cli/master.py

@@ -119,6 +124,9 @@ def logs(args: Namespace) -> None:
                help="number of lines to show, counting from the end "
                "of the log (default is all)")
        ]),
+        Cmd("cleanup-logs", cleanup_logs, "cleanup expired task logs", [
+            Group(cli.output_format_args["json"], cli.output_format_args["yaml"])


It may not be necessary to have Group(cli.output_format_args["json"], cli.output_format_args["yaml"]) as cleanup_logs() prints one line.

Could we relocate this command to task.py, since this is related to task?

Removed the yaml/json options. But I think this should stay in master because that's what product wanted

docs/reference/experiment-config-reference.rst

jgongd

LGTM

cla-bot bot added the cla-signed label Dec 29, 2023

erikwilson requested a review from stoksc December 29, 2023 19:52

stoksc reviewed Jan 3, 2024

View reviewed changes

erikwilson force-pushed the erikw/feat/log-retention branch from 5c263ed to 518963d Compare January 9, 2024 22:50

erikwilson force-pushed the erikw/feat/log-retention branch 2 times, most recently from fc8b731 to 78625da Compare January 16, 2024 21:47

erikwilson force-pushed the erikw/feat/log-retention branch from cd1e9f2 to 10767f9 Compare January 17, 2024 23:43

stoksc self-assigned this Jan 23, 2024

stoksc reviewed Jan 23, 2024

View reviewed changes

determined-ci requested a review from a team January 30, 2024 04:16

determined-ci added the documentation Improvements or additions to documentation label Jan 30, 2024

erikwilson force-pushed the erikw/feat/log-retention branch from 3a6bb1a to 7874b8a Compare January 30, 2024 18:02

erikwilson marked this pull request as ready for review January 30, 2024 20:55

erikwilson requested review from a team as code owners January 30, 2024 20:55

erikwilson requested review from wes-turner, stoksc and tara-det-ai January 30, 2024 20:55

erikwilson force-pushed the erikw/feat/log-retention branch from f5e91cb to 147bb54 Compare February 2, 2024 17:01

wes-turner reviewed Feb 3, 2024

View reviewed changes

dzhu reviewed Feb 5, 2024

View reviewed changes

docs/reference/deploy/master-config-reference.rst Outdated Show resolved Hide resolved

docs/reference/deploy/master-config-reference.rst Outdated Show resolved Hide resolved

erikwilson force-pushed the erikw/feat/log-retention branch from c97db92 to d816d35 Compare February 20, 2024 17:34

determined-ci requested a review from a team February 20, 2024 17:34

erikwilson assigned salonig23 Feb 20, 2024

erikwilson requested a review from hamidzr March 13, 2024 17:22

salonig23 force-pushed the erikw/feat/log-retention branch from 69839b2 to 6210319 Compare March 14, 2024 21:15

erikwilson force-pushed the erikw/feat/log-retention branch from 6210319 to 3fb43da Compare March 14, 2024 22:47

salonig23 force-pushed the erikw/feat/log-retention branch 7 times, most recently from 6451894 to 6cf216e Compare March 21, 2024 18:24

erikwilson requested a review from a team as a code owner March 21, 2024 18:24

erikwilson requested a review from jgongd March 21, 2024 18:24

salonig23 force-pushed the erikw/feat/log-retention branch from 6cf216e to 76f85f0 Compare March 25, 2024 18:40

jgongd reviewed Mar 26, 2024

View reviewed changes

docs/reference/experiment-config-reference.rst Outdated Show resolved Hide resolved

salonig23 force-pushed the erikw/feat/log-retention branch from 76f85f0 to 3524b9a Compare March 26, 2024 17:56

salonig23 requested a review from jgongd March 26, 2024 17:57

salonig23 force-pushed the erikw/feat/log-retention branch from 3524b9a to ad0d3cc Compare March 26, 2024 18:58

jgongd approved these changes Mar 26, 2024

View reviewed changes

salonig23 force-pushed the erikw/feat/log-retention branch 3 times, most recently from da7f1dc to 89c07fb Compare March 26, 2024 23:43

erikwilson and others added 5 commits March 27, 2024 16:26

feat: add task log retention

b8e61d1

update from comments

a3dd454

move migration

285ca56

fix for tests

66ed57f

test changes and docs edits

ee1e140

salonig23 force-pushed the erikw/feat/log-retention branch from 89c07fb to ee1e140 Compare March 27, 2024 23:27

salonig23 merged commit 85bb3c8 into main Mar 28, 2024
90 of 101 checks passed

salonig23 deleted the erikw/feat/log-retention branch March 28, 2024 00:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add log retention for database logger #8622

feat: add log retention for database logger #8622

erikwilson commented Dec 29, 2023 •

edited

Loading

netlify bot commented Dec 29, 2023 •

edited

Loading

erikwilson commented Jan 9, 2024 •

edited

Loading

codecov bot commented Jan 9, 2024 •

edited

Loading

stoksc left a comment

stoksc Jan 22, 2024

stoksc Jan 22, 2024

erikwilson Feb 20, 2024

stoksc Jan 22, 2024

erikwilson Jan 24, 2024

wes-turner Feb 3, 2024

erikwilson Feb 20, 2024

hkumar92 commented Feb 20, 2024

erikwilson commented Feb 20, 2024

hkumar92 commented Feb 20, 2024

jgongd Mar 26, 2024

jgongd Mar 26, 2024

salonig23 Mar 26, 2024

jgongd left a comment

feat: add log retention for database logger #8622

feat: add log retention for database logger #8622

Conversation

erikwilson commented Dec 29, 2023 • edited Loading

Description

Test Plan

Commentary (optional)

Checklist

Ticket

netlify bot commented Dec 29, 2023 • edited Loading

✅ Deploy Preview for determined-ui ready!

erikwilson commented Jan 9, 2024 • edited Loading

codecov bot commented Jan 9, 2024 • edited Loading

Codecov Report

stoksc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkumar92 commented Feb 20, 2024

erikwilson commented Feb 20, 2024

hkumar92 commented Feb 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgongd left a comment

Choose a reason for hiding this comment

erikwilson commented Dec 29, 2023 •

edited

Loading

netlify bot commented Dec 29, 2023 •

edited

Loading

erikwilson commented Jan 9, 2024 •

edited

Loading

codecov bot commented Jan 9, 2024 •

edited

Loading