From 5c3a148d6ecd35f46c08b75a8eadd8b3c802dbee Mon Sep 17 00:00:00 2001 From: Fadila Khadar Date: Tue, 22 Nov 2022 14:02:01 +0100 Subject: [PATCH] satellite/overlaycache: fix typo in UpdateCheckIn request - fix a malformed SQL query - add test to be sure we don't have this problem again. Change-Id: I3fde8c59ba01335411e51d964bec95bc26cfc961 --- satellite/overlay/service.go | 2 + satellite/satellitedb/overlaycache.go | 64 ++++++++++++++-------- satellite/satellitedb/overlaycache_test.go | 39 +++++++++++++ 3 files changed, 81 insertions(+), 24 deletions(-) diff --git a/satellite/overlay/service.go b/satellite/overlay/service.go index 6c005c0fd..1fb8e9445 100644 --- a/satellite/overlay/service.go +++ b/satellite/overlay/service.go @@ -123,6 +123,8 @@ type DB interface { TestSuspendNodeOffline(ctx context.Context, nodeID storj.NodeID, suspendedAt time.Time) (err error) // TestNodeCountryCode sets node country code. TestNodeCountryCode(ctx context.Context, nodeID storj.NodeID, countryCode string) (err error) + // TestUpdateCheckInDirectUpdate tries to update a node info directly. Returns true if it succeeded, false if there were no node with the provided (used for testing). + TestUpdateCheckInDirectUpdate(ctx context.Context, node NodeCheckInInfo, timestamp time.Time, semVer version.SemVer, walletFeatures string) (updated bool, err error) // IterateAllContactedNodes will call cb on all known nodes (used in restore trash contexts). IterateAllContactedNodes(context.Context, func(context.Context, *SelectedNode) error) error diff --git a/satellite/satellitedb/overlaycache.go b/satellite/satellitedb/overlaycache.go index 7cb6cdc1a..90c45b265 100644 --- a/satellite/satellitedb/overlaycache.go +++ b/satellite/satellitedb/overlaycache.go @@ -1270,24 +1270,7 @@ func (cache *overlaycache) getNodesForDQLastSeenBefore(ctx context.Context, cuto return nodeIDs, rows.Err() } -// UpdateCheckIn updates a single storagenode with info from when the the node last checked in. -func (cache *overlaycache) UpdateCheckIn(ctx context.Context, node overlay.NodeCheckInInfo, timestamp time.Time, config overlay.NodeSelectionConfig) (err error) { - defer mon.Task()(&ctx)(&err) - - if node.Address.GetAddress() == "" { - return Error.New("error UpdateCheckIn: missing the storage node address") - } - - semVer, err := version.NewSemVer(node.Version.GetVersion()) - if err != nil { - return Error.New("unable to convert version to semVer") - } - - walletFeatures, err := encodeWalletFeatures(node.Operator.GetWalletFeatures()) - if err != nil { - return Error.Wrap(err) - } - +func (cache *overlaycache) updateCheckInDirectUpdate(ctx context.Context, node overlay.NodeCheckInInfo, timestamp time.Time, semVer version.SemVer, walletFeatures string) (updated bool, err error) { // First try the fast path. var res sql.Result res, err = cache.db.ExecContext(ctx, ` @@ -1310,7 +1293,7 @@ func (cache *overlaycache) UpdateCheckIn(ctx context.Context, node overlay.NodeC END, last_ip_port=$16, wallet_features=$17, - country_code=$18 + country_code=$18, last_software_update_email = CASE WHEN $19::bool IS TRUE THEN $15::timestamptz WHEN $20::bool IS FALSE THEN NULL @@ -1340,11 +1323,40 @@ func (cache *overlaycache) UpdateCheckIn(ctx context.Context, node overlay.NodeC node.SoftwareUpdateEmailSent, node.VersionBelowMin, ) - if err == nil { - affected, affectedErr := res.RowsAffected() - if affectedErr == nil && affected > 0 { - return nil - } + if err != nil { + return false, Error.Wrap(err) + } + affected, affectedErr := res.RowsAffected() + if affectedErr != nil { + return false, Error.Wrap(err) + } + return affected > 0, nil +} + +// UpdateCheckIn updates a single storagenode with info from when the the node last checked in. +func (cache *overlaycache) UpdateCheckIn(ctx context.Context, node overlay.NodeCheckInInfo, timestamp time.Time, config overlay.NodeSelectionConfig) (err error) { + defer mon.Task()(&ctx)(&err) + + if node.Address.GetAddress() == "" { + return Error.New("error UpdateCheckIn: missing the storage node address") + } + + semVer, err := version.NewSemVer(node.Version.GetVersion()) + if err != nil { + return Error.New("unable to convert version to semVer") + } + + walletFeatures, err := encodeWalletFeatures(node.Operator.GetWalletFeatures()) + if err != nil { + return Error.Wrap(err) + } + + updated, err := cache.updateCheckInDirectUpdate(ctx, node, timestamp, semVer, walletFeatures) + if err != nil { + return Error.Wrap(err) + } + if updated { + return nil } _, err = cache.db.ExecContext(ctx, ` @@ -1580,3 +1592,7 @@ func (cache *overlaycache) IterateAllNodeDossiers(ctx context.Context, cb func(c } } } + +func (cache *overlaycache) TestUpdateCheckInDirectUpdate(ctx context.Context, node overlay.NodeCheckInInfo, timestamp time.Time, semVer version.SemVer, walletFeatures string) (updated bool, err error) { + return cache.updateCheckInDirectUpdate(ctx, node, timestamp, semVer, walletFeatures) +} diff --git a/satellite/satellitedb/overlaycache_test.go b/satellite/satellitedb/overlaycache_test.go index 1417bc668..6ddb9e8b3 100644 --- a/satellite/satellitedb/overlaycache_test.go +++ b/satellite/satellitedb/overlaycache_test.go @@ -13,6 +13,7 @@ import ( "storj.io/common/storj" "storj.io/common/testcontext" "storj.io/common/testrand" + "storj.io/private/version" "storj.io/storj/private/teststorj" "storj.io/storj/satellite" "storj.io/storj/satellite/overlay" @@ -184,3 +185,41 @@ func TestSetNodeContained(t *testing.T) { require.False(t, cacheInfo.Contained) }) } + +func TestUpdateCheckInDirectUpdate(t *testing.T) { + satellitedbtest.Run(t, func(ctx *testcontext.Context, t *testing.T, db satellite.DB) { + cache := db.OverlayCache() + db.OverlayCache() + selectionCfg := overlay.NodeSelectionConfig{ + OnlineWindow: 4 * time.Hour, + } + nodeID := teststorj.NodeIDFromString("testnode0") + checkInInfo := overlay.NodeCheckInInfo{ + IsUp: true, + Address: &pb.NodeAddress{ + Address: "1.2.3.4", + }, + Version: &pb.NodeVersion{ + Version: "v0.0.0", + CommitHash: "", + Timestamp: time.Time{}, + Release: false, + }, + Operator: &pb.NodeOperator{ + Email: "test@storj.test", + }, + } + now := time.Now().UTC() + checkInInfo.NodeID = nodeID + semVer, err := version.NewSemVer(checkInInfo.Version.Version) + require.NoError(t, err) + // node unknown - should not be updated by updateCheckInDirectUpdate + updated, err := cache.TestUpdateCheckInDirectUpdate(ctx, checkInInfo, now, semVer, "encodedwalletfeature") + require.NoError(t, err) + require.False(t, updated) + require.NoError(t, cache.UpdateCheckIn(ctx, checkInInfo, now, selectionCfg)) + updated, err = cache.TestUpdateCheckInDirectUpdate(ctx, checkInInfo, now.Add(6*time.Hour), semVer, "encodedwalletfeature") + require.NoError(t, err) + require.True(t, updated) + }) +}