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: storage: Path type filters #9013

Merged
merged 9 commits into from
Jul 15, 2022
Merged

feat: storage: Path type filters #9013

merged 9 commits into from
Jul 15, 2022

Conversation

magik6k
Copy link
Contributor

@magik6k magik6k commented Jul 11, 2022

Related Issues

Implements #8794

Proposed Changes

Add two new fields to [storagepath]/sectorstore.json:

// AllowTypes lists sector file types which are allowed to be put into this
// path. If empty, all file types are allowed.
//
// Valid values
// - "unsealed"
// - "sealed"
// - "cache"
// - "update"
// - "update-cache"
// Any other value will generate a warning and be ignored.

"AllowTypes": [
  "sealed", "unsealed", "update",
],



// DenyTypes lists sector file types which aren't allowed to be put into this
// path.
//
// Valid values
// - "unsealed"
// - "sealed"
// - "cache"
// - "update"
// - "update-cache"
// Any other value will generate a warning and be ignored.

"DenyTypes": [
  "cache", "update-cache"
]

After a path with this config is attached, lotus-miner will stop putting new files of disallowed types into that path.

If there are existing files with disallowed types in a path, those files will remain readable for PoSt/Retrieval, so the worst that can happen in case of misconfiguration is that sealing tasks will get stuck waiting for storage to become available.

The most basic setup to separate sealed data from unsealed copies would be to:

  • Not have any filters on sealing paths (no change)
  • Add "DenyTypes": ["unsealed"] to long-term sealed storage paths
  • Add "AllowTypes": ["unsealed"] to long-term unsealed storage paths

Here's how storage list looks with filters set:
2022-07-12-131643_816x445_scrot

Checklist

Before you mark the PR ready for review, please make sure that:

  • All commits have a clear commit message.
  • The PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, test
    • area: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps
  • This PR has tests for new functionality or change in behaviour
  • If new user-facing features are introduced, clear usage guidelines and / or documentation updates should be included in https://lotus.filecoin.io or Discussion Tutorials.
  • CI is green

@codecov
Copy link

codecov bot commented Jul 11, 2022

Codecov Report

Merging #9013 (db9105e) into master (e7c8082) will increase coverage by 0.05%.
The diff coverage is 76.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9013      +/-   ##
==========================================
+ Coverage   40.69%   40.74%   +0.05%     
==========================================
  Files         707      707              
  Lines       78737    78875     +138     
==========================================
+ Hits        32043    32139      +96     
- Misses      41235    41251      +16     
- Partials     5459     5485      +26     
Impacted Files Coverage Δ
api/api_storage.go 0.00% <ø> (ø)
cmd/lotus-miner/init.go 0.00% <0.00%> (ø)
cmd/lotus-miner/storage.go 43.93% <0.00%> (-0.94%) ⬇️
itests/kit/blockminer.go 88.64% <0.00%> (+17.99%) ⬆️
storage/sealer/storiface/index.go 100.00% <ø> (ø)
storage/paths/local.go 61.25% <66.66%> (+0.08%) ⬆️
storage/paths/index.go 75.64% <70.00%> (-1.17%) ⬇️
storage/sealer/storiface/filetype.go 87.90% <81.81%> (-3.35%) ⬇️
itests/kit/ensemble.go 92.23% <100.00%> (+0.12%) ⬆️
itests/kit/node_miner.go 81.25% <100.00%> (+0.23%) ⬆️
... and 35 more

@magik6k magik6k force-pushed the feat/path-type-filters branch from e842992 to 2c801b8 Compare July 12, 2022 11:57
@magik6k magik6k changed the title storage: Path type filters feat: storage: Path type filters Jul 12, 2022
@magik6k magik6k force-pushed the feat/path-type-filters branch from 2c801b8 to 07dc204 Compare July 12, 2022 12:05
@magik6k magik6k marked this pull request as ready for review July 12, 2022 12:12
@magik6k magik6k requested a review from a team as a code owner July 12, 2022 12:12
Comment on lines +188 to +193
// if api shuts down before mining, we may get an error which we should probably just ignore
// (fixing it will require rewriting most of the mining loop)
if err != nil && !strings.Contains(err.Error(), "websocket connection closed") {
require.NoError(bm.t, err)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review note: This isn't exactly related to this PR, but it should fix lots of test flakiness we're seeing

Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nits, but looks like it works.

storage/sealer/storiface/filetype_test.go Show resolved Hide resolved
storage/paths/index.go Outdated Show resolved Hide resolved
storage/paths/index.go Outdated Show resolved Hide resolved
i.pathAlerts[si.ID] = i.alerting.AddAlertType("sector-index", "pathconf-"+string(si.ID))
}

var hasConfigIsses bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this variable name supposed to be saying?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It tells you whether there are issues in the storage path config.

magik6k and others added 2 commits July 15, 2022 12:46
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.

2 participants