Fix engine migration crash #2275
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ejweber
force-pushed
the
6961-fix-migration-engine-crash
branch
from
be6aca6
to
c471422
Compare
|
With this PR, the following modifications in steps 12 and 13 from longhorn/longhorn#6961 (comment) are observed: When the manually created volume attachment is deleted ( The good:
The bad:
|
ejweber
force-pushed
the
6961-fix-migration-engine-crash
branch
from
c471422
to
5f98944
Compare
Longhorn 6961 Signed-off-by: Eric Weber <eric.weber@suse.com>
Longhorn 6961 Signed-off-by: Eric Weber <eric.weber@suse.com>
ejweber
force-pushed
the
6961-fix-migration-engine-crash
branch
from
5f98944
to
ffc8a93
Compare
|
I also backported and tested in Harvester. I was not able to reproduce I/O errors with the fix (though the stuck migration still persists). |
|
Passed end-to-end tests: https://ci.longhorn.io/job/private/job/longhorn-tests-regression/5195/. |
…erpart Longhorn 6961 Signed-off-by: Eric Weber <eric.weber@suse.com>
ejweber
force-pushed
the
6961-fix-migration-engine-crash
branch
from
ffc8a93
to
5326131
Compare
innobead
approved these changes
Nov 12, 2023
| @@ -303,11 +303,18 @@ func (rc *ReplicaController) syncReplica(key string) (err error) { | |||
| return errors.Wrapf(err, "failed to cleanup the related replica instance before deleting replica %v", replica.Name) | |||
| } | |||
|
|
|||
| rs, err := rc.ds.ListReplicasByNodeRO(replica.Spec.NodeID) | |||
There was a problem hiding this comment.
nit: rs can be initialized later in the following else if replica.Spec.NodeID != "" condition.
|
@mergify backport v1.5.x v1.4.x |
✅ Backports have been created
|
This was referenced Nov 12, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
longhorn/longhorn#6961
Just be6aca6 is sufficient to prevent the engine from crashing under the circumstances from longhorn/longhorn#6961 (comment). However, this commit alone leads to orphaned data once the migration is complete (as the replica we refuse to add to the engine becomes inactive without an active counterpart).
I experimented with a path to making use of this newly inactive replica after the migration (to avoid unnecessary rebuilding), but I ultimately decided that the migration logic refactor this required was outside the scope of the ticket.
The other commits are an attempt to ensure we don't orphan the newly inactive replica's directory by providing a path to cleaning it up.
I need more time to test this because I'm running into issues with backing image download checksums. For some reason, the download of the backing image to the uncordoned node is consistently failing with checksum issues.