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

Force umount for NFS mount (like with longhorn) #8521

Merged
merged 1 commit into from Nov 15, 2023

Conversation

smutel
Copy link
Contributor

@smutel smutel commented Oct 3, 2023

Proposed Changes

When using k3s with storage system like longhorn, the uninstall script hangs when trying to unmount filesystems managed by NFS.

Types of Changes

Bugfix

Verification

  • Install k3s with longhorn
  • Create a volume
  • Uninstall it

Testing

Not sure that the install.sh is covered by tests right now.

Linked Issues

User-Facing Change

@smutel smutel requested a review from a team as a code owner October 3, 2023 07:31
Copy link
Contributor

@brandond brandond left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! If you change the install script, you need to update the install script checksum file as well.

@smutel smutel force-pushed the ImproveUninstall branch 2 times, most recently from 7fbf764 to 39d5356 Compare October 17, 2023 16:23
@smutel
Copy link
Contributor Author

smutel commented Oct 17, 2023

Done.

@smutel smutel force-pushed the ImproveUninstall branch 3 times, most recently from 2d974ae to ef6fa05 Compare October 20, 2023 08:11
@dereknola
Copy link
Contributor

dereknola commented Oct 25, 2023

@smutel Thanks for the PR. To follow CNCF Guidelines we require that all commits be signed. See https://github.com/k3s-io/k3s/pull/8521/checks?check_run_id=17890940938 for instructions on recommit your changes with a signed commit.

Once DCO passes, we can get this PR in. We are currently in code freeze, so it won't be till likely next week that this gets merged.

Also don't worry about s390x failures in CI, its a known bug right now.

@codecov
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bbafb86) 47.12% compared to head (c643f0a) 49.36%.

❗ Current head c643f0a differs from pull request most recent head 82eb681. Consider uploading reports for the commit 82eb681 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8521      +/-   ##
==========================================
+ Coverage   47.12%   49.36%   +2.23%     
==========================================
  Files         148      148              
  Lines       15674    15674              
==========================================
+ Hits         7387     7737     +350     
+ Misses       7089     6698     -391     
- Partials     1198     1239      +41     
Flag Coverage Δ
e2etests 46.88% <ø> (?)
inttests 41.09% <ø> (-3.52%) ⬇️
unittests 17.87% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 44 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Samuel Mutel <12967891+smutel@users.noreply.github.com>
@smutel
Copy link
Contributor Author

smutel commented Nov 8, 2023

Rebase done. Could we merge this PR ? I already rebase this PR 3 times due to sha256 on install.sh.

@rds13
Copy link

rds13 commented Nov 14, 2023

👍

@brandond
Copy link
Contributor

s390x flaked; merging

@brandond brandond merged commit 19fd7e3 into k3s-io:master Nov 15, 2023
13 of 14 checks passed
@DanielJuravski
Copy link

Is there a reason why this commit wasn't cherry-picked to 1.26?

@caroline-suse-rancher
Copy link
Contributor

@DanielJuravski We don't backport changes to the install script, as it's unnecessary. See that noted in the associated 1.26 issue - #8856

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.

None yet

6 participants