Skip to content

Conversation

@jmckulk
Copy link
Collaborator

@jmckulk jmckulk commented Sep 5, 2025

Checklist:

  • Have you added an explanation of what your changes do and why you'd like them to be included?
  • Have you updated or added documentation for the change, as applicable?
  • Have you tested your changes on all related environments with successful results, as applicable?
    • Have you added automated tests?

Type of Changes:

  • New feature
  • Bug fix
  • Documentation
  • Testing enhancement
  • Other

What is the current behavior (link to any open issues here)?

What is the new behavior (if this is a feature change)?

  • Breaking change (fix or feature that would cause existing functionality to change)

Other Information:

@jmckulk jmckulk force-pushed the jmckulk/autogrow-tuning branch from 9e2962e to ccf0e06 Compare September 5, 2025 14:52
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should these fields be added to the v1 api?

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 I think so? I am curious as to why v1beta1 has this change

	if s.DataVolumeClaimSpec.AutoGrow != nil {
		s.DataVolumeClaimSpec.AutoGrow.Default()
	}

but v1 doesn't?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh its probably a missed change. I'm a little unsure when this is necessary 🤔

Name: naming.ContainerPGBackRestConfig,
Command: reloadCommand(
naming.ContainerPGBackRestConfig,
cluster.Spec.Backups.PGBackRest.Repos),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We use a similar reloadCommand for both instance and pgbackrest reloader sidecars. This change introduces new parameters to both functions but they unfortunately don't match. The instance accepts two VolumeClaimSpecWithAutoGrow but the pgbackrest pod accepts a list of pgbackrest repos.

Ideally we would pass the same types to each function because they are doing the same things 🤔

Open to thoughts here too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Open to other thoughts about where this function should be 💭

@jmckulk jmckulk force-pushed the jmckulk/autogrow-tuning branch 2 times, most recently from b15053f to ac13b9a Compare September 5, 2025 18:30
@jmckulk jmckulk marked this pull request as ready for review September 5, 2025 18:36
@jmckulk jmckulk force-pushed the jmckulk/autogrow-tuning branch 2 times, most recently from e82c0b6 to dc0c5eb Compare September 5, 2025 20:04
- Add new fields for Autogrow that allow configuring usage trigger and
  maxGrow size.
- Update pgdata, pgwal, and repo volume fields to use new types
- Update tests to account for new fields
- Update Sidecar bash logic to use new autogrow fields
@jmckulk jmckulk force-pushed the jmckulk/autogrow-tuning branch from dc0c5eb to 19d9042 Compare September 5, 2025 20:36
MaxGrow *resource.Quantity `json:"maxGrow,omitempty"`
}

func (spec *AutoGrowSpec) Default() {
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 Do we need this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll make a note to check on this later.

@jmckulk jmckulk merged commit 1d28067 into CrunchyData:main Sep 5, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants