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

Allow to store logs in files #3568

Merged
merged 22 commits into from
Jun 6, 2024
Merged

Conversation

qwerty287
Copy link
Contributor

@qwerty287 qwerty287 commented Mar 29, 2024

@qwerty287 qwerty287 added server feature add new functionality labels Mar 29, 2024
@qwerty287
Copy link
Contributor Author

In it's current state, this allows to store logs in files on the regular filesystem. Do we need native support for something like S3 or can we just ask users to mount it directly into the FS?

@zc-devs
Copy link
Contributor

zc-devs commented May 4, 2024

Do we need native support for something like S3 or can we just ask users to mount it directly into the FS?

in object storage

Native S3.

  1. I believe, environments exist, where you cannot mount arbitrary FUSE (AWS EKS, Windows, I guess).
  2. I'm not sure if FUSE S3 can patch a file. If can't, then this is not well from performance and maybe costs perspective.

Sadly, there is no description in the issue, but I suppose it is related to growing Database. If so, tiering across storage mediums could be implemented.

  1. Some bunch of data stores in DB.
  2. After some retention condition old data archives to S3 (Delete old pipeline logs after X days or Y new runs #1068).

That said, this PR adds a value, therefore it can be merged, but without closing the issue.

@qwerty287
Copy link
Contributor Author

Sadly, there is no description in the issue, but I suppose it is related to growing Database.

Yes, I also think so.

Sadly, there is no description in the issue, but I suppose it is related to #3506 (comment). If so, tiering across storage mediums could be implemented.

  1. Some bunch of data stores in DB.
  2. After some retention condition old data archives to S3 (Delete old pipeline logs after X days or Y new runs #1068).

I get your point, but this looks pretty complicated instead of just deleting the pipelines/logs what is the idea in the linked issue.

That said, this PR adds a value, therefore it can be merged, but without closing the issue.

Probably it's best to just merge this how it's now, but keep the discussion open and ask there what the requirements are.

@qwerty287 qwerty287 changed the title Allow to store logs outside of DB Allow to store logs in files May 6, 2024
@qwerty287 qwerty287 marked this pull request as ready for review May 6, 2024 08:56
@qwerty287 qwerty287 added this to the 2.6.0 milestone May 6, 2024
@qwerty287 qwerty287 requested a review from a team June 1, 2024 11:10
@woodpecker-bot
Copy link
Collaborator

woodpecker-bot commented Jun 2, 2024

Deployment of preview was torn down

@anbraten
Copy link
Member

anbraten commented Jun 5, 2024

Could we try to merge #3722 first or do you think they wouldn't conflict that much?

@qwerty287
Copy link
Contributor Author

I don't think there are many conflicts, this is only about the storing. I took a quick look at your changes and it looks like you didn't change it (the db methods)?

cmd/server/server.go Outdated Show resolved Hide resolved
cmd/server/flags.go Outdated Show resolved Hide resolved
server/grpc/rpc.go Outdated Show resolved Hide resolved
server/services/log/service.go Show resolved Hide resolved
server/services/log/file/file.go Outdated Show resolved Hide resolved
server/services/log/file/file.go Outdated Show resolved Hide resolved
server/services/log/file/file.go Outdated Show resolved Hide resolved
cmd/server/server.go Outdated Show resolved Hide resolved
cmd/server/setup.go Outdated Show resolved Hide resolved
docs/docs/30-administration/10-server-config.md Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jun 6, 2024

Codecov Report

Attention: Patch coverage is 0% with 62 lines in your changes missing coverage. Please review.

Project coverage is 25.81%. Comparing base (2c7edbf) to head (7e318d7).

Files Patch % Lines
server/services/log/file/file.go 0.00% 49 Missing ⚠️
cmd/server/setup.go 0.00% 6 Missing ⚠️
cmd/server/server.go 0.00% 4 Missing ⚠️
server/api/pipeline.go 0.00% 2 Missing ⚠️
server/grpc/rpc.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3568      +/-   ##
==========================================
- Coverage   25.87%   25.81%   -0.06%     
==========================================
  Files         361      362       +1     
  Lines       26667    26725      +58     
==========================================
  Hits         6899     6899              
- Misses      19235    19293      +58     
  Partials      533      533              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@qwerty287 qwerty287 merged commit c724684 into woodpecker-ci:main Jun 6, 2024
7 of 9 checks passed
@qwerty287 qwerty287 deleted the log-obj branch June 6, 2024 12:34
@woodpecker-bot woodpecker-bot mentioned this pull request Jun 6, 2024
1 task
6543 pushed a commit to 6543-forks/woodpecker that referenced this pull request Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature add new functionality server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants