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 strict-local volume restore stuck in attaching state #2049

Closed
wants to merge 1 commit into from

Conversation

derekbit
Copy link
Member

@derekbit derekbit commented Jul 6, 2023

For a volume being restored, the spec.nodeID is empyty but the status.ownerID is set. Thus, set r.Spec.HardNodeAffinity to volume.status.ownerID instead.

longhorn/longhorn#6239

Copy link
Contributor

@ejweber ejweber left a comment

Choose a reason for hiding this comment

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

LGTM

For a volume being restored, the spec.nodeID is empyty but the status.ownerID is set.
Thus, set r.Spec.HardNodeAffinity to volume.status.ownerID instead.

Longhorn 6239

Signed-off-by: Derek Su <derek.su@suse.com>
@derekbit derekbit requested review from innobead and a team July 10, 2023 05:54
@derekbit
Copy link
Member Author

@mergify backport v1.5.x

@mergify
Copy link

mergify bot commented Jul 10, 2023

backport v1.5.x

🟠 Waiting for conditions to match

  • merged [:pushpin: backport requirement]

@derekbit
Copy link
Member Author

@mergify backport v1.4.x

@mergify
Copy link

mergify bot commented Jul 10, 2023

backport v1.4.x

🟠 Waiting for conditions to match

  • merged [:pushpin: backport requirement]

if v.Spec.NodeID == "" {
continue
if v.Status.RestoreRequired {
r.Spec.HardNodeAffinity = v.Status.OwnerID
Copy link
Member

@innobead innobead Jul 10, 2023

Choose a reason for hiding this comment

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

@derekbit Can you explain where v.Status.OwnerID will be set when the volume is being restored? so it can be used here.

Copy link
Member Author

Choose a reason for hiding this comment

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

For a volume being restored, the volume.spec.nodeID and volume.status.ownerID are empty, and the later one is determined in https://github.com/longhorn/longhorn-manager/blob/master/controller/volume_controller.go#L295.

Copy link
Member

@innobead innobead left a comment

Choose a reason for hiding this comment

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

I think this has been fixed in the master and 1.5.x code base already as below. The node id will be set by vol.Status.OwnerID.
/controller/volume_restore_controller.go#L190-L194

	if vol.Status.RestoreRequired {
		createOrUpdateAttachmentTicket(va, restoringAttachmentTicketID, vol.Status.OwnerID, longhorn.TrueValue, longhorn.AttacherTypeVolumeRestoreController)
	} else {
		delete(va.Spec.AttachmentTickets, restoringAttachmentTicketID)
	}

The fix here is more for 1.4.x instead?

@derekbit
Copy link
Member Author

Ah, yes. You're right. I didn't notice the fix in v1.5.x and master. So, the fix targets v1.4.x.

@innobead
Copy link
Member

then, let's change the target branch is 1.4.x instead. 🙂

@derekbit
Copy link
Member Author

Target for v1.4.x only

@derekbit derekbit closed this Jul 10, 2023
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.

3 participants