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

Add retries when removing device mapper target #1200

Merged
merged 7 commits into from
Nov 3, 2021

Conversation

anmaxvl
Copy link
Contributor

@anmaxvl anmaxvl commented Oct 19, 2021

Additionally ignore the error from device mapper if target removal
still fails. The corresponding layer path is already unmounted
at that point and this avoids having an inconsistent state.

Signed-off-by: Maksim An maksiman@microsoft.com

@anmaxvl anmaxvl requested a review from a team as a code owner October 19, 2021 01:31
Comment on lines 283 to 294

// This is workaround for "device or resource busy" error, which occasionally happens after the device mapper
// target has been unmounted.
for i := 0; i < 10; i++ {
if err = rm(); err != nil {
time.Sleep(10 * time.Millisecond)
continue
}
break
}
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we still be checking to see if the context is cancelled even though we have a limited number of tries here? I'm not sure what makes sense to do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I doubt we'll ever hit a context cancellation/timeout since the combined retry time is relatively short (~100ms) compared to the default context timeout. Should we even consider an outside context within this function? I'd think not?

Additionally ignore the error from device mapper if target removal
still fails. The corresponding layer path is already unmounted
at that point and this avoids having an inconsistent state.

Signed-off-by: Maksim An <maksiman@microsoft.com>
Signed-off-by: Maksim An <maksiman@microsoft.com>
Signed-off-by: Maksim An <maksiman@microsoft.com>
Signed-off-by: Maksim An <maksiman@microsoft.com>
@anmaxvl
Copy link
Contributor Author

anmaxvl commented Nov 1, 2021

@msscotb added a unit test. @dcantah, @katiewasnothere PTAL as well

Signed-off-by: Maksim An <maksiman@microsoft.com>
Signed-off-by: Maksim An <maksiman@microsoft.com>
Copy link
Contributor

@dcantah dcantah left a comment

Choose a reason for hiding this comment

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

lgtm again

Copy link
Contributor

@msscotb msscotb left a comment

Choose a reason for hiding this comment

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

lgtm

@dcantah
Copy link
Contributor

dcantah commented Nov 2, 2021

Actually, re-reading what error is returned from ioctl() (the dmError one) I don't think we can just compare the error to syscall.EBUSY. You'd have to cast to dmError and if it succeeds then you can check err.Err == syscall.EBUSY I believe.

Signed-off-by: Maksim An <maksiman@microsoft.com>
Copy link
Contributor

@dcantah dcantah left a comment

Choose a reason for hiding this comment

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

ok last approval I swear 😂

@anmaxvl
Copy link
Contributor Author

anmaxvl commented Nov 2, 2021

ok last approval I swear 😂

cmon, it's a great presence of mind you got going there! 😄

Copy link
Contributor

@msscotb msscotb left a comment

Choose a reason for hiding this comment

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

lgtm again

@anmaxvl anmaxvl merged commit b8917e1 into microsoft:master Nov 3, 2021
@anmaxvl anmaxvl deleted the device-mapper-retries branch November 3, 2021 04:51
helsaawy pushed a commit to helsaawy/hcsshim that referenced this pull request Nov 3, 2021
Add retries when removing device mapper target

Additionally ignore the error from device mapper if target removal
still fails. The corresponding layer path is already unmounted
at that point and this avoids having an inconsistent state.

Signed-off-by: Maksim An <maksiman@microsoft.com>
princepereira pushed a commit to princepereira/hcsshim that referenced this pull request Aug 29, 2024
Add retries when removing device mapper target

Additionally ignore the error from device mapper if target removal
still fails. The corresponding layer path is already unmounted
at that point and this avoids having an inconsistent state.

Signed-off-by: Maksim An <maksiman@microsoft.com>
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.

4 participants