NO-ISSUE: Add Lease type to the private API

[jhernand]

Summary

• Add a Lease resource type to the private API, inspired by the Kubernetes Lease concept. This
  is a coordination primitive that will be used to implement leader election for the controller,
  allowing multiple instances to run while only one actively reconciles.
• The Lease message has a LeaseSpec with fields for holder identity, lease duration (using
  google.protobuf.Duration), acquire and renew timestamps, and a transition counter.
• The Leases gRPC service provides standard CRUD plus Signal operations with HTTP annotations
  on /api/private/v1/leases.

Included changes:

• Proto definitions: lease_type.proto and leases_service.proto.
• Generated Go code via buf generate.
• Lease added to the Event oneof payload in event_type.proto.
• Database migration 24_create_leases_tables.up.sql.
• PrivateLeasesServer implementation delegating to GenericServer.
• Server registration in start_grpc_server_cmd.go.
• Lease case in GenericServer.setPayload for event notifications.
• Support for google.protobuf.Duration in the custom JSON encoder.
• Unit tests for the lease server.

Test plan

• ginkgo run -r internal passes (all 34 suites, including 11 new lease server tests).
• buf lint passes.
• go build ./... succeeds.

Summary by CodeRabbit

• New Features
  • Introduced Leases API for distributed coordination and leader election scenarios, providing comprehensive lease management with create, list, retrieve, update, and delete operations.
  • List operations support pagination and filtering for efficient lease discovery.
  • Added signaling operations to coordinate lease state changes.

[openshift-ci-robot]

@jhernand: This pull request explicitly references no jira issue.

Details

In response to this:

Summary

• Add a Lease resource type to the private API, inspired by the Kubernetes Lease concept. This
  is a coordination primitive that will be used to implement leader election for the controller,
  allowing multiple instances to run while only one actively reconciles.
• The Lease message has a LeaseSpec with fields for holder identity, lease duration (using
  google.protobuf.Duration), acquire and renew timestamps, and a transition counter.
• The Leases gRPC service provides standard CRUD plus Signal operations with HTTP annotations
  on /api/private/v1/leases.

Included changes:

• Proto definitions: lease_type.proto and leases_service.proto.
• Generated Go code via buf generate.
• Lease added to the Event oneof payload in event_type.proto.
• Database migration 24_create_leases_tables.up.sql.
• PrivateLeasesServer implementation delegating to GenericServer.
• Server registration in start_grpc_server_cmd.go.
• Lease case in GenericServer.setPayload for event notifications.
• Support for google.protobuf.Duration in the custom JSON encoder.
• Unit tests for the lease server.

Test plan

• ginkgo run -r internal passes (all 34 suites, including 11 new lease server tests).
• buf lint passes.
• go build ./... succeeds.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhernand

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [jhernand]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

Walkthrough

This PR introduces a new private gRPC leases service for distributed coordination. It includes new protobuf definitions for Lease type and LeasesService with CRUD operations, a PostgreSQL migration creating the leases table, a PrivateLeasesServer implementation with builder pattern, integration into the gRPC server startup, JSON encoding support for Duration types, and comprehensive tests validating core functionality.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• NO-ISSUE: Add database operation metrics #372: Metrics registerer plumbing through server builders and gRPC server initialization.

Suggested reviewers

• larsks
• adriengentil

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically summarizes the main addition: a new Lease type to the private API, which is the primary objective across all changed files.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (3)

internal/servers/private_leases_server_test.go (1)

113-288: Good test coverage, but consider adding Signal RPC test.

The behavior tests comprehensively cover Create, List (with variations), Get, Update, and Delete operations. However, there's no test for the Signal RPC.

Consider adding a test case:

It("Signals a lease", func() {
    createResponse, err := leasesServer.Create(ctx, ...)
    Expect(err).ToNot(HaveOccurred())
    
    _, err = leasesServer.Signal(ctx, privatev1.LeasesSignalRequest_builder{
        Id: createResponse.GetObject().GetId(),
    }.Build())
    Expect(err).ToNot(HaveOccurred())
})

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/servers/private_leases_server_test.go` around lines 113 - 288, Add a
new test It("Signals a lease", ...) inside the existing Describe("Behaviour")
block that creates a lease using leasesServer.Create (as in other tests), then
calls leasesServer.Signal with a privatev1.LeasesSignalRequest_builder
containing the created object's Id, and asserts no error is returned; reference
the leasesServer.Signal method and privatev1.LeasesSignalRequest_builder to
locate where to add the call and assertions consistent with other tests.

internal/servers/private_leases_server.go (2)

70-74: Documentation inconsistency: SetTenancyLogic should note that it's mandatory.

SetLogger (line 52) documents that it's mandatory, and Build() validates both logger and tenancyLogic. Consider updating the doc comment for consistency.

📝 Suggested doc update

-// SetTenancyLogic sets the tenancy logic that will be used to determine the tenants for objects.
+// SetTenancyLogic sets the tenancy logic that will be used to determine the tenants for objects.
+// This is mandatory.
 func (b *PrivateLeasesServerBuilder) SetTenancyLogic(value auth.TenancyLogic) *PrivateLeasesServerBuilder {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/servers/private_leases_server.go` around lines 70 - 74, The doc
comment for SetTenancyLogic is inconsistent with SetLogger: update the comment
for the SetTenancyLogic method to state that providing a tenancyLogic is
mandatory (since Build() validates both logger and tenancyLogic), mirroring the
wording/style used in SetLogger so callers know SetTenancyLogic must be set
before Build is called; reference the SetTenancyLogic method and the Build
validation that checks tenancyLogic to align documentation with behavior.

---

43-44: Consider removing unused logger field.

The logger field is assigned during Build() but is never read in any of the PrivateLeasesServer methods. All logging is handled by the embedded generic server. If this field is reserved for future use, consider adding a comment; otherwise, removing it would reduce confusion.

♻️ Suggested cleanup

 type PrivateLeasesServer struct {
 	privatev1.UnimplementedLeasesServer
 
-	logger  *slog.Logger
 	generic *GenericServer[*privatev1.Lease]
 }

And in Build():

 	result = &PrivateLeasesServer{
-		logger:  b.logger,
 		generic: generic,
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/servers/private_leases_server.go` around lines 43 - 44, The struct
field logger on PrivateLeasesServer is never read—remove the unused logger field
declaration and its assignment in Build() (or, if you intend to keep it for
future use, add a clear comment above the field explaining why it’s reserved),
and update any imports/usages (e.g., references in Build() that set logger)
accordingly; keep the embedded generic *GenericServer[*privatev1.Lease] as the
authoritative logger source (PrivateLeasesServer, Build(), logger).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@internal/servers/private_leases_server_test.go`:
- Around line 113-288: Add a new test It("Signals a lease", ...) inside the
existing Describe("Behaviour") block that creates a lease using
leasesServer.Create (as in other tests), then calls leasesServer.Signal with a
privatev1.LeasesSignalRequest_builder containing the created object's Id, and
asserts no error is returned; reference the leasesServer.Signal method and
privatev1.LeasesSignalRequest_builder to locate where to add the call and
assertions consistent with other tests.

In `@internal/servers/private_leases_server.go`:
- Around line 70-74: The doc comment for SetTenancyLogic is inconsistent with
SetLogger: update the comment for the SetTenancyLogic method to state that
providing a tenancyLogic is mandatory (since Build() validates both logger and
tenancyLogic), mirroring the wording/style used in SetLogger so callers know
SetTenancyLogic must be set before Build is called; reference the
SetTenancyLogic method and the Build validation that checks tenancyLogic to
align documentation with behavior.
- Around line 43-44: The struct field logger on PrivateLeasesServer is never
read—remove the unused logger field declaration and its assignment in Build()
(or, if you intend to keep it for future use, add a clear comment above the
field explaining why it’s reserved), and update any imports/usages (e.g.,
references in Build() that set logger) accordingly; keep the embedded generic
*GenericServer[*privatev1.Lease] as the authoritative logger source
(PrivateLeasesServer, Build(), logger).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 82a0895f-4133-4db9-a750-f92fc1ce1099

📥 Commits

Reviewing files that changed from the base of the PR and between a28882f and c2dba8a.

⛔ Files ignored due to path filters (8)

• internal/api/osac/private/v1/event_type.pb.go is excluded by !**/*.pb.go
• internal/api/osac/private/v1/event_type_protoopaque.pb.go is excluded by !**/*.pb.go
• internal/api/osac/private/v1/lease_type.pb.go is excluded by !**/*.pb.go
• internal/api/osac/private/v1/lease_type_protoopaque.pb.go is excluded by !**/*.pb.go
• internal/api/osac/private/v1/leases_service.pb.go is excluded by !**/*.pb.go
• internal/api/osac/private/v1/leases_service.pb.gw.go is excluded by !**/*.pb.gw.go
• internal/api/osac/private/v1/leases_service_grpc.pb.go is excluded by !**/*.pb.go
• internal/api/osac/private/v1/leases_service_protoopaque.pb.go is excluded by !**/*.pb.go

📒 Files selected for processing (9)

• internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go
• internal/database/migrations/24_create_leases_tables.up.sql
• internal/json/json_encoder.go
• internal/servers/generic_server.go
• internal/servers/private_leases_server.go
• internal/servers/private_leases_server_test.go
• proto/private/osac/private/v1/event_type.proto
• proto/private/osac/private/v1/lease_type.proto
• proto/private/osac/private/v1/leases_service.proto