Skip to content

Commit

Permalink
storagenode/orders: Remove unnecessary V0 order version checks
Browse files Browse the repository at this point in the history 
The V0->V1 order file transition happened a very long time ago, so there
will never again be unsent V0 order limits that need to be sent from
storagenodes (they would have all expired by now).

There is some logic to check for "unsent order limits" that are V0 -
this commit removes that, as well as tests that rely on "list unsent
order limits" to return V0 order limits.

https: //github.com/storj/storj-private/issues/713
Change-Id: Id2fc92b60400746d7ffdb1dd4d8d7d4deb4eed57
  • Loading branch information
@mobyvb
mobyvb authored and Storj Robot committed May 6, 2024
1 parent 31a9ca9 commit b9fe1e7
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 182 deletions.
31 changes: 8 additions & 23 deletions storagenode/orders/ordersfile/common.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,19 +64,8 @@ type Readable interface {

// OpenWritableUnsent creates or opens for appending the unsent orders file for a given satellite ID and creation hour.
func OpenWritableUnsent(unsentDir string, satelliteID storj.NodeID, creationTime time.Time) (Writable, error) {
// if V0 file already exists, use that. Otherwise use V1 file.
versionToUse := V0
fileName := UnsentFileName(satelliteID, creationTime, V0)
fileName := UnsentFileName(satelliteID, creationTime, V1)
filePath := filepath.Join(unsentDir, fileName)
if _, err := os.Stat(filePath); os.IsNotExist(err) {
fileName = UnsentFileName(satelliteID, creationTime, V1)
filePath = filepath.Join(unsentDir, fileName)
versionToUse = V1
}

if versionToUse == V0 {
return OpenWritableV0(filePath)
}
return OpenWritableV1(filePath, satelliteID, creationTime)
}

Expand Down Expand Up @@ -144,9 +133,12 @@ func MoveUnsent(unsentDir, archiveDir string, satelliteID storj.NodeID, createdA
}

// it expects the file name to be in the format "unsent-orders-<satelliteID>-<createdAtHour>.<version>".
// V0 will not have ".<version>" at the end of the filename.
// V0 will not have ".<version>" at the end of the filename, but all unsent orders are now V1, so it is safe to disregard.
// TODO: should we remove version of being returned? Right now, we only handle one version, however,
// may we want to keep it in case that in the future we need to introduce a new format?
func getUnsentFileInfo(filename string) (satellite storj.NodeID, createdHour time.Time, version Version, err error) {
filename, version = getVersion(filename)
version = V1
filename = strings.TrimSuffix(filename, fmt.Sprintf(".%s", V1)) // remove version suffix from filename

if !strings.HasPrefix(filename, unsentFilePrefix) {
return storj.NodeID{}, time.Time{}, version, Error.New("invalid path: %q", filename)
Expand Down Expand Up @@ -178,7 +170,8 @@ func getUnsentFileInfo(filename string) (satellite storj.NodeID, createdHour tim
// it expects the file name to be in the format "archived-orders-<satelliteID>-<createdAtHour>-<archviedAtTime>-<status>.<version>".
// V0 will not have ".<version>" at the end of the filename.
func getArchivedFileInfo(name string) (satelliteID storj.NodeID, createdAtHour, archivedAt time.Time, status string, version Version, err error) {
name, version = getVersion(name)
version = V1
name = strings.TrimSuffix(name, fmt.Sprintf(".%s", V1)) // remove version suffix from filename

if !strings.HasPrefix(name, archiveFilePrefix) {
return storj.NodeID{}, time.Time{}, time.Time{}, "", version, Error.New("invalid path: %q", name)
Expand Down Expand Up @@ -248,11 +241,3 @@ func getCreationHourString(t time.Time) string {
timeStr := strconv.FormatInt(creationHour, 10)
return timeStr
}

func getVersion(filename string) (trimmedPath string, version Version) {
ext := filepath.Ext(filename)
if ext == "."+string(V1) {
return strings.TrimSuffix(filename, ext), V1
}
return filename, V0
}
25 changes: 0 additions & 25 deletions storagenode/orders/ordersfile/common_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -327,28 +327,3 @@ func TestGetCreationHourString(t *testing.T) {
})
}
}

func TestGetVersion(t *testing.T) {
type args struct {
filename string
}
var tests []struct {
name string
args func(t *testing.T) args

want1 string
want2 Version
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
tArgs := tt.args(t)

got1, got2 := getVersion(tArgs.filename)

assert.Equal(t, tt.want1, got1)

assert.Equal(t, tt.want2, got2)
})
}
}
134 changes: 0 additions & 134 deletions storagenode/orders/store_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -293,76 +293,6 @@ func TestOrdersDB_ListUnsentBySatellite_Expired(t *testing.T) {
})
}

