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

Fix Tenant PodInformer for Tenant Status and remove recurrent job #2150

Merged
merged 4 commits into from
Jun 7, 2024

Conversation

cesnietor
Copy link
Contributor

@cesnietor cesnietor commented Jun 6, 2024

Use PodInformer to Update Tenant status.
Previous code had both an informer and a recurrent job (every 3 min) but it wasn't working great so this changes so that it updates the tenant status every time there's a change on the pods.
Before informer was reacting on all pods and then filtering them when handling the event, now it only reacts on pods that match the label selector.

Changes:

  • Use custom informer so that it can react only on pods that contain the Tenant labels
  • Requeues event if tenant is not in green status
  • Remove recurrent health job
  • Updates al the places where we had v1.min.io/tenant with miniov2.TenantLabel

Test Steps

  • Add a Tenant
  • Do some changes to the Pod
  • Delete the pod
  • Tenant Status should be updated on both changes (use node cordon if needed)

@cesnietor cesnietor self-assigned this Jun 6, 2024
ramondeklein
ramondeklein previously approved these changes Jun 6, 2024
Copy link
Contributor

@ramondeklein ramondeklein left a comment

Choose a reason for hiding this comment

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

Great improvement too to remove the "v1.min.io/tenant" literals. Only a single remark about a function that implies to do more than it can and inlining it may be a better alternative.

pkg/utils/utils.go Outdated Show resolved Hide resolved
pjuarezd
pjuarezd previously approved these changes Jun 6, 2024
ramondeklein
ramondeklein previously approved these changes Jun 6, 2024
@pjuarezd
Copy link
Member

pjuarezd commented Jun 6, 2024

this might need some more work, shared findings with @cesnietor wiating for his opinion before merge

Signed-off-by: pjuarezd <pjuarezd@users.noreply.github.com>
@pjuarezd pjuarezd dismissed stale reviews from ramondeklein and themself via 3281e38 June 6, 2024 21:17
pjuarezd
pjuarezd previously approved these changes Jun 6, 2024
@cesnietor cesnietor requested a review from ramondeklein June 6, 2024 21:48
@pjuarezd
Copy link
Member

pjuarezd commented Jun 6, 2024

this might need some more work, shared findings with @cesnietor wiating for his opinion before merge

All good now, tested and doubled tested

@cesnietor
Copy link
Contributor Author

this might need some more work, shared findings with @cesnietor wiating for his opinion before merge

for documenting purposes:
Pedro noticed that pod changes was not enough to trigger an update and get the latest status cause the service might take some time to be ready (green) and that wouldn't trigger another health check so we now enqueue again if the service is not ready. The queue already has a backoff so that takes care of the load.

@cniackz cniackz merged commit c6737b7 into minio:master Jun 7, 2024
30 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.

4 participants