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

ROX-15806: Remove pause reconcile annotation before Central deletion #905

Merged
merged 2 commits into from
Mar 24, 2023

Conversation

vladbologa
Copy link
Contributor

@vladbologa vladbologa commented Mar 23, 2023

Description

If a Central has the pause-reconcile annotation set, and its deletion is requested, it currently remains stuck in a deprovisioning state. The Central itself is not deleted by the Operator, however fleetshard-sync deletes all its other resources, making it unusable.

This PR takes the approach of setting the pause-reconcile annotation to false (if it's present). This way the Central is deleted, and the UI is not in an inconsistent state.

An alternative would be to have also fleetshard-sync not reconcile a Central that has the annotation set.

Checklist (Definition of Done)

  • Unit and integration tests added
  • Added test description under Test manual
  • Documentation added if necessary (i.e. changes to dev setup, test execution, ...)
  • CI and all relevant tests are passing
  • Add the ticket number to the PR title if available, i.e. ROX-12345: ...
  • Discussed security and business related topics privately. Will move any security and business related topics that arise to private communication channel.

Test manual

Tested locally.

@vladbologa vladbologa temporarily deployed to development March 23, 2023 15:55 — with GitHub Actions Inactive
@vladbologa vladbologa requested a review from kovayur March 23, 2023 16:39
Copy link
Contributor

@kovayur kovayur left a comment

Choose a reason for hiding this comment

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

Looks good, only one small improvement


if central.Annotations != nil {
// avoid being stuck in a deprovisioning state due to the pause reconcile annotation
if _, exists := central.Annotations[pauseReconcileAnnotation]; exists {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe check that it's set to true and not just exist? This will help avoid unnecessary updates in the reconciliation loop before Central is actually deleted.

@kovayur
Copy link
Contributor

kovayur commented Mar 23, 2023

Unit test?

@vladbologa
Copy link
Contributor Author

vladbologa commented Mar 24, 2023

Unit test?

I'm trying to write a test, but it's proving hard to write a meaningful test with the fake client.

ensureCentralCRDeleted simply deletes the Central CR in the test environment, so I can't check afterwards if it set the annotation to false.

Any ideas? I could go for a e2e test for example. I can also extract the code that changes the annotation into a separate function, and only unit test that function.

@openshift-ci openshift-ci bot removed the lgtm label Mar 24, 2023
@vladbologa vladbologa temporarily deployed to development March 24, 2023 13:37 — with GitHub Actions Inactive
Copy link
Contributor

@kovayur kovayur left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci openshift-ci bot added the lgtm label Mar 24, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 24, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kovayur, vladbologa

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@vladbologa vladbologa merged commit 929095c into main Mar 24, 2023
@vladbologa vladbologa deleted the vbologa/ROX-15806-remove-pause-reconcile-on-delete branch March 24, 2023 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants