Add state-store backed port allocation by danegsta · Pull Request #159 · microsoft/dcp

danegsta · 2026-05-17T04:30:56Z

DCP currently picks free ports by probing random ephemeral ports and recording recent choices in an MRU file, which still leaves a timing window where another process can claim the port after the probe closes. This change adds a state-store-backed allocation model so DCP instances can coordinate non-ephemeral child-process port reservations through SQLite while keeping the legacy MRU allocator as an isolated fallback.

Summary

Add a port_allocations state-store table and reservation APIs with normalized packed IP addresses, owner identity tracking, inactive-owner cleanup, and wildcard address conflict handling.
Split networking allocation into a public dispatcher plus separate state-store and MRU allocator implementations.
Add configurable state-store allocation ranges, default non-ephemeral scanning, local bind-probe/reservation serialization, and context-aware allocation/reserve/release APIs.
Wire controller startup, service proxy allocation, declared service ports, endpoint reservations, templating, and kubeconfig generation through the updated allocator APIs.
Release state-store reservations when service proxies stop, endpoints are cleaned up, or services are deleted.

Notes for reviewers

The state-store path and legacy MRU path intentionally do not share persistence. State-store allocation writes only SQLite reservations; MRU fallback writes only the MRU file. The new range scanner uses a deterministic randomized stride over the configured range instead of precomputing candidates.

Validation

make test-prereqs && go test -count 1 -parallel 32 ./internal/networking ./internal/statestore ./controllers
make test
make lint

karolz-ms

I am still reviewing the whole PR, but need to take a break and don't want to lose the comments I already made, so here they are.

