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

fix: do not allow total all kind of snapshots to be bigger than 250 #1405

Merged
merged 1 commit into from
Feb 9, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/cmd/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func ControllerCmd() cli.Command {
},
cli.IntFlag{
Name: "snapshot-max-count",
Value: 250,
Value: types.MaximumTotalSnapshotCount,
Usage: "Maximum number of snapshots to keep",
},
cli.StringFlag{
Expand Down
2 changes: 1 addition & 1 deletion app/cmd/replica.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ func ReplicaCmd() cli.Command {
},
cli.IntFlag{
Name: "snapshot-max-count",
Value: 250,
Value: types.MaximumTotalSnapshotCount,
Usage: "Maximum number of snapshots to keep",
},
cli.StringFlag{
Expand Down
2 changes: 2 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ go 1.23.0

toolchain go1.23.5

replace github.com/longhorn/types => ../types
Copy link
Member

Choose a reason for hiding this comment

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

Missed this. Do we need the replacement? @PhanLe1010

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in the PR description, we need to merge this dependent PR first longhorn/types#45 then remove the replace directive


require (
github.com/docker/go-units v0.5.0
github.com/gofrs/flock v0.12.1
Expand Down
2 changes: 0 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,6 @@ github.com/longhorn/go-iscsi-helper v0.0.0-20250111093313-7e1930499625 h1:d39A30
github.com/longhorn/go-iscsi-helper v0.0.0-20250111093313-7e1930499625/go.mod h1:yIm3sGRuYOw/Y3XzRhG5+3FlZBOfrU5EZOavzwL2jVs=
github.com/longhorn/sparse-tools v0.0.0-20241216160947-2b328f0fa59c h1:OFz3haCSPdgiiJvXLBeId/4dPu0dxIEqkQkfNMufLwc=
github.com/longhorn/sparse-tools v0.0.0-20241216160947-2b328f0fa59c/go.mod h1:dfbJqfI8+T9ZCp5zhTYcBi/64hPBNt5/vFF3gTlfMmc=
github.com/longhorn/types v0.0.0-20241225162202-00d3a5fd7502 h1:jgw7nosooLe1NQEdCGzM/nEOFzPcurNO+0PDsicc5+A=
github.com/longhorn/types v0.0.0-20241225162202-00d3a5fd7502/go.mod h1:3jHuVDtpkXQzpnp4prguDBskVRric2kmF8aSPkRJ4jw=
github.com/mattn/go-colorable v0.1.13 h1:fFA4WZxdEF4tXPZVKMLwD8oUnCTTo08duU7wxecdEvA=
github.com/mattn/go-colorable v0.1.13/go.mod h1:7S9/ev0klgBDR4GtXTXX8a3vIGJpMovkB8vQcUbaXHg=
github.com/mattn/go-isatty v0.0.16/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM=
Expand Down
4 changes: 2 additions & 2 deletions pkg/backend/file/file.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,8 @@ func (f *Wrapper) SectorSize() (int64, error) {
return 4096, nil
}

func (f *Wrapper) GetSnapshotCountAndSizeUsage() (int, int64, error) {
return 1, 0, nil
func (f *Wrapper) GetSnapshotCountAndSizeUsage() (int, int, int64, error) {
return 1, 1, 0, nil
}

func (f *Wrapper) GetRevisionCounter() (int64, error) {
Expand Down
8 changes: 4 additions & 4 deletions pkg/backend/remote/remote.go
Original file line number Diff line number Diff line change
Expand Up @@ -222,16 +222,16 @@ func (r *Remote) SectorSize() (int64, error) {
return replicaInfo.SectorSize, nil
}

func (r *Remote) GetSnapshotCountAndSizeUsage() (int, int64, error) {
func (r *Remote) GetSnapshotCountAndSizeUsage() (int, int, int64, error) {
replicaInfo, err := r.info()
if err != nil {
return 0, 0, err
return 0, 0, 0, err
}
switch replicaInfo.State {
case "open", "dirty", "rebuilding":
return replicaInfo.SnapshotCountUsage, replicaInfo.SnapshotSizeUsage, nil
return replicaInfo.SnapshotCountUsage, replicaInfo.SnapshotCountTotal, replicaInfo.SnapshotSizeUsage, nil
}
return 0, 0, fmt.Errorf("invalid state %v for counting snapshots", replicaInfo.State)
return 0, 0, 0, fmt.Errorf("invalid state %v for counting snapshots", replicaInfo.State)
}

func (r *Remote) GetRevisionCounter() (int64, error) {
Expand Down
9 changes: 6 additions & 3 deletions pkg/controller/control.go
Original file line number Diff line number Diff line change
Expand Up @@ -286,13 +286,16 @@ func (c *Controller) Snapshot(inputName string, labels map[string]string, should
}

func (c *Controller) canDoSnapshot() error {
countUsage, sizeUsage, err := c.backend.GetSnapshotCountAndSizeUsage()
countUsage, countTotal, sizeUsage, err := c.backend.GetSnapshotCountAndSizeUsage()
if err != nil {
return err
}
if countUsage >= c.snapshotMaxCount {
return fmt.Errorf("snapshot count usage %d is equal or larger than snapshotMaxCount %d", countUsage, c.snapshotMaxCount)
}
if countTotal >= types.MaximumTotalSnapshotCount {
return fmt.Errorf("snapshot count total is already too big: %v", countTotal)
}
// if SnapshotMaxSize is 0, it means no limit
if c.SnapshotMaxSize == 0 {
return nil
Expand Down Expand Up @@ -758,7 +761,7 @@ func (c *Controller) SetSnapshotMaxCount(count int) error {
c.Lock()
defer c.Unlock()

countUsage, _, err := c.backend.GetSnapshotCountAndSizeUsage()
countUsage, _, _, err := c.backend.GetSnapshotCountAndSizeUsage()
if err != nil {
return err
}
Expand Down Expand Up @@ -796,7 +799,7 @@ func (c *Controller) SetSnapshotMaxSize(size int64) error {
c.Lock()
defer c.Unlock()

_, sizeUsage, err := c.backend.GetSnapshotCountAndSizeUsage()
_, _, sizeUsage, err := c.backend.GetSnapshotCountAndSizeUsage()
if err != nil {
return err
}
Expand Down
21 changes: 12 additions & 9 deletions pkg/controller/replicator.go
Original file line number Diff line number Diff line change
Expand Up @@ -353,35 +353,38 @@ type backendWrapper struct {
mode types.Mode
}

func (r *replicator) GetSnapshotCountAndSizeUsage() (countUsage int, sizeUsage int64, err error) {
func (r *replicator) GetSnapshotCountAndSizeUsage() (countUsage, countTotal int, sizeUsage int64, err error) {
var (
count int
size int64
hasResult bool
currentCountUsage, currentCountTotal int
size int64
hasResult bool
)
for _, backend := range r.backends {
if backend.mode == types.ERR {
continue
}
// ignore error and try next one
count, size, err = backend.backend.GetSnapshotCountAndSizeUsage()
currentCountUsage, currentCountTotal, size, err = backend.backend.GetSnapshotCountAndSizeUsage()
if err != nil {
logrus.Errorf("Failed to get snapshot count and size usage: %v", err)
continue
}
hasResult = true
if countUsage < count {
countUsage = count
if countUsage < currentCountUsage {
countUsage = currentCountUsage
}
if countTotal < currentCountTotal {
countTotal = currentCountTotal
}
if sizeUsage < size {
sizeUsage = size
}
}

if !hasResult {
return 0, 0, fmt.Errorf("cannot get an valid result for snapshot usage")
return 0, 0, 0, fmt.Errorf("cannot get an valid result for snapshot usage")
}
return countUsage, sizeUsage, nil
return countUsage, countTotal, sizeUsage, nil
}

func (r *replicator) GetHeadFileSize() (int64, error) {
Expand Down
1 change: 1 addition & 0 deletions pkg/replica/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ func GetReplicaInfo(r *enginerpc.Replica) *types.ReplicaInfo {
RevisionCounterDisabled: r.RevisionCounterDisabled,
UnmapMarkDiskChainRemoved: r.UnmapMarkDiskChainRemoved,
SnapshotCountUsage: int(r.SnapshotCountUsage),
SnapshotCountTotal: int(r.SnapshotCountTotal),
SnapshotSizeUsage: r.SnapshotSizeUsage,
}

Expand Down
14 changes: 8 additions & 6 deletions pkg/replica/replica.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ import (
const (
volumeMetaData = "volume.meta"

maximumChainLength = 250

tmpFileSuffix = ".tmp"

// Special indexes inside r.volume.files
Expand Down Expand Up @@ -151,7 +149,7 @@ func New(ctx context.Context, size, sectorSize int64, dir string, backingFile *b
func NewReadOnly(ctx context.Context, dir, head string, backingFile *backingfile.BackingFile) (*Replica, error) {
// size and sectorSize don't matter because they will be read from metadata
// snapshotMaxCount and SnapshotMaxSize don't matter because readonly replica can't create a new disk
return construct(ctx, true, 0, diskutil.ReplicaSectorSize, dir, head, backingFile, false, false, 250, 0)
return construct(ctx, true, 0, diskutil.ReplicaSectorSize, dir, head, backingFile, false, false, types.MaximumTotalSnapshotCount, 0)
}

func construct(ctx context.Context, readonly bool, size, sectorSize int64, dir, head string, backingFile *backingfile.BackingFile, disableRevCounter, unmapMarkDiskChainRemoved bool, snapshotMaxCount int, snapshotMaxSize int64) (*Replica, error) {
Expand Down Expand Up @@ -1050,7 +1048,7 @@ func (r *Replica) openLiveChain() error {
return err
}

if len(chain) > maximumChainLength {
if len(chain) > types.MaximumTotalSnapshotCount {
return fmt.Errorf("live chain is too long: %v", len(chain))
}

Expand Down Expand Up @@ -1398,11 +1396,15 @@ func (r *Replica) ListDisks() map[string]DiskInfo {
return result
}

func (r *Replica) GetSnapshotCountUsage() int {
func (r *Replica) GetSnapshotCount() (int, int) {
r.RLock()
defer r.RUnlock()

return r.getSnapshotCountUsage()
return r.getSnapshotCountUsage(), r.getSnapshotCountTotal()
}

func (r *Replica) getSnapshotCountTotal() int {
return len(r.diskData)
}

func (r *Replica) getSnapshotCountUsage() int {
Expand Down
4 changes: 3 additions & 1 deletion pkg/replica/rpc/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,9 @@ func (rs *ReplicaServer) getReplica() (replica *enginerpc.Replica) {
chain, _ := r.DisplayChain()
replica.Chain = chain
replica.RemainSnapshots = int32(r.GetRemainSnapshotCounts())
replica.SnapshotCountUsage = int32(r.GetSnapshotCountUsage())
snapCountUsage, snapCountTotal := r.GetSnapshotCount()
replica.SnapshotCountUsage = int32(snapCountUsage)
replica.SnapshotCountTotal = int32(snapCountTotal)
replica.SnapshotSizeUsage = r.GetSnapshotSizeUsage()
if !r.IsRevCounterDisabled() {
replica.RevisionCounter = r.GetRevisionCounter()
Expand Down
1 change: 1 addition & 0 deletions pkg/types/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ type ReplicaInfo struct {
RevisionCounterDisabled bool `json:"revisioncounterdisabled"`
UnmapMarkDiskChainRemoved bool `json:"unmapMarkDiskChainRemoved"`
SnapshotCountUsage int `json:"snapshotCountUsage"`
SnapshotCountTotal int `json:"snapshotCountTotal"`
SnapshotSizeUsage int64 `json:"snapshotSizeUsage"`
}

Expand Down
4 changes: 3 additions & 1 deletion pkg/types/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ const (
EngineFrontendISCSI = "tgt-iscsi"

VolumeHeadName = "volume-head"

MaximumTotalSnapshotCount = 250
)

type DataServerProtocol string
Expand Down Expand Up @@ -105,7 +107,7 @@ type Backend interface {
ResetRebuild() error
SetSnapshotMaxCount(count int) error
SetSnapshotMaxSize(size int64) error
GetSnapshotCountAndSizeUsage() (int, int64, error)
GetSnapshotCountAndSizeUsage() (int, int, int64, error)
}

type BackendFactory interface {
Expand Down
Loading