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(csi): Invalid mount point with volume detached #2043

Merged
merged 1 commit into from
Jul 6, 2023

Conversation

mantissahz
Copy link
Contributor

@mantissahz mantissahz commented Jul 3, 2023

Volume detached because only one replica failed and not remount the globalmount. We need to handle it at NodePublishVolume because os.ReadDir will be passed when it becomes a read-only file system after an unexpected volume detaching.

Ref: longhorn/longhorn#4814, longhorn/longhorn#5801

Test case test:
https://ci.longhorn.io/job/private/job/longhorn-tests-regression/4403/
https://ci.longhorn.io/job/private/job/longhorn-tests-regression/4425/

@mantissahz mantissahz requested a review from innobead July 3, 2023 02:40
@mantissahz mantissahz self-assigned this Jul 3, 2023
@mantissahz mantissahz requested a review from a team as a code owner July 3, 2023 02:40
shuo-wu
shuo-wu previously approved these changes Jul 3, 2023
Copy link
Contributor

@shuo-wu shuo-wu left a comment

Choose a reason for hiding this comment

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

Others LGTM

csi/util.go Show resolved Hide resolved
@mantissahz mantissahz requested a review from a team July 3, 2023 03:44
@mergify
Copy link

mergify bot commented Jul 3, 2023

This pull request is now in conflicts. Could you fix it @mantissahz? 🙏

csi/util.go Outdated Show resolved Hide resolved
csi/util.go Outdated Show resolved Hide resolved
shuo-wu
shuo-wu previously approved these changes Jul 3, 2023
Copy link
Contributor

@shuo-wu shuo-wu left a comment

Choose a reason for hiding this comment

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

LGTM. After addressing Derek's comments, we can merge it

derekbit
derekbit previously approved these changes Jul 3, 2023
Copy link
Member

@derekbit derekbit left a comment

Choose a reason for hiding this comment

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

LGTM

csi/util.go Show resolved Hide resolved
csi/node_server.go Outdated Show resolved Hide resolved
csi/util.go Outdated Show resolved Hide resolved
csi/util.go Outdated Show resolved Hide resolved
csi/util.go Outdated Show resolved Hide resolved
@innobead
Copy link
Member

innobead commented Jul 4, 2023

@mantissahz Please help check the remaining reviews from Chinya and Phan.

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.

In general, LGTM.

James, well done, but a few comments need you help to check.

csi/util.go Outdated Show resolved Hide resolved
csi/util.go Outdated Show resolved Hide resolved
csi/util.go Show resolved Hide resolved
@innobead
Copy link
Member

innobead commented Jul 5, 2023

@mergify backport v1.5.x

@mergify
Copy link

mergify bot commented Jul 5, 2023

backport v1.5.x

✅ Backports have been created

c3y1huang
c3y1huang previously approved these changes Jul 5, 2023
derekbit
derekbit previously approved these changes Jul 5, 2023
csi/util.go Outdated Show resolved Hide resolved
csi/util.go Outdated Show resolved Hide resolved
@mantissahz mantissahz dismissed stale reviews from derekbit and c3y1huang via aebe700 July 5, 2023 16:33
@mantissahz mantissahz force-pushed the data-locality-pod-remount branch 2 times, most recently from aebe700 to 96d6a89 Compare July 6, 2023 05:22
Volume detached because only one replica failed and not remount the
`globalmount`. We need to handle it at NodePublishVolume because
os.ReadDir will be passed when it becomes a read-only file system
after an unexpected volume detaching.

Use an empty file to test the file system

Ref: 4814, 5801

Signed-off-by: James Lu <james.lu@suse.com>
@innobead innobead merged commit a9d6289 into longhorn:master Jul 6, 2023
@innobead
Copy link
Member

innobead commented Jul 6, 2023

@mergify backport v1.5.x v1.4.x v1.3.x

@mergify
Copy link

mergify bot commented Jul 6, 2023

backport v1.5.x v1.4.x v1.3.x

✅ Backports have been created

@mantissahz
Copy link
Contributor Author

@mergify backport v1.4.x v1.3.x

@mergify
Copy link

mergify bot commented Jul 6, 2023

backport v1.4.x v1.3.x

✅ Backports have been created

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.

6 participants