enhance gpu node indexer #328

Omarabdul3ziz · 2023-08-14T13:00:29Z

Description

we kept the periodically running ticker to requery the nodes and upsert the GPUs info each hour.
also for newly added nodes a Postgres trigger is added on the node table which will notify the client with the added node twin id and then the client will ask the node with an RMB call if it has a GPU or not.

Changes

create a new trigger/function in the database setup string
create a listener on the db event and run it in a separate routine

Related Issues

improve the gpu indexing with the change in number of nodes #322

Checklist

Tests included
Build pass
Documentation
Code format and docstring

- use postgres NOTIFY/LISTEN and TRIGGER to requery the nodes each time node table count changes

AbdelrahmanElawady

Looks great, the SQL part I am not sure about it but other changes are OK.

AbdelrahmanElawady · 2023-08-15T11:42:56Z

grid-proxy/internal/gpuindexer/gpuindexer.go

+			payload := notification.Extra
+			twinId, err := strconv.ParseInt(payload, 10, 64)
+			if err != nil {
+				log.Error().Err(err).Msgf("failed to parse twin id %v", twinId)


twinId should be payload

Omarabdul3ziz · 2023-10-16T07:25:13Z

work is done here. converted to draft until it is needed

xmonader · 2023-10-22T08:53:13Z

grid-proxy/internal/explorer/db/postgres.go

-	$$ LANGUAGE plpgsql;`
+	$$ LANGUAGE plpgsql;
+
+	CREATE OR REPLACE FUNCTION notify_node_change() RETURNS trigger AS $notify_node_change$


change the function to notify_nodes_count_changed

xmonader · 2023-10-22T09:08:48Z

grid-proxy/internal/explorer/db/postgres.go

-// NewPostgresDatabase returns a new postgres db client
-func NewPostgresDatabase(host string, port int, user, password, dbname string) (Database, error) {
-	psqlInfo := fmt.Sprintf("host=%s port=%d user=%s "+
+func GetConnectionString(host string, port int, user, password, dbname string) string {


buildConnectionString

xmonader · 2023-10-22T09:09:16Z

grid-proxy/internal/explorer/db/postgres.go

+}
+
+// NewPostgresDatabase returns a new postgres db client
+func NewPostgresDatabase(psqlInfo string) (Database, error) {


connString, psqlInfo doesn't provide any information on the parameter

xmonader · 2023-10-22T09:09:26Z

grid-proxy/internal/explorer/db/postgres.go

-func NewPostgresDatabase(host string, port int, user, password, dbname string) (Database, error) {
-	psqlInfo := fmt.Sprintf("host=%s port=%d user=%s "+
+func GetConnectionString(host string, port int, user, password, dbname string) string {
+	return fmt.Sprintf("host=%s port=%d user=%s "+


why sslmode is disabled?

it was always like this, should we enable it?

xmonader · 2023-10-22T09:12:46Z

grid-proxy/cmds/proxy_server/main.go

@@ -129,7 +131,7 @@ func main() {
 		log.Fatal().Err(err).Msg("failed to create GPU indexer")
 	}

-	indexer.Start(ctx)
+	indexer.Start(ctx, dbConnStr)


the indexer, can already insert to the database, why is this passed?

https://github.com/threefoldtech/tfgrid-sdk-go/blob/development/grid-proxy/internal/gpuindexer/gpuindexer.go#L23

xmonader · 2023-10-22T09:16:54Z

grid-proxy/internal/gpuindexer/gpuindexer.go

+const (
+	resultsBatcherCleanupInterval = 10 * time.Second
+	minListenerReconnectInterval  = 10 * time.Second
+	dbNotificationChannel         = "node_added"


dbNodeAddedNotificationChannel

xmonader · 2023-10-22T09:17:15Z

grid-proxy/internal/gpuindexer/gpuindexer.go

@@ -26,6 +32,7 @@ type NodeGPUIndexer struct {
 	batchSize              int
 	nodesGPUResultsChan    chan []types.NodeGPU
 	nodesGPUBatchesChan    chan []types.NodeGPU
+	nodesChangeChan        chan int64


nodeAddedChan

xmonader · 2023-10-22T09:21:21Z

grid-proxy/internal/gpuindexer/gpuindexer.go

@@ -150,7 +208,7 @@ func (n *NodeGPUIndexer) gpuNodeResultsBatcher(ctx context.Context) {
 	}
 }

-func (n *NodeGPUIndexer) Start(ctx context.Context) {
+func (n *NodeGPUIndexer) Start(ctx context.Context, connStr string) {


I understand why you did it. however this isn't correct, this should be either inferred from n.db or defined as a method on the db struct

went with defining method on the db that will return the connStr

xmonader · 2023-10-22T09:23:59Z

grid-proxy/internal/gpuindexer/gpuindexer.go

+	}
+}
+
+func (n *NodeGPUIndexer) getGPUInfo(ctx context.Context, twinId int64) {


twinId should uint32, and do the type conversion before passing to the function

found it uint64 for the most of the types, so edited it to be uint64 instead

xmonader · 2023-10-22T09:25:45Z

grid-proxy/internal/gpuindexer/gpuindexer.go

+	err := listener.Listen(dbNotificationChannel)
+	if err != nil {
+		log.Error().Err(err).Msg("failed to listen to DB changes")
+		return


if this ever happens, this will fail and will never recover, that startDBListener should always be running, if you exit this routine silently, what do we do? we fall back completely to the 1hr scheduler? is that the intended behavior?

yes this exactly what will happen

xmonader · 2023-10-22T09:26:09Z

grid-proxy/internal/gpuindexer/gpuindexer.go

@@ -44,6 +51,7 @@ func NewNodeGPUIndexer(
 		db:                     db,
 		nodesGPUResultsChan:    make(chan []types.NodeGPU),
 		nodesGPUBatchesChan:    make(chan []types.NodeGPU),
+		nodesChangeChan:        make(chan int64),


I wonder about the type, why not uint32, given that's the twinID type?

- rename channel/trigger - add connStr to the db and its getter -

…-sdk-go into development_proxy_enhanceIndexer

Omarabdul3ziz added 2 commits August 14, 2023 15:58

enhance gpu node indexer:

9911e8a

- use postgres NOTIFY/LISTEN and TRIGGER to requery the nodes each time node table count changes

only index the newly added node

1a54cbe

Omarabdul3ziz marked this pull request as ready for review August 14, 2023 17:56

Omarabdul3ziz requested review from alichaddad and sameh-farouk as code owners August 14, 2023 17:56

AbdelrahmanElawady reviewed Aug 15, 2023

View reviewed changes

fix error message to use the value before conversion

492497f

AbdelrahmanElawady approved these changes Aug 15, 2023

View reviewed changes

rawdaGastan added the grid-proxy belongs to grid proxy label Aug 16, 2023

xmonader requested review from muhamadazmy and xmonader August 30, 2023 20:34

Omarabdul3ziz changed the title ~~enhance gpu node indexer:~~ enhance gpu node indexer Oct 16, 2023

Omarabdul3ziz marked this pull request as draft October 16, 2023 07:25

xmonader reviewed Oct 22, 2023

View reviewed changes

xmonader marked this pull request as ready for review October 22, 2023 09:08

xmonader requested review from MarioBassem, ashraffouda, rawdaGastan and AlaaElattar as code owners October 22, 2023 09:08

xmonader reviewed Oct 22, 2023

View reviewed changes

Omarabdul3ziz added 3 commits October 26, 2023 13:22

resolve comments:

e62f113

- rename channel/trigger - add connStr to the db and its getter -

Merge branch 'development' of https://github.com/threefoldtech/tfgrid…

67b9806

…-sdk-go into development_proxy_enhanceIndexer

update twinId type

8606786

Omarabdul3ziz merged commit 2efb640 into development Oct 26, 2023
17 checks passed

Omarabdul3ziz deleted the development_proxy_enhanceIndexer branch October 26, 2023 11:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhance gpu node indexer #328

enhance gpu node indexer #328

Omarabdul3ziz commented Aug 14, 2023 •

edited

Loading

AbdelrahmanElawady left a comment

AbdelrahmanElawady Aug 15, 2023

Omarabdul3ziz commented Oct 16, 2023

xmonader Oct 22, 2023

xmonader Oct 22, 2023

xmonader Oct 22, 2023

xmonader Oct 22, 2023

Omarabdul3ziz Oct 26, 2023

xmonader Oct 22, 2023

xmonader Oct 22, 2023

xmonader Oct 22, 2023

xmonader Oct 22, 2023

xmonader Oct 22, 2023

Omarabdul3ziz Oct 26, 2023

xmonader Oct 22, 2023

Omarabdul3ziz Oct 26, 2023

xmonader Oct 22, 2023

Omarabdul3ziz Oct 26, 2023

xmonader Oct 22, 2023

enhance gpu node indexer #328

enhance gpu node indexer #328

Conversation

Omarabdul3ziz commented Aug 14, 2023 • edited Loading

Description

Changes

Related Issues

Checklist

AbdelrahmanElawady left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Omarabdul3ziz commented Oct 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Omarabdul3ziz commented Aug 14, 2023 •

edited

Loading