Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: add ephemeralVolumeSource option to cluster spec. #3678

Merged
merged 8 commits into from Jan 23, 2024
Merged

feat: add ephemeralVolumeSource option to cluster spec. #3678

merged 8 commits into from Jan 23, 2024

Conversation

jfyne
Copy link
Contributor

@jfyne jfyne commented Jan 9, 2024 •
edited by mnencia

Allow the user to optionally set the ephemeralStorage option in the cluster spec.

Closes #3677

Refs:

@github-actions github-actions bot added backport-requested ◀️ This pull request should be backported to all supported releases release-1.20 release-1.21 release-1.22 labels Jan 9, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 9, 2024

❗ By default, the pull request is configured to backport to all release branches.

  • To stop backporting this pr, remove the label: backport-requested ◀️ or add the label 'do not backport'
  • To stop backporting this pr to a certain release branch, remove the specific branch label: release-x.y

@jfyne jfyne force-pushed the dev/3677 branch 2 times, most recently from 3e8ceeb to 48584e9 Compare January 10, 2024 08:43
@jfyne jfyne changed the title feat: add ephemeralStorage option to cluster spec. feat: add ephemeralVolumeSource option to cluster spec. Jan 10, 2024
@cloudnative-pg cloudnative-pg deleted a comment from github-actions bot Jan 12, 2024
@github-actions GitHub Actions
Copy link
Contributor

@armru, here's the link to the E2E on CNPG workflow run: https://github.com/cloudnative-pg/cloudnative-pg/actions/runs/7504582916

@armru
Copy link
Member

armru commented Jan 12, 2024

Hello, I've made several minor adjustments, the most notable one is to avoid increasing the CRD size.

whats your opinion on the patch @mnencia @leonardoce @gbartolini ?

@armru
Copy link
Member

armru commented Jan 13, 2024

/test limit=local

@github-actions GitHub Actions
Copy link
Contributor

@armru, here's the link to the E2E on CNPG workflow run: https://github.com/cloudnative-pg/cloudnative-pg/actions/runs/7512869603

@gbartolini
Copy link
Contributor

I like this patch. However, this conflicts with #2830.

My suggestion is to:

  • clearly document this behaviour and make it the preferred way to configure ephemeral storage
  • highlight the relationship with EphemeralVolumesSizeLimit (which relies on emptydir)
  • if needed, add validation that excludes one or the other

If the ephemeral PVC is defined, this has higher precedence than the existing behavior. The EphemeralVolumesSizeLimit option is evaluated only if the PVC is not specified or is removed during the standard operations of the cluster.

@jfyne
Copy link
Contributor Author

jfyne commented Jan 15, 2024

@armru would you like me to make the requested changes?

@armru
Copy link
Member

armru commented Jan 15, 2024

Yes, thanks very much

@jfyne
Copy link
Contributor Author

jfyne commented Jan 15, 2024

Looks like there was no code change required, just a documentation update. I have done that now

@armru
Copy link
Member

armru commented Jan 15, 2024

I think we should also add a validation webhook to prevent both EphemeralVolumesSizeLimit and EphemeralVolumeSource from being defined simultaneously.

@jfyne
Copy link
Contributor Author

jfyne commented Jan 16, 2024

@armru I've had a go, I haven't written an operator before. Is what I have done enough, and also the correct error?

@armru armru force-pushed the dev/3677 branch 2 times, most recently from c165cec to 6565c68 Compare January 17, 2024 10:31
@armru
Copy link
Member

armru commented Jan 17, 2024 •
edited

Hello @jfyne, could you add unit tests for the webhook validation?
Also I've made some small changes, feel free to give me your feedback :)

@jfyne
Copy link
Contributor Author

jfyne commented Jan 17, 2024 •
edited

I've added the validation unit tests. Changes look good to me 👍

@mnencia
Copy link
Member

mnencia commented Jan 18, 2024

/ok-to-merge E2E tests are green

@cnpg-bot cnpg-bot added the ok to merge 👌 This PR can be merged label Jan 18, 2024
@mnencia
Copy link
Member

mnencia commented Jan 18, 2024

Unit tests are failing

jfyne and others added 8 commits January 23, 2024 09:35
@jfyne
feat: add ephemeralVolumeSource option to cluster spec.
40144a3 
Allow the user to optionally set the ephemeralVolumeSource option in the
cluster spec.

Refs:
 - cloudnative-pg#3677
 - cloudnative-pg#317

Signed-off-by: Josh Fyne <josh.fyne@gmail.com>
@armru @jfyne
chore: lint
b2f4a85 
Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
@jfyne
chore(docs): Documentation to describe ephemeralVolumeSource.
981a431 
Indicates that ephemeralVolumeSource is the preferred way to configure
ephemeral volumes.

Describes how ephemeralVolumeSource and ephemeralVolumesSourceLimit
interact with each other. ephemeralVolumeSource taking precedence.

Signed-off-by: Josh Fyne <josh.fyne@gmail.com>
@jfyne
feat: Cluster ephemeral volume configuration validation.
497b7cb 
Signed-off-by: Josh Fyne <josh.fyne@gmail.com>
@armru @jfyne
chore: review, adds unit tests
c7e8d9a 
Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
@jfyne
chore: Ephemeral volume configuraion validation tests.
5de0990 
Signed-off-by: Josh Fyne <josh.fyne@gmail.com>
@armru @jfyne
chore: ensure that shm can be set
d4eb279 
Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
@armru @jfyne
chore: lint
b32f3c8 
Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
@mnencia mnencia merged commit 0e7ac95 into cloudnative-pg:main Jan 23, 2024
25 checks passed
cnpg-bot pushed a commit that referenced this pull request Jan 23, 2024
@jfyne
feat: add ephemeralVolumeSource option to cluster spec. (#3678)
d45b3c1 
Allow the user to optionally set the `ephemeralStorage` option in the
cluster spec.

Closes #3677

Refs:
 - #3677
 - #317

Signed-off-by: Josh Fyne <josh.fyne@gmail.com>
Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Co-authored-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
(cherry picked from commit 0e7ac95)
@github-actions github-actions bot mentioned this pull request Jan 23, 2024
Closed
mnencia pushed a commit that referenced this pull request Jan 23, 2024
@jfyne @mnencia
feat: add ephemeralVolumeSource option to cluster spec. (#3678)
feca490 
Allow the user to optionally set the `ephemeralStorage` option in the
cluster spec.

Closes #3677

Refs:
 - #3677
 - #317

Signed-off-by: Josh Fyne <josh.fyne@gmail.com>
Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Co-authored-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
(cherry picked from commit 0e7ac95)
mnencia pushed a commit that referenced this pull request Jan 23, 2024
@jfyne @mnencia
feat: add ephemeralVolumeSource option to cluster spec. (#3678)
ff0847b 
Allow the user to optionally set the `ephemeralStorage` option in the
cluster spec.

Closes #3677

Refs:
 - #3677
 - #317

Signed-off-by: Josh Fyne <josh.fyne@gmail.com>
Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Co-authored-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
(cherry picked from commit 0e7ac95)
@jfyne jfyne deleted the dev/3677 branch January 24, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnencia mnencia mnencia approved these changes

@armru armru armru approved these changes

@fcanovai fcanovai Awaiting requested review from fcanovai

@gbartolini gbartolini Awaiting requested review from gbartolini

@leonardoce leonardoce Awaiting requested review from leonardoce

@phisco phisco Awaiting requested review from phisco

@sxd sxd Awaiting requested review from sxd

Assignees

@mnencia mnencia

@leonardoce leonardoce

@armru armru

Labels
backport-requested ◀️ This pull request should be backported to all supported releases ok to merge 👌 This PR can be merged release-1.20 release-1.21 release-1.22
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature]: Allow configuration of EphemeralVolumeSource.
6 participants
@jfyne @armru @gbartolini @mnencia @leonardoce @cnpg-bot