karolz-ms · 2026-06-02T19:00:14Z

 	return allLocalIps, nil
 }

 func GetEphemeralPortRange() (int, int, bool) {


I know this is not part of your change, but we should add a comment to this public API describing what the "matched" bool return value is

karolz-ms · 2026-06-02T19:13:55Z

+	_, _ = hash.Write([]byte(address))
+	hashValue := hash.Sum64()
+	offset := int(hashValue % uint64(total))
+	step := stateStorePortAllocationCandidateStep(total, hashValue/uint64(total))


Do you really want the hash value to be divided by the total here? If yes, why is this calculation done here as opposed to the body of the step-calculating function?

karolz-ms · 2026-06-02T21:54:21Z

+	candidates := newStateStorePortAllocationCandidateIterator(config.portRanges, string(protocol), address)
+	for {
+		if ctxErr := allocationCtx.Err(); ctxErr != nil {
+			return 0, ctxErr, config.mode == portAllocatorModeStateStoreWithFallback


Nit: we can probably just compute shouldFallback (instead of using config.mode == portAllocatorModeStateStoreWithFallback here and below) before going into for loop. Not for performance, but just to make the code a bit more readable.

karolz-ms · 2026-06-03T22:27:13Z

+		for _, conflictingReservation := range inactiveReservations {
+			deleteErr := deletePortReservation(ctx, conn, conflictingReservation)
+			if deleteErr != nil {
+				return deleteErr


Should the inability to delete inactive reservation cause the new reservation to fail?

karolz-ms · 2026-06-04T16:52:14Z

+			releaseErr = errors.Join(releaseErr, proxyReleaseErr)
+		}
+	}
+	return releaseErr


I am not convinced that the failure to release a port needs to be handled in any special way beyond logging it. The other mechanisms we have (reaping orphaned port reservations in particular) should be enough to keep the user machine usable.

karolz-ms · 2026-06-04T16:58:17Z

+
+	releaseExistingErr := stopProxiesAndReleasePortReservations(ctx, psd.proxies, log)
+	psd.proxiesStopped = len(psd.proxies) > 0
+	if releaseExistingErr != nil {


See my comment on stopProxiesAndReleasePortReservations()--I question whether we should fail if port reservations cannot be released.

karolz-ms · 2026-06-04T16:59:25Z

 	if portAllocationErr != nil {
-		stopProxies(proxies, log)
+		releaseErr := stopProxiesAndReleasePortReservations(ctx, proxies, log)
+		if releaseErr != nil {


Again, consider making stopProxiesAndReleasePortReservations a "never fails" method

karolz-ms · 2026-06-04T17:12:02Z


-func (r *ServiceReconciler) stopService(svcName types.NamespacedName, svc *apiv1.Service, log logr.Logger) {
-	serviceData, found := r.serviceInfo.LoadAndDelete(svcName)
+func reserveDeclaredServicePort(ctx context.Context, svc *apiv1.Service, log logr.Logger) objectChange {


Not sure if we need this?

If the service (Spec.Port) is using a specific (nonzero) port, then the user took responsibility for selecting that particular port and we should neither "reserve" nor "release" it. It is semantically different from the case of "the user asked for random port and we decided to use 1234, therefore 1234 is reserved by current DCP instance". The only concern with this scenario (user-chosen port) is #26 and we should fix it, but perhaps via a separate PR, and it probably calls for a separate kind of "port reservation".

On the other hand, if the port is 0 (Service should use random port), then trying to "reserve" or "release" it should be a no-op and waste of time.

So it seems in either case what this method does is not what we want.

karolz-ms · 2026-06-04T17:19:35Z

 	}
 }

+func (r *ServiceReconciler) releaseServiceEndpointPortReservations(ctx context.Context, svc *apiv1.Service, log logr.Logger) objectChange {


This is more than unnecessary, this is harmful (as part of service controller).

Whatever created the Endpoint should be responsible for deleting the Endpoint and releasing any port reservations it might have made as part of creating the Endpoint.

karolz-ms · 2026-06-04T18:00:55Z

+ *  Licensed under the MIT License. See LICENSE in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+package controllers


This should go into test/integration package.

karolz-ms · 2026-06-04T18:09:49Z

 			if len(ips) == 0 {
 				return "", fmt.Errorf("could not obtain valid IP address(es) for 'localhost': %w", err)
 			}
 			requestedServiceAddress = networking.IpToString(ips[rand.Intn(len(ips))])


This makes result of getRequestedServiceAddress() non-deterministic, which might cause us take multiple port reservations per single Service instance (e.g. because of multiple reconciliations). I would try to make the result deterministic e.g. choose the IP based on the hash of service name or something along these lines.

karolz-ms · 2026-06-04T18:11:27Z

+		change = r.stopService(ctx, svc.NamespacedName(), &svc, log)
+		change |= releaseDeclaredServicePort(ctx, &svc, log)
+		change |= r.releaseServiceEndpointPortReservations(ctx, &svc, log)
+		if change == noChange {


releaseDeclaredServicePort, releaseServiceEndpointPortReservations, and stopProxiesAndReleasePortReservations all return additionalReconciliationNeeded on any error. The finalizer stays, the Service is undeletable from the cluster, and the controller burns CPU retrying. If state-store is permanently degraded the operator has no way out. Cap the retry attempts (after which we log and proceed), or distinguish transient vs. terminal errors.

karolz-ms · 2026-06-04T18:19:06Z

+	var svc apiv1.Service
+	if getErr := client.Get(ctx, serviceProducer.ServiceNamespacedName(), &svc); getErr != nil {
+		if apierrors.IsNotFound(getErr) {
+			return nil


I think the caller needs to learn about this error... or we skip the service existence check, delete this utility and make the callers call networking.ReserveSpecificPort directly.

karolz-ms · 2026-06-04T18:20:51Z

+	}, false, nil
+}
+
+func allocatePortFromStateStoreRange(ctx context.Context, protocol apiv1.PortProtocol, address string, log logr.Logger) (int32, error, bool) {


More than one function in this file do not return the error as the last return value (non-idiomatic Go).

karolz-ms · 2026-06-04T18:26:10Z

+		`SELECT protocol, address, port,
+			owner_pid, owner_identity_time, updated_at_unix_nano
+		 FROM port_allocations
+		 WHERE protocol = ? AND port = ?`,


Shouldn't it also check the address?

As written here this will do full table scan because the index is on (protocol, address, port)

karolz-ms · 2026-06-04T18:33:31Z

+	allocationTimeout     time.Duration
+}
+
+func getStateStorePortAllocationConfig(log logr.Logger) (*stateStorePortAllocationConfig, bool, error) {


The result of this should probably be cached, because it is called on every allocate/check/reserve/release call.

karolz-ms · 2026-06-04T20:06:47Z

 		if cleanupErr := stateStore.DeleteInactiveResourceLeases(ctrlCtx); cleanupErr != nil {
 			log.Error(cleanupErr, "Failed to clean up inactive state store resource leases")
 		}
+		if cleanupErr := stateStore.DeleteInactivePortReservations(ctrlCtx); cleanupErr != nil {


Let's do this (and maybe the resource leases too) in a separate goroutine. Determining inactive port reservations involves process discovery which is slow

karolz-ms · 2026-06-04T20:57:38Z

+	return reservations, nil
+}
+
+func portReservationAddressesConflict(requestedAddress []byte, reservedAddress []byte) bool {


portReservationAddressesConflict short-circuits on length mismatch, but the conflict message blames "another DCP process." When the same DCP process has a wildcard reservation (e.g. 0.0.0.0:1234) and the same process later calls ReserveSpecificPort for 192.168.x.y:1234, the same-owner check in reservePort (reuseSameOwner && sameOwner && exactAddress) doesn't apply because the addresses differ — so the code falls into the active = resourceLeaseOwnerIsActive(...) branch (true, it's us) and returns ErrPortReservationHeld, which the allocator surfaces as "port %d is already reserved by another DCP process". The message is wrong in this case and operationally confusing. Detect same-owner and emit a clearer message ("conflicts with this process's own wildcard reservation").

Replace fragile random ephemeral port selection with a configurable state-store backed allocation path while keeping the MRU allocator as an isolated fallback. The new allocator stores normalized IP reservations, handles wildcard address conflicts, serializes local bind probes with reservation writes, and cleans up reservations for stopped service proxies and deleted service endpoints. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Release pre-reserved endpoint ports when Endpoint persistence fails and prevent stopped proxy data from being reused after reservation release errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Explain why the state-store allocator defaults to 20000-32767. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Define named constants for the state-store allocator's default scanning range. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Avoid forcing unmatched Windows and macOS ephemeral fallback ranges to Linux's default minimum. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

karolz-ms reviewed Jun 2, 2026

View reviewed changes

Comment thread internal/networking/port_allocator.go

karolz-ms reviewed Jun 3, 2026

View reviewed changes

Comment thread internal/statestore/migrations/000002_port_allocations.up.sql Outdated

karolz-ms reviewed Jun 3, 2026

View reviewed changes

Comment thread internal/statestore/ports.go Outdated

karolz-ms reviewed Jun 3, 2026

View reviewed changes

Comment thread internal/statestore/ports.go Outdated

karolz-ms reviewed Jun 3, 2026

View reviewed changes

Comment thread internal/statestore/ports.go Outdated

karolz-ms reviewed Jun 3, 2026

View reviewed changes

Comment thread internal/statestore/ports.go Outdated

karolz-ms reviewed Jun 4, 2026

View reviewed changes

Comment thread controllers/service_controller.go Outdated

karolz-ms reviewed Jun 4, 2026

View reviewed changes

Comment thread controllers/service_controller.go Outdated

karolz-ms reviewed Jun 4, 2026

View reviewed changes

Comment thread controllers/service_controller.go Outdated

karolz-ms reviewed Jun 4, 2026

View reviewed changes

Comment thread internal/statestore/ports.go Outdated

karolz-ms reviewed Jun 4, 2026

View reviewed changes

Comment thread internal/statestore/ports.go Outdated

karolz-ms reviewed Jun 4, 2026

View reviewed changes

danegsta and others added 3 commits June 10, 2026 16:11

Fix port reservation cleanup on controller failures

b1ac039

Release pre-reserved endpoint ports when Endpoint persistence fails and prevent stopped proxy data from being reused after reservation release errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Document default port allocation range

f8216e6

Explain why the state-store allocator defaults to 20000-32767. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

danegsta and others added 4 commits June 10, 2026 16:11

Extract default port allocation range constants

2e2ea0d

Define named constants for the state-store allocator's default scanning range. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Use OS default ephemeral fallback range

266fc0c

Avoid forcing unmatched Windows and macOS ephemeral fallback ranges to Linux's default minimum. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Refine port reservation APIs

94d5c7c

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Remove unused port reservation query

7909b66

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

danegsta force-pushed the danegsta/port-allocation-plan branch from 58e497e to 7909b66 Compare June 10, 2026 23:12

Uh oh!

Conversation

danegsta commented May 17, 2026

Summary

Notes for reviewers

Validation

Uh oh!

karolz-ms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karolz-ms Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karolz-ms Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

karolz-ms Jun 4, 2026 •

edited

Loading

karolz-ms Jun 4, 2026 •

edited

Loading