func TestOrdersStore_CorruptUnsentV0(t *testing.T) {
ctx := testcontext.New(t)
defer ctx.Cleanup()
dirName := ctx.Dir("test-orders")
now := time.Now()
satellite := testrand.NodeID()
tomorrow := now.Add(24 * time.Hour)

// make order limit grace period 1 hour
ordersStore, err := orders.NewFileStore(zaptest.NewLogger(t), dirName, time.Hour)
require.NoError(t, err)

// empty store means no orders can be listed
unsent, err := ordersStore.ListUnsentBySatellite(ctx, tomorrow)
require.NoError(t, err)
require.Len(t, unsent, 0)

sn := testrand.SerialNumber()
info := &ordersfile.Info{
Limit: &pb.OrderLimit{
SerialNumber: sn,
SatelliteId: satellite,
Action: pb.PieceAction_GET,
OrderCreation: now,
},
Order: &pb.Order{
SerialNumber: sn,
Amount: 1,
},
}
// store two orders for the same window using deprecated V0
unsentFileName := ordersfile.UnsentFileName(satellite, now, ordersfile.V0)
unsentDir := filepath.Join(dirName, "unsent")
unsentFilePath := filepath.Join(unsentDir, unsentFileName)
of, err := ordersfile.OpenWritableV0(unsentFilePath)
require.NoError(t, err)
require.NoError(t, of.Append(info))
require.NoError(t, of.Append(info))
require.NoError(t, of.Close())

// check that we can see both orders tomorrow
unsent, err = ordersStore.ListUnsentBySatellite(ctx, tomorrow)
require.NoError(t, err)
require.Len(t, unsent, 1)
require.Len(t, unsent[satellite].InfoList, 2)

// corrupt unsent orders file by removing the last byte
err = filepath.Walk(unsentDir, func(path string, info os.FileInfo, err error) error {
require.NoError(t, err)
if info.IsDir() {
return nil
}
err = os.Truncate(path, info.Size()-1)
return err
})
require.NoError(t, err)

// add another order, which we shouldn't see for V0 since it is after the corrupted one
of, err = ordersfile.OpenWritableV0(unsentFilePath)
require.NoError(t, err)
require.NoError(t, of.Append(info))
require.NoError(t, of.Close())

// only the second order should be corrupted, so we should still see one order
unsent, err = ordersStore.ListUnsentBySatellite(ctx, tomorrow)
require.NoError(t, err)
require.Len(t, unsent, 1)
require.Len(t, unsent[satellite].InfoList, 1)
}

func TestOrdersStore_CorruptUnsentV1(t *testing.T) {
ctx := testcontext.New(t)
defer ctx.Cleanup()
Expand Down Expand Up @@ -440,70 +370,6 @@ func TestOrdersStore_CorruptUnsentV1(t *testing.T) {
require.EqualValues(t, sn3, unsent[satellite].InfoList[1].Order.SerialNumber)
}

func TestOrdersStore_V0ToV1(t *testing.T) {
ctx := testcontext.New(t)
defer ctx.Cleanup()
dirName := ctx.Dir("test-orders")
now := time.Now()
satellite := testrand.NodeID()
tomorrow := now.Add(24 * time.Hour)

// make order limit grace period 1 hour
ordersStore, err := orders.NewFileStore(zaptest.NewLogger(t), dirName, time.Hour)
require.NoError(t, err)

// empty store means no orders can be listed
unsent, err := ordersStore.ListUnsentBySatellite(ctx, tomorrow)
require.NoError(t, err)
require.Len(t, unsent, 0)

sn1 := testrand.SerialNumber()
sn2 := testrand.SerialNumber()
info := &ordersfile.Info{
Limit: &pb.OrderLimit{
SerialNumber: sn1,
SatelliteId: satellite,
Action: pb.PieceAction_GET,
OrderCreation: now,
},
Order: &pb.Order{
SerialNumber: sn1,
Amount: 1,
},
}
// store sn1 and sn2 in the same window
// sn1 is stored with deprecated V0, so sn2 should also be stored with V0 even when Enqueue() is used
unsentFileName := ordersfile.UnsentFileName(satellite, now, ordersfile.V0)
unsentDir := filepath.Join(dirName, "unsent")
unsentFilePath := filepath.Join(unsentDir, unsentFileName)
of, err := ordersfile.OpenWritableV0(unsentFilePath)
require.NoError(t, err)

require.NoError(t, of.Append(info))
info.Limit.SerialNumber = sn2
info.Order.SerialNumber = sn2
require.NoError(t, of.Append(info))
require.NoError(t, of.Close())

// check that we can see both orders tomorrow
unsent, err = ordersStore.ListUnsentBySatellite(ctx, tomorrow)
require.NoError(t, err)
require.Len(t, unsent, 1)
require.Len(t, unsent[satellite].InfoList, 2)
require.Equal(t, ordersfile.V0, unsent[satellite].Version)

// archive file to free up window
require.NoError(t, ordersStore.Archive(satellite, unsent[satellite], time.Now(), pb.SettlementWithWindowResponse_ACCEPTED))
// new file should be created with version V1
require.NoError(t, ordersStore.Enqueue(info))

unsent, err = ordersStore.ListUnsentBySatellite(ctx, tomorrow)
require.NoError(t, err)
require.Len(t, unsent, 1)
require.Len(t, unsent[satellite].InfoList, 1)
require.Equal(t, ordersfile.V1, unsent[satellite].Version)
}

func verifyInfosEqual(t *testing.T, a, b *ordersfile.Info) {
t.Helper()

Expand Down

0 comments on commit b9fe1e7

Please sign in to comment.