PX Resource Gateway #1621

dgoel-px · 2024-08-05T23:15:32Z

What this PR does / why we need it:

Which issue(s) this PR fixes (optional)
Closes #

Special notes for your reviewer:

Signed-off-by: Divyam Goel <[email protected]>

lpabon

still reviewing...

Dockerfile.proto

Makefile

lpabon

continuing...

Makefile

cmd/px-resource-gateway/px_resource_gateway.go

lpabon

Please also add unit tests to all of these files. You also need e2e tests: create small grpc servers and have a client send requests to tests the full paths.

pkg/px-resource-gateway/priority_queue.go

pkg/px-resource-gateway/px_resource_gateway.go

lpabon · 2024-08-06T01:20:20Z

proto/pxresourcegateway.proto

+    };
+  }
+
+  // KeepAlive sends a heartbeat to the semaphore service to keep the lock alive


This needs a timeout. Keep alive for "how long"? for example 5 secs? etc.

lpabon · 2024-08-06T01:21:45Z

pkg/px-resource-gateway/px_resource_gateway.go

+	return &emptypb.Empty{}, nil
+}
+
+func (s *server) KeepAlive(ctx context.Context, req *pb.KeepAliveRequest) (*emptypb.Empty, error) {


This function is confusing on the side effects and expectations. I added a comment to the .proto file on this rpc. Maybe add more comments on how this function works and expectations as a comment on the .proto file

lpabon · 2024-08-06T01:26:10Z

pkg/px-resource-gateway/semaphore.go

+	// check if the node is at the front of the queue
+	nextResouceId, priority := s.priorityQ.Front()
+	if nextResouceId == "" {
+		panic("Queue is empty")


Can we do something else other than panic here? I would suggest handling this situation

lpabon · 2024-08-06T01:27:44Z

pkg/px-resource-gateway/semaphore.go

@@ -0,0 +1,310 @@
+package server


Please explain the algorithm as comment header in this file to help maintainers (and me as the reviewer)

I'm skipping this file until the algorithm is included.

Signed-off-by: Divyam Goel <[email protected]>

piyush-nimbalkar

Please add UTs to the resource gateway installation and the actual server logic too before you merge the PR.

Makefile

deploy/crds/core_v1_storagecluster_crd.yaml

piyush-nimbalkar · 2024-08-09T03:48:16Z

drivers/storage/portworx/util/util.go

@@ -109,6 +109,8 @@ const (
 	AnnotationPVCControllerSecurePort = pxAnnotationPrefix + "/pvc-controller-secure-port"
 	// AnnotationAutopilotCPU annotation for overriding the default CPU for Autopilot
 	AnnotationAutopilotCPU = pxAnnotationPrefix + "/autopilot-cpu"
+	// AnnotationPxResourceGatewayCPU annotation for overriding the default CPU for PxResourceGateway
+	AnnotationPxResourceGatewayCPU = pxAnnotationPrefix + "/px-resource-gateway-cpu"


Why do we need this if we have resources section in the cluster spec?

Do we need this annotation? I see a couple of valid comments which are resolved w/o a comment. If you had an offline conversation can you mention them here.

I resolved the comments because I had addressed them locally. I have pushed the changes now.

drivers/storage/portworx/component/px_resource_gateway.go

piyush-nimbalkar · 2024-08-09T04:42:58Z

I have looked at only the deployment logic. I am assuming @nrevanna and @lpabon are looking at the actual server implementation.

Docs PR: https://github.com/pure-px/portworx-documentation/pull/977

Signed-off-by: Divyam Goel <[email protected]>

pkg/px-resource-gateway/priority_queue.go

pkg/px-resource-gateway/semaphore.go

Signed-off-by: Divyam Goel <[email protected]>

github-actions · 2024-08-17T01:50:33Z

This PR is stale because it has been in review for 3 days with no activity.

adityadani · 2024-08-19T18:05:52Z

cmd/px-resource-gateway/px_resource_gateway.go

+	// Setup a signal handler
+	signal_handler := util.NewSigIntManager(func() {
+		pxResourceGatewayServer.Stop()
+		os.Remove(PxResourceGatewayServerSocket)
+		os.Exit(0)
+	})
+	err = signal_handler.Start()
+	if err != nil {
+		fmt.Printf("Unable to start signal handler: %v", err)
+		os.Exit(1)
+	}


Also setup memory and cpu profiling, and then you can add a signal handler for SIGUSR1 to dump the heap and stack profile -

operator/cmd/operator/operator.go

Line 148 in 303cc37

log.Infof("pprof profiling is enabled, creating profiling server at port %v", defaultPprofPort)

adityadani · 2024-08-19T18:07:23Z

drivers/storage/portworx/util/util.go

@@ -109,6 +109,8 @@ const (
 	AnnotationPVCControllerSecurePort = pxAnnotationPrefix + "/pvc-controller-secure-port"
 	// AnnotationAutopilotCPU annotation for overriding the default CPU for Autopilot
 	AnnotationAutopilotCPU = pxAnnotationPrefix + "/autopilot-cpu"
+	// AnnotationPxResourceGatewayCPU annotation for overriding the default CPU for PxResourceGateway
+	AnnotationPxResourceGatewayCPU = pxAnnotationPrefix + "/px-resource-gateway-cpu"


Do we need this annotation? I see a couple of valid comments which are resolved w/o a comment. If you had an offline conversation can you mention them here.

pkg/px-resource-gateway/px_resource_gateway.go

pkg/px-resource-gateway/semaphore.go

adityadani · 2024-08-19T18:24:22Z

pkg/px-resource-gateway/semaphore.go

+
+var (
+	NLocks                  = 10
+	ConfigMapUpdateInterval = time.Second * 1


IMO 1 sec is still too aggressive for a single client to invoke a k8s api. In comparison, our default operator reconcilor will wake up every 30s and then make a bunch of k8s apis if required.

If the goal is to just persist the locks so that if this pod crashes we can recover then isn't 30s good enough?

pkg/px-resource-gateway/semaphore.go

adityadani · 2024-08-19T18:32:57Z

pkg/px-resource-gateway/semaphore.go

+		data[clientId] = fmt.Sprintf("%d", lockId)
+	}
+
+	if !isUpdateRequired(data, s.configMap.Data) {


Is the goal to re-initialize the s.locks from s.configMap.Data when this process starts?
Right now, I don't see this config map's contents being used anywhere

pkg/px-resource-gateway/px_resource_gateway.go

adityadani · 2024-08-19T19:26:07Z

pkg/px-resource-gateway/semaphore.go

+		if _, ok := removedNode[clientId]; ok { // already removed
+			continue
+		}
+		s.priorityQ.Remove(clientId)


I am missing something here, why not remove a dead node unconditionally from the priorityQueue if it was part of s.locks ?

Signed-off-by: Divyam Goel <[email protected]>

add px-resource-gateway service

8da22a8

Signed-off-by: Divyam Goel <[email protected]>

github-actions bot added area/build area/cmd area/vendor labels Aug 5, 2024

lpabon requested changes Aug 6, 2024

View reviewed changes

Dockerfile.proto Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

lpabon requested changes Aug 6, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

cmd/px-resource-gateway/px_resource_gateway.go Outdated Show resolved Hide resolved

cmd/px-resource-gateway/px_resource_gateway.go Outdated Show resolved Hide resolved

lpabon requested changes Aug 6, 2024

View reviewed changes

dgoel-px added 3 commits August 8, 2024 00:55

deployment and reviews

5c4c2d3

Signed-off-by: Divyam Goel <[email protected]>

reviews

7f8e76c

Signed-off-by: Divyam Goel <[email protected]>

minor changes

1d4cc0f

Signed-off-by: Divyam Goel <[email protected]>

github-actions bot added area/components area/specs labels Aug 8, 2024

dgoel-px requested a review from lpabon August 9, 2024 00:11

github-actions bot added the stage/in-review label Aug 9, 2024

nrevanna requested a review from piyush-nimbalkar August 9, 2024 00:12

piyush-nimbalkar requested a review from a team August 9, 2024 00:30

piyush-nimbalkar suggested changes Aug 9, 2024

View reviewed changes

harsh-px and others added 3 commits August 12, 2024 13:54

PWX-38379 change pool and volume resize alerts from warning to info

82c84a6

Docs PR: https://github.com/pure-px/portworx-documentation/pull/977

Removing empty response

8f7a776

rename proto and add health

cad773b

Signed-off-by: Divyam Goel <[email protected]>

sangleganesh reviewed Aug 13, 2024

View reviewed changes

cleanup

b459d2a

Signed-off-by: Divyam Goel <[email protected]>

github-actions bot added the stage/in-review-stale label Aug 17, 2024

adityadani reviewed Aug 19, 2024

View reviewed changes

px-security, cleanup, lockHoldTimeout, LoadResources, and others

7c296a0

Signed-off-by: Divyam Goel <[email protected]>

github-actions bot added the area/controller label Aug 20, 2024

dgoel-px added 2 commits August 22, 2024 21:57

rename to resource-gateway

cd0b271

Signed-off-by: Divyam Goel <[email protected]>

refactor and unit tests

304b688

Signed-off-by: Divyam Goel <[email protected]>

github-actions bot added the area/portworx label Aug 23, 2024

dgoel-px added 2 commits August 24, 2024 03:27

tests for semaphore_server

0bdfad7

Signed-off-by: Divyam Goel <[email protected]>

fix available permis && unit test refactor

cfac9b8

Signed-off-by: Divyam Goel <[email protected]>

dgoel-px requested review from adityadani, piyush-nimbalkar and sangleganesh September 5, 2024 00:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PX Resource Gateway #1621

PX Resource Gateway #1621

dgoel-px commented Aug 5, 2024

lpabon left a comment

lpabon left a comment

lpabon left a comment

lpabon Aug 6, 2024

lpabon Aug 6, 2024

lpabon Aug 6, 2024

lpabon Aug 6, 2024

lpabon Aug 6, 2024

piyush-nimbalkar left a comment

piyush-nimbalkar Aug 9, 2024

adityadani Aug 19, 2024

dgoel-px Aug 20, 2024

piyush-nimbalkar commented Aug 9, 2024

github-actions bot commented Aug 17, 2024

adityadani Aug 19, 2024

adityadani Aug 19, 2024

adityadani Aug 19, 2024

adityadani Aug 19, 2024

adityadani Aug 19, 2024

PX Resource Gateway #1621

Are you sure you want to change the base?

PX Resource Gateway #1621

Conversation

dgoel-px commented Aug 5, 2024

lpabon left a comment

Choose a reason for hiding this comment

lpabon left a comment

Choose a reason for hiding this comment

lpabon left a 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

piyush-nimbalkar left a 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

piyush-nimbalkar commented Aug 9, 2024

github-actions bot commented Aug 17, 2024

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