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

feat(house_keeper): Remove GCS Blob Cleaner #321

Merged
merged 12 commits into from
Nov 7, 2023
Merged

Conversation

EmilLuta
Copy link
Contributor

What ❔

This feature is replaced with a 30 days TTL policy in GCS. The policy is already applied, this work cleans unnecessary component on core side.

Why ❔

Unnecessary work on infra resources. Additionally, the implementation closely ties the codebase to GCS.

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted via zk fmt and zk lint.

@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ba88f67) 35.85% compared to head (e90efca) 36.06%.
Report is 16 commits behind head on main.

❗ Current head e90efca differs from pull request most recent head 7c48e40. Consider uploading reports for the commit 7c48e40 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #321      +/-   ##
==========================================
+ Coverage   35.85%   36.06%   +0.20%     
==========================================
  Files         519      518       -1     
  Lines       27828    27702     -126     
==========================================
+ Hits         9978     9990      +12     
+ Misses      17850    17712     -138     
Files Coverage Δ
core/lib/config/src/configs/house_keeper.rs 50.00% <ø> (ø)
core/lib/dal/src/blocks_dal.rs 80.22% <ø> (+2.17%) ⬆️
core/lib/dal/src/prover_dal.rs 48.80% <ø> (+4.72%) ⬆️
core/lib/dal/src/witness_generator_dal.rs 59.77% <ø> (+10.00%) ⬆️
core/lib/zksync_core/src/lib.rs 8.12% <ø> (ø)

... and 33 files with indirect coverage changes

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

popzxc
popzxc previously approved these changes Oct 26, 2023
@popzxc
Copy link
Member

popzxc commented Oct 26, 2023

Probably we should document somewhere that we expect GCS TTL to be configured, but I'm not sure where exactly.

@EmilLuta
Copy link
Contributor Author

EmilLuta commented Oct 26, 2023

Probably we should document somewhere that we expect GCS TTL to be configured, but I'm not sure where exactly.

I think you're right and agree with the thought overall. This seems more like infra work and they already keep most infra as IaC. That said, us not documenting this will end up with file accumulating in some storage (not a disastrous end-state). You think we should start some infra invariant documentation we expect for core? If yes, perhaps better done through a different task. WDYT?

@popzxc
Copy link
Member

popzxc commented Oct 26, 2023

I agree that it doesn't have to be done in this PR.
I also agree that infra-agnostic docs will be good to have -- we aim to be fully infra-agnostic via zkStack in the future; so knowing that there is no embedded blob cleaning functionality may be useful to some folks (imagine someone running their own object store on bare metal).

@EmilLuta
Copy link
Contributor Author

I agree that it doesn't have to be done in this PR. I also agree that infra-agnostic docs will be good to have -- we aim to be fully infra-agnostic via zkStack in the future; so knowing that there is no embedded blob cleaning functionality may be useful to some folks (imagine someone running their own object store on bare metal).

Created ticket to track it.

This feature is replaced with a 30 days TTL policy in GCS.
The policy is already applied, this work cleans unnecessary component on
core side.
@EmilLuta EmilLuta added this pull request to the merge queue Oct 26, 2023
@Deniallugo Deniallugo removed this pull request from the merge queue due to a manual request Oct 26, 2023
Deniallugo
Deniallugo previously approved these changes Oct 26, 2023
auto-merge was automatically disabled October 26, 2023 15:19

Merge queue setting changed

RomanBrodetski
RomanBrodetski previously approved these changes Nov 7, 2023
RomanBrodetski
RomanBrodetski previously approved these changes Nov 7, 2023
@EmilLuta EmilLuta added this pull request to the merge queue Nov 7, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 7, 2023
@EmilLuta EmilLuta added this pull request to the merge queue Nov 7, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 7, 2023
@EmilLuta EmilLuta added this pull request to the merge queue Nov 7, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 7, 2023
@EmilLuta EmilLuta added this pull request to the merge queue Nov 7, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 7, 2023
@EmilLuta EmilLuta added this pull request to the merge queue Nov 7, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 7, 2023
@EmilLuta EmilLuta added this pull request to the merge queue Nov 7, 2023
Merged via the queue into main with commit 9548914 Nov 7, 2023
30 checks passed
@EmilLuta EmilLuta deleted the evl-remove-house-keeper branch November 7, 2023 18:56
github-merge-queue bot pushed a commit that referenced this pull request Nov 15, 2023
🤖 I have created a release *beep* *boop*
---


##
[18.0.0](core-v17.1.0...core-v18.0.0)
(2023-11-14)


### ⚠ BREAKING CHANGES

* boojum integration
([#112](#112))

### Features

* **basic_witness_input_producer:** Witness inputs queued after BWIP run
([#345](#345))
([9c2be91](9c2be91))
* boojum integration
([#112](#112))
([e76d346](e76d346))
* **core:** adds a get proof endpoint in zks namespace
([#455](#455))
([f4313a4](f4313a4))
* **core:** Split config definitions and deserialization
([#414](#414))
([c7c6b32](c7c6b32))
* **dal:** Do not load config from env in DAL crate
([#444](#444))
([3fe1bb2](3fe1bb2))
* **house_keeper:** Remove GCS Blob Cleaner
([#321](#321))
([9548914](9548914))
* **job-processor:** report attempts metrics
([#448](#448))
([ab31f03](ab31f03))
* **vm:** Use the one interface for all vms
([#277](#277))
([91bb99b](91bb99b))


### Bug Fixes

* **boojnet:** various boojnet fixes
([#462](#462))
([f13648c](f13648c))
* change vks upgrade logic
([#491](#491))
([cb394f3](cb394f3))
* **eth-sender:** Correct ABI for get_verification_key
([#445](#445))
([8af0d85](8af0d85))
* **metadata-calculator:** Save commitment for pre-boojum
([#481](#481))
([664ce33](664ce33))
* Versioned L1 batch metadata
([#450](#450))
([8a40dc3](8a40dc3))
* **vm:** storage_refunds for `vm_refunds_enhancement`
([#449](#449))
([1e1e59f](1e1e59f))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
EmilLuta added a commit that referenced this pull request Mar 31, 2024
The entire codebase for this change was merged a long time again. See [321](#321).
This PR cleans up the database leftovers. This logic is moved to GCS.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants