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.

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

feat(chart): prevent CRDs from being recycled #458

Conversation

cmontemuino
Copy link
Contributor

@cmontemuino cmontemuino commented Mar 26, 2024

Description

With current implementation a chart uninstall also removes the CRDs, which triggers deletion of all created CRs.

Motivation and Context

Add a configuration to keep CRDs by default, instructing Helm to not delete such resources. Also add the possibility to specify extra annotations. This is especially interesting for Argo CD users, as Argo CD does not install charts the Helm way. In such a case, an extra annotation to prevent prunning resources is required.

Regression

How Has This Been Tested?

Case 1: keep all CRDs

helm chart template

Case 2: keep CSI VolumeSnaphots CRDs only

helm chart template --set crds.jaeger.keep=false

Case 3: keep Jaeger CRDs only

helm chart template --set crds.csi.volumeSnapshots.keep=false

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added unit tests to cover my changes.

Problem: with current implementation a chart uninstal also removes the
CRDs, which triggers deletion of all created CRs.

Solution: add a configuration to keep CRDs by default, instructing Helm
to not delete such resources. Also add the possibility to specify extra
annotations. This is especially interesting for Argo CD users, as Argo
CD does not install charts the Helm way. In such a case, an extra
annotation to prevent prunning resources is required.

Signed-off-by: cmontemuino <1761056+cmontemuino@users.noreply.github.com>
Problem: the crdIsAbsent check is preventing to upgrade CRDs

Solution: remove the check and rely on `crds.xxx.enabled` property
instead. This should be a conscious decicion made by users anyways.

Signed-off-by: cmontemuino <1761056+cmontemuino@users.noreply.github.com>
@tiagolobocastro
Copy link
Contributor

thanks @cmontemuino!
bors merge

bors-openebs-mayastor bot pushed a commit that referenced this pull request Mar 27, 2024
458: feat(chart): prevent CRDs from being recycled r=tiagolobocastro a=cmontemuino

<!

Co-authored-by: cmontemuino <1761056+cmontemuino@users.noreply.github.com>
@bors-openebs-mayastor
Copy link
Contributor

Build failed:

@tiagolobocastro
Copy link
Contributor

@cmontemuino could you please re-run the helm doc generator, here's the failure from CI:

 ## Values

 

-| Key | Type | Default | Description |

-|-----|------|---------|-------------|

-| csi.volumeSnapshots.annotations | object | `{}` | Annotations to be added to all CRDs |

-| csi.volumeSnapshots.enabled | bool | `true` | Install Volume Snapshot CRDs |

-| csi.volumeSnapshots.keep | bool | `true` | Keep CRDs on chart uninstall |

-| jaeger.annotations | object | `{}` | Annotations to be added to all CRDs |

-| jaeger.enabled | bool | `true` | Install Jaeger CRDs |

-| jaeger.keep | bool | `true` | Keep CRDs on chart uninstall |

+| Key | Description | Default |

+|:----|:------------|:--------|

+| csi.&ZeroWidthSpace;volumeSnapshots.&ZeroWidthSpace;annotations | Annotations to be added to all CRDs | <pre>{<br><br>}</pre> |

+| csi.&ZeroWidthSpace;volumeSnapshots.&ZeroWidthSpace;enabled | Install Volume Snapshot CRDs | `true` |

+| csi.&ZeroWidthSpace;volumeSnapshots.&ZeroWidthSpace;keep | Keep CRDs on chart uninstall | `true` |

+| jaeger.&ZeroWidthSpace;annotations | Annotations to be added to all CRDs | <pre>{<br><br>}</pre> |

+| jaeger.&ZeroWidthSpace;enabled | Install Jaeger CRDs | `true` |

+| jaeger.&ZeroWidthSpace;keep | Keep CRDs on chart uninstall | `true` |

Signed-off-by: cmontemuino <1761056+cmontemuino@users.noreply.github.com>
@Abhinandan-Purkait
Copy link
Member

bors try

bors-openebs-mayastor bot pushed a commit that referenced this pull request Mar 28, 2024
@bors-openebs-mayastor
Copy link
Contributor

try

Build succeeded:

@Abhinandan-Purkait
Copy link
Member

Thanks @cmontemuino. Are we good to merge now?

@Abhinandan-Purkait
Copy link
Member

bors merge

@bors-openebs-mayastor
Copy link
Contributor

Build succeeded:

@bors-openebs-mayastor bors-openebs-mayastor bot merged commit 04f727f into openebs:develop Mar 28, 2024
5 checks passed
@cmontemuino cmontemuino deleted the prevent-crds-from-being-recycled branch March 28, 2024 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants