stream: slot and FES should not be created if the publication creation fails (zalando#2704)

idanovinda · web-flow · commit 94d36327ba40 · 2024-08-02T15:09:37.000+02:00
* slot should not be created if the publication creation fails
* not create FES resource when slot doesn't exist
diff --git a/e2e/tests/test_e2e.py b/e2e/tests/test_e2e.py
@@ -2041,6 +2041,20 @@ def test_stream_resources(self):
                                 "recoveryEventType": "test-event-dlq"
                             }
                         }
+                    },
+                    {
+                        "applicationId": "test-app2",
+                        "batchSize": 100,
+                        "database": "foo",
+                        "enableRecovery": True,
+                        "tables": {
+                            "test_non_exist_table": {
+                                "eventType": "test-event",
+                                "idColumn": "id",
+                                "payloadColumn": "payload",
+                                "recoveryEventType": "test-event-dlq"
+                            }
+                        }
                     }
                 ]
             }
@@ -2064,6 +2078,18 @@ def test_stream_resources(self):
                 "zalando.org", "v1", "default", "fabriceventstreams", label_selector="cluster-name=acid-minimal-cluster")["items"]), 1,
                 "Could not find Fabric Event Stream resource", 10, 5)
 
+        # check if the non-existing table in the stream section does not create a publication and slot
+        get_publication_query_not_exist_table = """
+            SELECT * FROM pg_publication WHERE pubname = 'fes_foo_test_app2';
+        """
+        get_slot_query_not_exist_table = """
+            SELECT * FROM pg_replication_slots WHERE slot_name = 'fes_foo_test_app2';
+        """
+        self.eventuallyEqual(lambda: len(self.query_database(leader.metadata.name, "foo", get_publication_query_not_exist_table)), 0,
+            "Publication is created for non-existing tables", 10, 5)
+        self.eventuallyEqual(lambda: len(self.query_database(leader.metadata.name, "foo", get_slot_query_not_exist_table)), 0,
+            "Replication slot is created for non-existing tables", 10, 5)
+
         # grant create and ownership of test_table to foo_user, reset search path to default
         grant_permission_foo_user = """
             GRANT CREATE ON DATABASE foo TO foo_user;
diff --git a/pkg/cluster/streams.go b/pkg/cluster/streams.go
@@ -134,7 +134,6 @@ func (c *Cluster) syncPublication(dbName string, databaseSlotsList map[string]za
 		} else if currentTables != tableList {
 			alterPublications[slotName] = tableList
 		}
-		(*slotsToSync)[slotName] = slotAndPublication.Slot
 	}
 
 	// check if there is any deletion
@@ -148,24 +147,30 @@ func (c *Cluster) syncPublication(dbName string, databaseSlotsList map[string]za
 		return nil
 	}
 
+	var errorMessage error = nil
 	for publicationName, tables := range createPublications {
 		if err = c.executeCreatePublication(publicationName, tables); err != nil {
-			return fmt.Errorf("creation of publication %q failed: %v", publicationName, err)
+			errorMessage = fmt.Errorf("creation of publication %q failed: %v", publicationName, err)
+			continue
 		}
+		(*slotsToSync)[publicationName] = databaseSlotsList[publicationName].Slot
 	}
 	for publicationName, tables := range alterPublications {
 		if err = c.executeAlterPublication(publicationName, tables); err != nil {
-			return fmt.Errorf("update of publication %q failed: %v", publicationName, err)
+			errorMessage = fmt.Errorf("update of publication %q failed: %v", publicationName, err)
+			continue
 		}
+		(*slotsToSync)[publicationName] = databaseSlotsList[publicationName].Slot
 	}
 	for _, publicationName := range deletePublications {
-		(*slotsToSync)[publicationName] = nil
 		if err = c.executeDropPublication(publicationName); err != nil {
-			return fmt.Errorf("deletion of publication %q failed: %v", publicationName, err)
+			errorMessage = fmt.Errorf("deletion of publication %q failed: %v", publicationName, err)
+			continue
 		}
+		(*slotsToSync)[publicationName] = nil
 	}
 
-	return nil
+	return errorMessage
 }
 
 func (c *Cluster) generateFabricEventStream(appId string) *zalandov1.FabricEventStream {
@@ -390,15 +395,15 @@ func (c *Cluster) syncStreams() error {
 	}
 
 	// finally sync stream CRDs
-	err = c.createOrUpdateStreams()
+	err = c.createOrUpdateStreams(slotsToSync)
 	if err != nil {
 		return err
 	}
 
 	return nil
 }
 
-func (c *Cluster) createOrUpdateStreams() error {
+func (c *Cluster) createOrUpdateStreams(createdSlots map[string]map[string]string) error {
 
 	// fetch different application IDs from streams section
 	// there will be a separate event stream resource for each ID
@@ -413,7 +418,7 @@ func (c *Cluster) createOrUpdateStreams() error {
 		return fmt.Errorf("could not list of FabricEventStreams: %v", err)
 	}
 
-	for _, appId := range appIds {
+	for idx, appId := range appIds {
 		streamExists := false
 
 		// update stream when it exists and EventStreams array differs
@@ -435,6 +440,12 @@ func (c *Cluster) createOrUpdateStreams() error {
 		}
 
 		if !streamExists {
+			// check if there is any slot with the applicationId
+			slotName := getSlotName(c.Spec.Streams[idx].Database, appId)
+			if _, exists := createdSlots[slotName]; !exists {
+				c.logger.Warningf("no slot %s with applicationId %s exists, skipping event stream creation", slotName, appId)
+				continue
+			}
 			c.logger.Infof("event streams with applicationId %s do not exist, create it", appId)
 			streamCRD, err := c.createStreams(appId)
 			if err != nil {
diff --git a/pkg/cluster/streams_test.go b/pkg/cluster/streams_test.go
@@ -41,6 +41,10 @@ var (
 	fesUser     string = fmt.Sprintf("%s%s", constants.EventStreamSourceSlotPrefix, constants.UserRoleNameSuffix)
 	slotName    string = fmt.Sprintf("%s_%s_%s", constants.EventStreamSourceSlotPrefix, dbName, strings.Replace(appId, "-", "_", -1))
 
+	fakeCreatedSlots map[string]map[string]string = map[string]map[string]string{
+		slotName: {},
+	}
+
 	pg = acidv1.Postgresql{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "Postgresql",
@@ -222,7 +226,7 @@ func TestGenerateFabricEventStream(t *testing.T) {
 	assert.NoError(t, err)
 
 	// create the streams
-	err = cluster.createOrUpdateStreams()
+	err = cluster.createOrUpdateStreams(fakeCreatedSlots)
 	assert.NoError(t, err)
 
 	// compare generated stream with expected stream
@@ -248,7 +252,7 @@ func TestGenerateFabricEventStream(t *testing.T) {
 	}
 
 	// sync streams once again
-	err = cluster.createOrUpdateStreams()
+	err = cluster.createOrUpdateStreams(fakeCreatedSlots)
 	assert.NoError(t, err)
 
 	streams, err = cluster.KubeClient.FabricEventStreams(namespace).List(context.TODO(), listOptions)
@@ -397,7 +401,7 @@ func TestUpdateFabricEventStream(t *testing.T) {
 	assert.NoError(t, err)
 
 	// now create the stream
-	err = cluster.createOrUpdateStreams()
+	err = cluster.createOrUpdateStreams(fakeCreatedSlots)
 	assert.NoError(t, err)
 
 	// change specs of streams and patch CRD
@@ -419,7 +423,7 @@ func TestUpdateFabricEventStream(t *testing.T) {
 	assert.NoError(t, err)
 
 	cluster.Postgresql.Spec = pgPatched.Spec
-	err = cluster.createOrUpdateStreams()
+	err = cluster.createOrUpdateStreams(fakeCreatedSlots)
 	assert.NoError(t, err)
 
 	// compare stream returned from API with expected stream
@@ -448,7 +452,7 @@ func TestUpdateFabricEventStream(t *testing.T) {
 	assert.NoError(t, err)
 
 	cluster.Postgresql.Spec = pgPatched.Spec
-	err = cluster.createOrUpdateStreams()
+	err = cluster.createOrUpdateStreams(fakeCreatedSlots)
 	assert.NoError(t, err)
 
 	result = cluster.generateFabricEventStream(appId)
@@ -466,7 +470,7 @@ func TestUpdateFabricEventStream(t *testing.T) {
 	assert.NoError(t, err)
 
 	cluster.Postgresql.Spec = pgUpdated.Spec
-	cluster.createOrUpdateStreams()
+	cluster.createOrUpdateStreams(fakeCreatedSlots)
 
 	streamList, err := cluster.KubeClient.FabricEventStreams(namespace).List(context.TODO(), listOptions)
 	if len(streamList.Items) > 0 || err != nil {

Original file line number	Diff line number	Diff line change
`@@ -134,7 +134,6 @@ func (c *Cluster) syncPublication(dbName string, databaseSlotsList map[string]za`
`134`	`134`	`} else if currentTables != tableList {`
`135`	`135`	`alterPublications[slotName] = tableList`
`136`	`136`	`}`
`137`		`- (*slotsToSync)[slotName] = slotAndPublication.Slot`
`138`	`137`	`}`
`139`	`138`
`140`	`139`	`// check if there is any deletion`
`@@ -148,24 +147,30 @@ func (c *Cluster) syncPublication(dbName string, databaseSlotsList map[string]za`
`148`	`147`	`return nil`
`149`	`148`	`}`
`150`	`149`
	`150`	`+ var errorMessage error = nil`
`151`	`151`	`for publicationName, tables := range createPublications {`
`152`	`152`	`if err = c.executeCreatePublication(publicationName, tables); err != nil {`
`153`		`- return fmt.Errorf("creation of publication %q failed: %v", publicationName, err)`
	`153`	`+ errorMessage = fmt.Errorf("creation of publication %q failed: %v", publicationName, err)`
	`154`	`+ continue`
`154`	`155`	`}`
	`156`	`+ (*slotsToSync)[publicationName] = databaseSlotsList[publicationName].Slot`
`155`	`157`	`}`
`156`	`158`	`for publicationName, tables := range alterPublications {`
`157`	`159`	`if err = c.executeAlterPublication(publicationName, tables); err != nil {`
`158`		`- return fmt.Errorf("update of publication %q failed: %v", publicationName, err)`
	`160`	`+ errorMessage = fmt.Errorf("update of publication %q failed: %v", publicationName, err)`
	`161`	`+ continue`
`159`	`162`	`}`
	`163`	`+ (*slotsToSync)[publicationName] = databaseSlotsList[publicationName].Slot`
`160`	`164`	`}`
`161`	`165`	`for _, publicationName := range deletePublications {`
`162`		`- (*slotsToSync)[publicationName] = nil`
`163`	`166`	`if err = c.executeDropPublication(publicationName); err != nil {`
`164`		`- return fmt.Errorf("deletion of publication %q failed: %v", publicationName, err)`
	`167`	`+ errorMessage = fmt.Errorf("deletion of publication %q failed: %v", publicationName, err)`
	`168`	`+ continue`
`165`	`169`	`}`
	`170`	`+ (*slotsToSync)[publicationName] = nil`
`166`	`171`	`}`
`167`	`172`
`168`		`- return nil`
	`173`	`+ return errorMessage`
`169`	`174`	`}`
`170`	`175`
`171`	`176`	`func (c Cluster) generateFabricEventStream(appId string) zalandov1.FabricEventStream {`
`@@ -390,15 +395,15 @@ func (c *Cluster) syncStreams() error {`
`390`	`395`	`}`
`391`	`396`
`392`	`397`	`// finally sync stream CRDs`
`393`		`- err = c.createOrUpdateStreams()`
	`398`	`+ err = c.createOrUpdateStreams(slotsToSync)`
`394`	`399`	`if err != nil {`
`395`	`400`	`return err`
`396`	`401`	`}`
`397`	`402`
`398`	`403`	`return nil`
`399`	`404`	`}`
`400`	`405`
`401`		`-func (c *Cluster) createOrUpdateStreams() error {`
	`406`	`+func (c *Cluster) createOrUpdateStreams(createdSlots map[string]map[string]string) error {`
`402`	`407`
`403`	`408`	`// fetch different application IDs from streams section`
`404`	`409`	`// there will be a separate event stream resource for each ID`
`@@ -413,7 +418,7 @@ func (c *Cluster) createOrUpdateStreams() error {`
`413`	`418`	`return fmt.Errorf("could not list of FabricEventStreams: %v", err)`
`414`	`419`	`}`
`415`	`420`
`416`		`- for _, appId := range appIds {`
	`421`	`+ for idx, appId := range appIds {`
`417`	`422`	`streamExists := false`
`418`	`423`
`419`	`424`	`// update stream when it exists and EventStreams array differs`
`@@ -435,6 +440,12 @@ func (c *Cluster) createOrUpdateStreams() error {`
`435`	`440`	`}`
`436`	`441`
`437`	`442`	`if !streamExists {`
	`443`	`+ // check if there is any slot with the applicationId`
	`444`	`+ slotName := getSlotName(c.Spec.Streams[idx].Database, appId)`
	`445`	`+ if _, exists := createdSlots[slotName]; !exists {`
	`446`	`+ c.logger.Warningf("no slot %s with applicationId %s exists, skipping event stream creation", slotName, appId)`
	`447`	`+ continue`
	`448`	`+ }`
`438`	`449`	`c.logger.Infof("event streams with applicationId %s do not exist, create it", appId)`
`439`	`450`	`streamCRD, err := c.createStreams(appId)`
`440`	`451`	`if err != nil {`