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

[IMPROVEMENT] Prevent unnecessary updates of instanceManager status #8420

Closed
derekbit opened this issue Apr 23, 2024 · 2 comments
Closed

[IMPROVEMENT] Prevent unnecessary updates of instanceManager status #8420

derekbit opened this issue Apr 23, 2024 · 2 comments
Assignees
Labels
backport/1.5.6 backport/1.6.2 component/longhorn-manager Longhorn manager (control plane) kind/improvement Request for improvement of existing function require/backport Require backport. Only used when the specific versions to backport have not been definied. require/manual-test-plan Require adding/updating manual test cases if they can't be automated
Milestone

Comments

@derekbit
Copy link
Member

Is your improvement request related to a feature? Please describe (👍 if you like this request)

If the engine or replica process map from an instance manager is empty, instanceManager.status.InstanceEngines or instanceManager.status.InstanceReplicas will be a nil map.

reflect.DeepEqual function treats two maps var m1 map[string]process and m2 := map[string]process as different maps. Therefore, to prevent unnecessary updates, we must check both that the length of the maps is zero and that the maps are identical.

https://github.com/longhorn/longhorn-manager/blob/master/controller/instance_manager_controller.go#L1636-L1638

Describe the solution you'd like

Describe alternatives you've considered

Additional context

@derekbit derekbit added require/doc Require updating the longhorn.io documentation require/manual-test-plan Require adding/updating manual test cases if they can't be automated kind/improvement Request for improvement of existing function require/backport Require backport. Only used when the specific versions to backport have not been definied. labels Apr 23, 2024
@derekbit derekbit self-assigned this Apr 23, 2024
@derekbit derekbit added this to the v1.7.0 milestone Apr 23, 2024
@derekbit derekbit added component/longhorn-manager Longhorn manager (control plane) and removed require/doc Require updating the longhorn.io documentation labels Apr 23, 2024
@longhorn-io-github-bot
Copy link

longhorn-io-github-bot commented Apr 23, 2024

Pre Ready-For-Testing Checklist

  • Where is the reproduce steps/test steps documented?
    The reproduce steps/test steps are at:

Run e2e test to check the fix doesn't introduce any side effects.

  • Does the PR include the explanation for the fix or the feature?

#8420 (comment)

  • Have the backend code been merged (Manager, Engine, Instance Manager, BackupStore etc) (including backport-needed/*)?
    The PR is at

longhorn/longhorn-manager#2737

  • Which areas/issues this PR might have potential impacts on?
    Area: instance manager update
    Issues

@chriscchien
Copy link
Contributor

Verified pass on longhorn master(longhorn-manager 1cda0e)

On daily regression, did not see outsatnding issue related to instanceManager.

In build 946 , all failed test case have found the root cause and have related ticket

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.5.6 backport/1.6.2 component/longhorn-manager Longhorn manager (control plane) kind/improvement Request for improvement of existing function require/backport Require backport. Only used when the specific versions to backport have not been definied. require/manual-test-plan Require adding/updating manual test cases if they can't be automated
Projects
None yet
Development

No branches or pull requests

4 participants