Skip to content

operator: support customized storage class name (PROJQUAY-4141) #1045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

dmesser
Copy link
Contributor

@dmesser dmesser commented Jun 6, 2025

This introduces a new, optional override for the managed components postgres and clairPostgres called storageClassName that allows the user to customize the storage class used to create the PVC.

The operator will now also track the health of the PVC in a dedicated fashion, observing its status and correlate potential failure events up to the component's status condition. For that the operator now features an indexer for namespace events.

Furthermore the verification logic between the components postgres and clairPostgres has been consolidated and the unit tests have been refactored to avoid code duplication between the largely identical test suite of the two components.

Summary by Sourcery

Introduce optional storageClassName override for Postgres and ClairPostgres PVCs, track PVC provisioning status via indexed events, and consolidate database component health checks into a unified function

New Features:

  • Allow customizing PVC storageClassName for managed Postgres and ClairPostgres components
  • Add PVC health monitoring and correlate provisioning events to component status

Enhancements:

  • Consolidate Postgres and ClairPostgres status checks into a shared helper that also evaluates PVC readiness
  • Extend middleware to apply storageClassName override alongside volume size adjustments
  • Add field indexer for Kubernetes Event involvedObject.uid to enable PVC event lookup
  • Update CRD schema and deep-copy logic to support storageClassName override

Documentation:

  • Provide sample YAML demonstrating storageClassName overrides

Tests:

  • Add unit tests for storageClassName override in PVC processing
  • Add comprehensive component status tests covering PVC pending, provisioning failures, and healthy states
  • Refactor existing tests to remove duplication between Postgres and ClairPostgres suites

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've reviewed this pull request using the Sourcery rules engine

@dmesser dmesser requested a review from Copilot June 7, 2025 21:23
Copilot

This comment was marked as outdated.

dmesser added 2 commits June 7, 2025 23:36
Signed-off-by: dmesser <[email protected]>
@dmesser dmesser requested a review from Copilot June 7, 2025 21:43
Copilot

This comment was marked as outdated.

@dmesser
Copy link
Contributor Author

dmesser commented Jun 8, 2025

/test ocp-latest-e2e

@dmesser dmesser requested review from Copilot and jonathankingfc June 8, 2025 22:22
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Introduce customizable storageClassName overrides for Postgres and ClairPostgres PVCs, track PVC provisioning health via indexed events, and consolidate component status logic and tests.

  • Add optional storageClassName override in middleware and CRD, with sample YAML and deep-copy support
  • Consolidate Postgres and ClairPostgres status checks into CheckDatabaseDeploymentAndPVCStatus, backed by unified tests
  • Register a field indexer for PVC events and update reconciliation error messaging

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
pkg/middleware/middleware_test.go Add tests for PVC storageClassName override
pkg/middleware/middleware.go Apply storageClassNameOverride in Process
pkg/cmpstatus/quaypostgres_test.go Remove legacy Postgres status tests
pkg/cmpstatus/quaypostgres.go Use shared CheckDatabaseDeploymentAndPVCStatus for Postgres
pkg/cmpstatus/evaluator_test.go Update expected error messages to include deployment names
pkg/cmpstatus/database_status_test.go Introduce unified database status tests covering PVC scenarios
pkg/cmpstatus/database_status.go Implement CheckDatabaseDeploymentAndPVCStatus consolidating status logic
pkg/cmpstatus/clairpostgres_test.go Remove legacy ClairPostgres status tests
pkg/cmpstatus/clairpostgres.go Use shared CheckDatabaseDeploymentAndPVCStatus for ClairPostgres
main.go Register field indexer for Event involvedObject.uid
go.mod Add direct golang.org/x/text dependency and remove duplicate indirect
controllers/quay/quayregistry_controller.go Enhance object creation error messages
config/samples/overrides.quayregistry.yaml Add sample YAML demonstrating storageClassName overrides
config/crd/bases/quay.redhat.com_quayregistries.yaml Extend CRD schema with storageClassName property
bundle/manifests/quayregistries.crd.yaml Sync CRD bundle with storageClassName schema updates
apis/quay/v1/zz_generated.deepcopy.go Generate deep-copy support for storageClassName override
apis/quay/v1/quayregistry_types_test.go Add validation tests for unsupported storageClassName overrides
apis/quay/v1/quayregistry_types.go Add storageClassName override support and validation logic
Comments suppressed due to low confidence (3)

pkg/cmpstatus/database_status_test.go:111

  • Add a test case covering the scenario where the PersistentVolumeClaim is not found. This should assert a Condition with Reason ComponentNotReady and a message like "PersistentVolumeClaim not found".
testCases := []testCase{

config/crd/bases/quay.redhat.com_quayregistries.yaml:983

  • Mark the storageClassName schema entry as nullable (add nullable: true or the equivalent kubebuilder annotation) so explicit null values are allowed in the CRD.
storageClassName:

pkg/middleware/middleware.go:234

  • [nitpick] Consider renaming the variable pvcstorage to pvcStorage to follow Go naming conventions (camelCase for multi-word names).
pvcstorage := pvc.Spec.Resources.Requests.Storage()

Comment on lines +122 to +136
for _, event := range eventList.Items {
if event.Reason == "ProvisioningFailed" {
return qv1.Condition{
Type: componentReadyType,
Status: metav1.ConditionFalse,
Reason: qv1.ConditionReasonPVCProvisioningFailed,
Message: fmt.Sprintf(
"%s PVC %s provisioning failed: %s",
componentName,
pvc.Name,
event.Message,
),
LastUpdateTime: metav1.NewTime(time.Now()),
}, nil
}
Copy link
Preview

Copilot AI Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Refactor the event loop and ProvisioningFailed check into a helper function to simplify CheckDatabaseDeploymentAndPVCStatus and improve readability.

Suggested change
for _, event := range eventList.Items {
if event.Reason == "ProvisioningFailed" {
return qv1.Condition{
Type: componentReadyType,
Status: metav1.ConditionFalse,
Reason: qv1.ConditionReasonPVCProvisioningFailed,
Message: fmt.Sprintf(
"%s PVC %s provisioning failed: %s",
componentName,
pvc.Name,
event.Message,
),
LastUpdateTime: metav1.NewTime(time.Now()),
}, nil
}
if cond := checkProvisioningFailedEvent(eventList.Items, componentName, pvc.Name); cond != nil {
return *cond, nil

Copilot uses AI. Check for mistakes.

@jonathankingfc
Copy link
Collaborator

@dmesser The code logic itself looks good to me, but I would like to test this in a live cluster as well for verification. Which storage classes were tested initially, and is there any additional cases that we would like to verify? Ideally we would also have an e2e test for the PVC provisioning status.

@dmesser
Copy link
Contributor Author

dmesser commented Jun 16, 2025

@jonathankingfc I tested it without an override and with an override containing an existing storage class and also with a non-existing one. I also tested that the error when trying to change the storage class name after the fact it correctly surfaces the error message about trying to change on immutable field. Those would be good tests scenarios.

I will look into the e2e kuttle tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants