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

[BUG] Datastore::GetEngine returns raw cached object which is then modified #2827

Closed
joshimoo opened this issue Jul 29, 2021 · 3 comments
Closed

[BUG] Datastore::GetEngine returns raw cached object which is then modified #2827

joshimoo opened this issue Jul 29, 2021 · 3 comments
Assignees
@joshimoo @khushboo-rancher
Labels
backport/1.1.3 Require to backport to 1.1.3 release branch component/longhorn-manager Longhorn manager (control plane) kind/bug priority/1 Highly recommended to implement or fix in this release (managed by PO)
Milestone
v1.2.0

Comments

@joshimoo
Copy link
Contributor

Describe the bug
The returned engine object is then modified during the sync as well as the monitor
refresh cycles. This in itself is really bad but even worse since the sync
and monitoring routine run concurrently.

Expected behavior
Datastore::GetResource functions should always return deep copy objects that are safe to modify.

Environment:

  • Longhorn version: 1.2
@joshimoo joshimoo added kind/bug component/longhorn-manager Longhorn manager (control plane) backport/1.1.3 Require to backport to 1.1.3 release branch labels Jul 29, 2021
@joshimoo joshimoo added this to the v1.2.0 milestone Jul 29, 2021
@joshimoo joshimoo self-assigned this Jul 29, 2021
@joshimoo joshimoo mentioned this issue Jul 29, 2021
Merged
@longhorn-io-github-bot
Copy link

longhorn-io-github-bot commented Jul 29, 2021 •
edited by joshimoo
Loading

Pre Ready-For-Testing Checklist

  • Where is the reproduce steps/test steps documented?
    The reproduce steps/test steps are at: used all the time inside of the controller

  • Is there a workaround for the issue? If so, where is it documented?
    The workaround is at:

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

  • Does the PR include deployment change (YAML/Chart)? If so, where are the PRs for both YAML file and Chart?
    The PR for the YAML change is at:
    The PR for the chart change is at:

  • Have the backend code been merged (Manager, Engine, Instance Manager, BackupStore etc) (including backport-needed/*)?
    The PR is at Fix Datastore::GetEngine returning raw cached object longhorn-manager#966
    Backport 2827 - fix ds get engine call returning raw cached object longhorn-manager#972

  • Which areas/issues this PR might have potential impacts on?
    Area controller, engine/volume/replica
    Issues possibly related [BUG] the size missmatch in engine CR #2725

  • If labeled: require/LEP Has the Longhorn Enhancement Proposal PR submitted?
    The LEP PR is at

  • If labeled: area/ui Has the UI issue filed or ready to be merged (including backport-needed/*)?
    The UI issue/PR is at

  • If labeled: require/doc Has the necessary document PR submitted or merged (including backport-needed/*)?
    The documentation issue/PR is at

  • If labeled: require/automation-e2e Has the end-to-end test plan been merged? Have QAs agreed on the automation test case? If only test case skeleton w/o implementation, have you created an implementation issue (including backport-needed/*)
    The automation skeleton PR is at
    The automation test case PR is at
    The issue of automation test case implementation is at (please create by the template)

  • If labeled: require/automation-engine Has the engine integration test been merged (including backport-needed/*)?
    The engine automation PR is at

  • If labeled: require/manual-test-plan Has the manual test plan been documented?
    The updated manual test plan is at

  • If the fix introduces the code for backward compatibility Has a separate issue been filed with the label release/obsolete-compatibility?
    The compatibility issue is filed at

@joshimoo joshimoo mentioned this issue Jul 31, 2021
Merged
@joshimoo
Copy link
Contributor Author

joshimoo commented Aug 2, 2021 •
edited
Loading

Internal behavior, cannot really be tested.
Should be covered by the e2e as well as regression testing as part of the v1.2 release.

@innobead innobead added the priority/1 Highly recommended to implement or fix in this release (managed by PO) label Aug 2, 2021
@khushboo-rancher khushboo-rancher self-assigned this Aug 2, 2021
@khushboo-rancher
Copy link
Contributor

Closing this as the integration tests with branch v1.1.3 and master look okay. The failures are tested on local system, they seemed fine. The failure tests are not due to product failure and are being investigated.

@innobead innobead added backport-needed/1.1.x and removed backport/1.1.3 Require to backport to 1.1.3 release branch labels Oct 11, 2021
@innobead innobead added the backport/1.1.3 Require to backport to 1.1.3 release branch label Dec 10, 2021
@github-project-automation github-project-automation bot moved this to New Issues in Longhorn Sprint Aug 4, 2024
@derekbit derekbit moved this from New Issues to Closed in Longhorn Sprint Aug 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@joshimoo joshimoo

@khushboo-rancher khushboo-rancher

Labels
backport/1.1.3 Require to backport to 1.1.3 release branch component/longhorn-manager Longhorn manager (control plane) kind/bug priority/1 Highly recommended to implement or fix in this release (managed by PO)
Projects
Longhorn Sprint
Status: Closed
Milestone
v1.2.0
Development

No branches or pull requests
4 participants
@innobead @joshimoo @khushboo-rancher @longhorn-io-github-bot