Unistore : Ensure Watch works in HA mode. (#93428)

* Replace Watch with WatchNext

* remove watchset

* fix previous page and closing the channel

* Remove the broadcaster cache to prevent dupplicated events

* add watch bookmark

* add watch bookmark

* cleanup comments

* disable the tests for bookmarks for now

* Ensure we send previosu events

* lint

* re-introduce the cache

* load from cache

* disabling legacy test

* disabling legacy test

* Update pkg/storage/unified/resource/server.go

Co-authored-by: Diego Augusto Molina <diegoaugustomolina@gmail.com>

* Could not read previous events

* add proper migration

* Add previous_resource_version to both history and resource

* First event should have an RV of 2 and not 1

* Test both storage backends

* fix the inital RV for the sql backend

* ensure graceful stop of the stream decoder

* gocyclo

---------

Co-authored-by: Diego Augusto Molina <diegoaugustomolina@gmail.com>

pkg/storage/unified/sql/backend.go

@@ -22,6 +22,7 @@ import (
 )
 
 const trace_prefix = "sql.resource."
+const defaultPollingInterval = 100 * time.Millisecond
 
 type Backend interface {
 	resource.StorageBackend
@@ -30,8 +31,9 @@ type Backend interface {
 }
 
 type BackendOptions struct {
-	DBProvider db.DBProvider
-	Tracer     trace.Tracer
+	DBProvider      db.DBProvider
+	Tracer          trace.Tracer
+	PollingInterval time.Duration
 }
 
 func NewBackend(opts BackendOptions) (Backend, error) {
@@ -43,12 +45,17 @@ func NewBackend(opts BackendOptions) (Backend, error) {
 	}
 	ctx, cancel := context.WithCancel(context.Background())
 
+	pollingInterval := opts.PollingInterval
+	if pollingInterval == 0 {
+		pollingInterval = defaultPollingInterval
+	}
 	return &backend{
-		done:       ctx.Done(),
-		cancel:     cancel,
-		log:        log.New("sql-resource-server"),
-		tracer:     opts.Tracer,
-		dbProvider: opts.DBProvider,
+		done:            ctx.Done(),
+		cancel:          cancel,
+		log:             log.New("sql-resource-server"),
+		tracer:          opts.Tracer,
+		dbProvider:      opts.DBProvider,
+		pollingInterval: pollingInterval,
 	}, nil
 }
 
@@ -70,6 +77,7 @@ type backend struct {
 
 	// watch streaming
 	//stream chan *resource.WatchEvent
+	pollingInterval time.Duration
 }
 
 func (b *backend) Init(ctx context.Context) error {
@@ -180,7 +188,6 @@ func (b *backend) create(ctx context.Context, event resource.WriteEvent) (int64,
 
 		return nil
 	})
-
 	return newVersion, err
 }
 
@@ -512,8 +519,7 @@ func (b *backend) WatchWriteEvents(ctx context.Context) (<-chan *resource.Writte
 }
 
 func (b *backend) poller(ctx context.Context, since groupResourceRV, stream chan<- *resource.WrittenEvent) {
-	interval := 100 * time.Millisecond // TODO make this configurable
-	t := time.NewTicker(interval)
+	t := time.NewTicker(b.pollingInterval)
 	defer close(stream)
 	defer t.Stop()
 
@@ -526,7 +532,7 @@ func (b *backend) poller(ctx context.Context, since groupResourceRV, stream chan
 			grv, err := b.listLatestRVs(ctx)
 			if err != nil {
 				b.log.Error("get the latest resource version", "err", err)
-				t.Reset(interval)
+				t.Reset(b.pollingInterval)
 				continue
 			}
 			for group, items := range grv {
@@ -543,7 +549,7 @@ func (b *backend) poller(ctx context.Context, since groupResourceRV, stream chan
 					next, err := b.poll(ctx, group, resource, since[group][resource], stream)
 					if err != nil {
 						b.log.Error("polling for resource", "err", err)
-						t.Reset(interval)
+						t.Reset(b.pollingInterval)
 						continue
 					}
 					if next > since[group][resource] {
@@ -552,7 +558,7 @@ func (b *backend) poller(ctx context.Context, since groupResourceRV, stream chan
 				}
 			}
 
-			t.Reset(interval)
+			t.Reset(b.pollingInterval)
 		}
 	}
 }
@@ -636,7 +642,8 @@ func (b *backend) poll(ctx context.Context, grp string, res string, since int64,
 					Resource:  rec.Key.Resource,
 					Name:      rec.Key.Name,
 				},
-				Type: resource.WatchEvent_Type(rec.Action),
+				Type:       resource.WatchEvent_Type(rec.Action),
+				PreviousRV: rec.PreviousRV,
 			},
 			ResourceVersion: rec.ResourceVersion,
 			// Timestamp:  , // TODO: add timestamp
@@ -663,15 +670,16 @@ func resourceVersionAtomicInc(ctx context.Context, x db.ContextExecer, d sqltemp
 
 	if errors.Is(err, sql.ErrNoRows) {
 		// if there wasn't a row associated with the given resource, we create one with
-		// version 1
+		// version 2 to match the etcd behavior.
 		if _, err = dbutil.Exec(ctx, x, sqlResourceVersionInsert, sqlResourceVersionRequest{
-			SQLTemplate: sqltemplate.New(d),
-			Group:       key.Group,
-			Resource:    key.Resource,
+			SQLTemplate:     sqltemplate.New(d),
+			Group:           key.Group,
+			Resource:        key.Resource,
+			resourceVersion: &resourceVersion{1},
 		}); err != nil {
 			return 0, fmt.Errorf("insert into resource_version: %w", err)
 		}
-		return 1, nil
+		return 2, nil
 	}
 
 	if err != nil {

pkg/storage/unified/sql/backend_test.go

@@ -227,7 +227,7 @@ func TestResourceVersionAtomicInc(t *testing.T) {
 
 		v, err := resourceVersionAtomicInc(ctx, b.DB, dialect, resKey)
 		require.NoError(t, err)
-		require.Equal(t, int64(1), v)
+		require.Equal(t, int64(2), v)
 	})
 
 	t.Run("happy path - update existing row", func(t *testing.T) {
@@ -304,7 +304,7 @@ func TestBackend_create(t *testing.T) {
 
 		v, err := b.create(ctx, event)
 		require.NoError(t, err)
-		require.Equal(t, int64(1), v)
+		require.Equal(t, int64(2), v)
 	})
 
 	t.Run("error inserting into resource", func(t *testing.T) {
@@ -409,7 +409,7 @@ func TestBackend_update(t *testing.T) {
 
 		v, err := b.update(ctx, event)
 		require.NoError(t, err)
-		require.Equal(t, int64(1), v)
+		require.Equal(t, int64(2), v)
 	})
 
 	t.Run("error in first update to resource", func(t *testing.T) {
@@ -513,7 +513,7 @@ func TestBackend_delete(t *testing.T) {
 
 		v, err := b.delete(ctx, event)
 		require.NoError(t, err)
-		require.Equal(t, int64(1), v)
+		require.Equal(t, int64(2), v)
 	})
 
 	t.Run("error deleting resource", func(t *testing.T) {

pkg/storage/unified/sql/data/resource_history_insert.sql

@@ -6,6 +6,7 @@ INSERT INTO {{ .Ident "resource_history" }}
         {{ .Ident "namespace" }},
         {{ .Ident "name" }},
 
+        {{ .Ident "previous_resource_version"}},
         {{ .Ident "value" }},
         {{ .Ident "action" }}
     )
@@ -17,6 +18,7 @@ INSERT INTO {{ .Ident "resource_history" }}
         {{ .Arg .WriteEvent.Key.Namespace }},
         {{ .Arg .WriteEvent.Key.Name }},
 
+        {{ .Arg .WriteEvent.PreviousRV }},
         {{ .Arg .WriteEvent.Value }},
         {{ .Arg .WriteEvent.Type }}
     )

pkg/storage/unified/sql/data/resource_history_poll.sql

@@ -5,7 +5,8 @@ SELECT
     {{ .Ident "resource" | .Into .Response.Key.Resource }},
     {{ .Ident "name" | .Into .Response.Key.Name }},
     {{ .Ident "value" | .Into .Response.Value }},
-    {{ .Ident "action" | .Into .Response.Action }}
+    {{ .Ident "action" | .Into .Response.Action }},
+    {{ .Ident "previous_resource_version" | .Into .Response.PreviousRV }}
 
     FROM {{ .Ident "resource_history" }}
     WHERE 1 = 1

pkg/storage/unified/sql/data/resource_insert.sql

@@ -7,6 +7,7 @@ INSERT INTO {{ .Ident "resource" }}
         {{ .Ident "namespace" }},
         {{ .Ident "name" }},
 
+        {{ .Ident "previous_resource_version" }},
         {{ .Ident "value" }},
         {{ .Ident "action" }}
     )
@@ -17,6 +18,7 @@ INSERT INTO {{ .Ident "resource" }}
         {{ .Arg .WriteEvent.Key.Namespace }},
         {{ .Arg .WriteEvent.Key.Name }},
 
+        {{ .Arg .WriteEvent.PreviousRV }},
         {{ .Arg .WriteEvent.Value }},
         {{ .Arg .WriteEvent.Type }}
     )

pkg/storage/unified/sql/data/resource_version_insert.sql

@@ -8,6 +8,6 @@ INSERT INTO {{ .Ident "resource_version" }}
     VALUES (
         {{ .Arg .Group }},
         {{ .Arg .Resource }},
-        1
+        2
     )
 ;

pkg/storage/unified/sql/db/migrations/resource_mig.go

@@ -10,8 +10,7 @@ func initResourceTables(mg *migrator.Migrator) string {
 	marker := "Initialize resource tables"
 	mg.AddMigration(marker, &migrator.RawSQLMigration{})
 
-	tables := []migrator.Table{}
-	tables = append(tables, migrator.Table{
+	resource_table := migrator.Table{
 		Name: "resource",
 		Columns: []*migrator.Column{
 			// primary identifier
@@ -33,9 +32,8 @@ func initResourceTables(mg *migrator.Migrator) string {
 		Indices: []*migrator.Index{
 			{Cols: []string{"namespace", "group", "resource", "name"}, Type: migrator.UniqueIndex},
 		},
-	})
-
-	tables = append(tables, migrator.Table{
+	}
+	resource_history_table := migrator.Table{
 		Name: "resource_history",
 		Columns: []*migrator.Column{
 			// primary identifier
@@ -62,7 +60,9 @@ func initResourceTables(mg *migrator.Migrator) string {
 			// index to support watch poller
 			{Cols: []string{"resource_version"}, Type: migrator.IndexType},
 		},
-	})
+	}
+
+	tables := []migrator.Table{resource_table, resource_history_table}
 
 	// tables = append(tables, migrator.Table{
 	// 	Name: "resource_label_set",
@@ -97,5 +97,13 @@ func initResourceTables(mg *migrator.Migrator) string {
 		}
 	}
 
+	mg.AddMigration("Add column previous_resource_version in resource_history", migrator.NewAddColumnMigration(resource_history_table, &migrator.Column{
+		Name: "previous_resource_version", Type: migrator.DB_BigInt, Nullable: false,
+	}))
+
+	mg.AddMigration("Add column previous_resource_version in resource", migrator.NewAddColumnMigration(resource_table, &migrator.Column{
+		Name: "previous_resource_version", Type: migrator.DB_BigInt, Nullable: false,
+	}))
+
 	return marker
 }

pkg/storage/unified/sql/queries.go

@@ -70,6 +70,7 @@ func (r sqlResourceRequest) Validate() error {
 type historyPollResponse struct {
 	Key             resource.ResourceKey
 	ResourceVersion int64
+	PreviousRV      int64
 	Value           []byte
 	Action          int
 }
@@ -101,6 +102,7 @@ func (r *sqlResourceHistoryPollRequest) Results() (*historyPollResponse, error)
 			Name:      r.Response.Key.Name,
 		},
 		ResourceVersion: r.Response.ResourceVersion,
+		PreviousRV:      r.Response.PreviousRV,
 		Value:           r.Response.Value,
 		Action:          r.Response.Action,
 	}, nil

pkg/storage/unified/sql/queries_test.go

@@ -104,6 +104,18 @@ func TestUnifiedStorageQueries(t *testing.T) {
 					},
 				},
 			},
+			sqlResourceHistoryPoll: {
+				{
+					Name: "single path",
+					Data: &sqlResourceHistoryPollRequest{
+						SQLTemplate:          mocks.NewTestingSQLTemplate(),
+						Resource:             "res",
+						Group:                "group",
+						SinceResourceVersion: 1234,
+						Response:             new(historyPollResponse),
+					},
+				},
+			},
 
 			sqlResourceUpdateRV: {
 				{
@@ -143,7 +155,8 @@ func TestUnifiedStorageQueries(t *testing.T) {
 					Data: &sqlResourceRequest{
 						SQLTemplate: mocks.NewTestingSQLTemplate(),
 						WriteEvent: resource.WriteEvent{
-							Key: &resource.ResourceKey{},
+							Key:        &resource.ResourceKey{},
+							PreviousRV: 1234,
 						},
 					},
 				},

pkg/storage/unified/sql/testdata/mysql--resource_history_insert-insert into resource_history.sql

@@ -5,6 +5,7 @@ INSERT INTO `resource_history`
         `resource`,
         `namespace`,
         `name`,
+        `previous_resource_version`,
         `value`,
         `action`
     )
@@ -14,6 +15,7 @@ INSERT INTO `resource_history`
         '',
         '',
         '',
+        1234,
         '[]',
         'UNKNOWN'
     )

pkg/storage/unified/sql/testdata/mysql--resource_history_poll-single path.sql

@@ -0,0 +1,16 @@
+SELECT
+    `resource_version`,
+    `namespace`,
+    `group`,
+    `resource`,
+    `name`,
+    `value`,
+    `action`,
+    `previous_resource_version`
+    FROM `resource_history`
+    WHERE 1 = 1
+    AND `group` = 'group'
+    AND `resource` = 'res'
+    AND `resource_version` > 1234
+    ORDER BY `resource_version` ASC
+;

pkg/storage/unified/sql/testdata/mysql--resource_insert-simple.sql

@@ -5,6 +5,7 @@ INSERT INTO `resource`
         `resource`,
         `namespace`,
         `name`,
+        `previous_resource_version`,
         `value`,
         `action`
     )
@@ -14,6 +15,7 @@ INSERT INTO `resource`
         'rr',
         'nn',
         'name',
+        123,
         '[]',
         'ADDED'
     )

pkg/storage/unified/sql/testdata/mysql--resource_version_insert-single path.sql

@@ -7,6 +7,6 @@ INSERT INTO `resource_version`
     VALUES (
         '',
         '',
-        1
+        2
     )
 ;

pkg/storage/unified/sql/testdata/postgres--resource_history_insert-insert into resource_history.sql

@@ -5,6 +5,7 @@ INSERT INTO "resource_history"
         "resource",
         "namespace",
         "name",
+        "previous_resource_version",
         "value",
         "action"
     )
@@ -14,6 +15,7 @@ INSERT INTO "resource_history"
         '',
         '',
         '',
+        1234,
         '[]',
         'UNKNOWN'
     )

pkg/storage/unified/sql/testdata/postgres--resource_history_poll-single path.sql

@@ -0,0 +1,16 @@
+SELECT
+    "resource_version",
+    "namespace",
+    "group",
+    "resource",
+    "name",
+    "value",
+    "action",
+    "previous_resource_version"
+    FROM "resource_history"
+    WHERE 1 = 1
+    AND "group" = 'group'
+    AND "resource" = 'res'
+    AND "resource_version" > 1234
+    ORDER BY "resource_version" ASC
+;

pkg/storage/unified/sql/testdata/postgres--resource_insert-simple.sql

@@ -5,6 +5,7 @@ INSERT INTO "resource"
         "resource",
         "namespace",
         "name",
+        "previous_resource_version",
         "value",
         "action"
     )
@@ -14,6 +15,7 @@ INSERT INTO "resource"
         'rr',
         'nn',
         'name',
+        123,
         '[]',
         'ADDED'
     )

pkg/storage/unified/sql/testdata/postgres--resource_version_insert-single path.sql

@@ -7,6 +7,6 @@ INSERT INTO "resource_version"
     VALUES (
         '',
         '',
-        1
+        2
     )
 ;

pkg/storage/unified/sql/testdata/sqlite--resource_history_insert-insert into resource_history.sql

@@ -5,6 +5,7 @@ INSERT INTO "resource_history"
         "resource",
         "namespace",
         "name",
+        "previous_resource_version",
         "value",
         "action"
     )
@@ -14,6 +15,7 @@ INSERT INTO "resource_history"
         '',
         '',
         '',
+        1234,
         '[]',
         'UNKNOWN'
     )

pkg/storage/unified/sql/testdata/sqlite--resource_history_poll-single path.sql

@@ -0,0 +1,16 @@
+SELECT
+    "resource_version",
+    "namespace",
+    "group",
+    "resource",
+    "name",
+    "value",
+    "action",
+    "previous_resource_version"
+    FROM "resource_history"
+    WHERE 1 = 1
+    AND "group" = 'group'
+    AND "resource" = 'res'
+    AND "resource_version" > 1234
+    ORDER BY "resource_version" ASC
+;

pkg/storage/unified/sql/testdata/sqlite--resource_insert-simple.sql

@@ -5,6 +5,7 @@ INSERT INTO "resource"
         "resource",
         "namespace",
         "name",
+        "previous_resource_version",
         "value",
         "action"
     )
@@ -14,6 +15,7 @@ INSERT INTO "resource"
         'rr',
         'nn',
         'name',
+        123,
         '[]',
         'ADDED'
     )

pkg/storage/unified/sql/testdata/sqlite--resource_version_insert-single path.sql

@@ -7,6 +7,6 @@ INSERT INTO "resource_version"
     VALUES (
         '',
         '',
-        1
+        2
     )
 ;