-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
feat: add experimental on-demand pinning #11252
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
Open
ihlec
wants to merge
17
commits into
ipfs:master
Choose a base branch
from
ihlec:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
85e21f7
feat: add experimental on-demand pinning
ihlec 1015c18
fix: addressed failing checks by cleaning up comments, spelling, and …
ihlec a0a150f
docs: move changelog highlight to vFUTURE
lidel 4815e8c
fix(pin): reserve "kubo:on-demand" pin names, make ondemand rm non-de…
ihlec b653636
fix(ondemandpin): do not overwrite pins created during provider lookup
ihlec e66ba68
fix(ondemandpin): derive unpin grace period from DHT record validity
ihlec e022855
fix(ondemandpin): validate OnDemandPinning config at daemon start
ihlec d8d5a54
fix(ondemandpin): key ownership on kubo:on-demand pin name only
ihlec 212f15e
fix(ondemandpin): refuse Null routing and treat failed lookups as unk…
ihlec bc6fb3f
fix(ondemandpin): add min/max replication deadband and grace jitter
ihlec fa13068
fix(ondemandpin): exponential check backoff for failing CIDs
ihlec c3b6891
fix(ondemandpin): do not apply checkTimeout to Pin fetches
ihlec 974103c
fix(ondemandpin): do not resurrect records removed during a check
ihlec 34a679a
fix(ondemandpin): more details for ls and DryRun mode
ihlec 3066da3
fix(ondemandpin): make ls --live explicit about routing and lookup fa…
ihlec 6d89f24
docs(ondemandpin): clarify naming and ipfs-cluster differentiation
ihlec 34f707b
fix(ondemandpin): announce pins via DHTProvider.StartProviding
ihlec File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| package config | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "time" | ||
|
|
||
| "github.com/libp2p/go-libp2p-kad-dht/amino" | ||
| ) | ||
|
|
||
| const ( | ||
| DefaultOnDemandPinReplicationTargetMin = 5 | ||
| DefaultOnDemandPinReplicationTargetMax = 7 | ||
| DefaultOnDemandPinCheckInterval = 10 * time.Minute | ||
|
|
||
| // Must exceed amino.DefaultProvideValidity so stale DHT records expire | ||
| // before unpin. The extra day covers check-interval skew. | ||
| DefaultOnDemandPinUnpinGracePeriod = amino.DefaultProvideValidity + 24*time.Hour | ||
|
|
||
| // Max NextCheckAt delay after repeated check failures (Cap). | ||
| DefaultOnDemandPinCheckBackoffMax = DefaultOnDemandPinUnpinGracePeriod | ||
| ) | ||
|
|
||
| type OnDemandPinning struct { | ||
| // Pin when fewer than this many providers are found in the DHT (excluding self). | ||
| ReplicationTargetMin OptionalInteger | ||
|
|
||
| // Start the unpin grace period only when more than this many providers are found. | ||
| ReplicationTargetMax OptionalInteger | ||
|
|
||
| // How often the checker evaluates all registered CIDs. | ||
| CheckInterval OptionalDuration | ||
|
|
||
| // How long replication must stay above max before unpinning; checker adds up to 2*CheckInterval of jitter. | ||
| UnpinGracePeriod OptionalDuration | ||
|
|
||
| // When true, the checker logs pin/unpin decisions but does not change the pinset. | ||
| DryRun Flag `json:",omitempty"` | ||
| } | ||
|
|
||
| // ValidateOnDemandPinningConfig rejects invalid min/max and non-positive durations. | ||
| func ValidateOnDemandPinningConfig(cfg *OnDemandPinning) error { | ||
| min := cfg.ReplicationTargetMin.WithDefault(DefaultOnDemandPinReplicationTargetMin) | ||
| max := cfg.ReplicationTargetMax.WithDefault(DefaultOnDemandPinReplicationTargetMax) | ||
| if min < 1 { | ||
| return fmt.Errorf("OnDemandPinning.ReplicationTargetMin must be at least 1, got %d", min) | ||
| } | ||
| if max < min { | ||
| return fmt.Errorf("OnDemandPinning.ReplicationTargetMax (%d) must be >= ReplicationTargetMin (%d)", max, min) | ||
| } | ||
| if interval := cfg.CheckInterval.WithDefault(DefaultOnDemandPinCheckInterval); interval <= 0 { | ||
| return fmt.Errorf("OnDemandPinning.CheckInterval must be positive, got %v", interval) | ||
| } | ||
| if grace := cfg.UnpinGracePeriod.WithDefault(DefaultOnDemandPinUnpinGracePeriod); grace <= 0 { | ||
| return fmt.Errorf("OnDemandPinning.UnpinGracePeriod must be positive, got %v", grace) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // ValidateOnDemandPinningRouting rejects Routing.Type=none. | ||
| // Otherwise the checker would pin every registered CID on a null router. | ||
| func ValidateOnDemandPinningRouting(routingType string) error { | ||
| if routingType == "none" { | ||
| return fmt.Errorf("on-demand pinning needs provider lookups; Routing.Type=%q is not usable", routingType) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // ValidateOnDemandPinningProvide rejects Provide.Enabled=false (pins would be invisible). | ||
| func ValidateOnDemandPinningProvide(provideEnabled bool) error { | ||
| if !provideEnabled { | ||
| return fmt.Errorf("on-demand pinning needs Provide.Enabled; other nodes cannot discover local pins when providing is off") | ||
| } | ||
| return nil | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| package config | ||
|
|
||
| import ( | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestValidateOnDemandPinningConfig(t *testing.T) { | ||
| t.Run("defaults are valid", func(t *testing.T) { | ||
| err := ValidateOnDemandPinningConfig(&OnDemandPinning{}) | ||
| assert.NoError(t, err) | ||
| }) | ||
|
|
||
| t.Run("zero replication min is rejected", func(t *testing.T) { | ||
| cfg := &OnDemandPinning{ReplicationTargetMin: *NewOptionalInteger(0)} | ||
| err := ValidateOnDemandPinningConfig(cfg) | ||
| assert.ErrorContains(t, err, "ReplicationTargetMin") | ||
| }) | ||
|
|
||
| t.Run("max below min is rejected", func(t *testing.T) { | ||
| cfg := &OnDemandPinning{ | ||
| ReplicationTargetMin: *NewOptionalInteger(5), | ||
| ReplicationTargetMax: *NewOptionalInteger(4), | ||
| } | ||
| err := ValidateOnDemandPinningConfig(cfg) | ||
| assert.ErrorContains(t, err, "ReplicationTargetMax") | ||
| }) | ||
|
|
||
| t.Run("zero check interval is rejected", func(t *testing.T) { | ||
| cfg := &OnDemandPinning{CheckInterval: *NewOptionalDuration(0)} | ||
| err := ValidateOnDemandPinningConfig(cfg) | ||
| assert.ErrorContains(t, err, "CheckInterval") | ||
| }) | ||
|
|
||
| t.Run("negative grace period is rejected", func(t *testing.T) { | ||
| cfg := &OnDemandPinning{UnpinGracePeriod: *NewOptionalDuration(-time.Hour)} | ||
| err := ValidateOnDemandPinningConfig(cfg) | ||
| assert.ErrorContains(t, err, "UnpinGracePeriod") | ||
| }) | ||
| } | ||
|
|
||
| func TestValidateOnDemandPinningRouting(t *testing.T) { | ||
| assert.NoError(t, ValidateOnDemandPinningRouting("auto")) | ||
| assert.NoError(t, ValidateOnDemandPinningRouting("dht")) | ||
| assert.NoError(t, ValidateOnDemandPinningRouting("delegated")) | ||
| assert.ErrorContains(t, ValidateOnDemandPinningRouting("none"), "none") | ||
| } | ||
|
|
||
| func TestValidateOnDemandPinningProvide(t *testing.T) { | ||
| assert.NoError(t, ValidateOnDemandPinningProvide(true)) | ||
| assert.ErrorContains(t, ValidateOnDemandPinningProvide(false), "Provide.Enabled") | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Could we settle the naming before this ships, and borrow ipfs-cluster's words instead of new ones?
"On demand" normally means fetch it when someone asks for it (video on demand). This checker does the opposite: it counts providers and pins when the content is rare. Cluster already has a vocabulary for that idea:
replication_factor_minandreplication_factor_maxin its config,ReplicationFactorMinandReplicationFactorMaxin its API,--replication-minand--replication-maxinipfs-cluster-ctl. The mechanism is not the same (cluster allocates among known peers, this checker observes the DHT), but the user-facing question is: how many copies should exist, and what happens when there are too few.Concretely: config section
ReplicationPinning,Experimental.ReplicationPinningEnabled, CLIipfs pin replication add|rm|ls, packagereplicationpin/.The min/max pair is worth borrowing too, not just the words. Cluster's min is the minimum healthy copies, and its max is the target number of nodes that should pin. The gap between the two is the deadband this checker does not have today (see my note on
checkRecord): pin below min, unpin only above max. A singleReplicationTargetcannot express that.The
--helptext would carry the same vocabulary. While it is being touched: the tagline says content is pinned "when few DHT providers exist in the routing table", but provider records are not in the routing table, they are found withFindProviders.This is much easier now than later. The pin name
on-demandand the datastore prefix/ondemand-pins/both end up on disk, so renaming after a release means a migration for anyone who turned the experiment on. ipfs/specs#532 carries the same term in its title.If the current naming is deliberate, I would rather hear your reasoning than push a rename through.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When going forward with ReplicationPinning, the user might ask:
I chose the wording to reflect a "demand to replicate". If a CID is at risk to be lost - only then start pinning.
Keeping the word "pinning" is essential to show the similarities. So that is a strength of both names that we should keep.
ReplicationPinning tells the user what it is doing, but does not tell the user what it is not doing.
I think we need to point out that it is a non-permanent pinning with conditions.
We could go with ConditionalPinning to keep the feature extensible for other conditions. Available disk space is such a condition, so it would be transparent that the replication demand is not the only condition.
On the other hand, I still think OnDemand pinning reflects the goal of the feature very well (The network needs you to jump in and replicate to protect the data from being lost).
""On demand" normally means fetch it when someone asks for it" - In this case the network asks for it.
"Uncle Sam wants you" vs "IPFS demands you to protect your data."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am open to follow your intuition here. Feel free to make a decision after considering my argument.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly about this tbh, just wanted to ensure the name is not a placeholder.
If you want to keep "on demand" that is fine with me, just make sure docs /
--helptext on the CLI commands explain that this feature is a decentralized and uncoordinated sibling of "replication factors" in ipfs-cluster (https://ipfscluster.io/documentation/guides/pinning/#replication-factors). (It is useful to tell people what feature is by how it is different (serving different need) from other thing they may be already familiar with.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clarified the naming and added ipfs-cluster differentiation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 6d89f24