Clean up fuse unmount workflow #18241

lucyge2022 · 2023-10-05T04:49:15Z

What changes are proposed in this pull request?

clean up unmount of fuse

Why are the changes needed?

alluxio-fuse may cause AlluxioFuse continues to hang, and also the current flow to unmount is scattered and all over the place.

Does this PR introduce any user facing changes?

no

alluxio-bot · 2023-10-05T18:15:51Z

Automated checks report:

PR title follows the conventions: FAIL
- The title of the PR does not pass all the checks. Please fix the following issues:
  - First word of title ("Fuse") is not an imperative verb. Please use one of the valid words
Commits associated with Github account: PASS

Some checks failed. Please fix the reported issues and reply 'alluxio-bot, check this please' to re-run checks.

alluxio-bot · 2023-10-05T19:54:27Z

Automated checks report:

PR title follows the conventions: PASS
Commits associated with Github account: PASS

All checks passed!

beinan

Java code looks good to me. Could you ping @ssz1997 to confirm these changes won't break the features on k8s side?

beinan · 2023-10-06T19:28:45Z

dora/integration/fuse/src/main/java/alluxio/fuse/AlluxioFuse.java

@@ -105,11 +105,10 @@ protected void startCommon(AlluxioConfiguration conf,
    try (FileSystem fs = createBaseFileSystem(fsContext, fuseOptions)) {
      AlluxioJniFuseFileSystem fuseFileSystem = createFuseFileSystem(fsContext, fs, fuseOptions);
      setupFuseFileSystem(fuseFileSystem);
-      launchFuse(fuseFileSystem, fsContext, fuseOptions, true); // This will block until umount
+      // This will block on serving until fuse gets destroyed
+      launchFuse(fuseFileSystem, fsContext, fuseOptions, true);
    } catch (Throwable t) {


Out of scope of this pr, but we should not swallow fatal error (such as jvm oom error)

yeah this is actually catching throwable at starting fuse with fuse_main_real, jvm oom throws from some other threads, but will get the exit(-1) back

beinan · 2023-10-06T19:30:21Z

dora/integration/jnifuse/fs/src/main/java/alluxio/jnifuse/AbstractFuseFileSystem.java

@@ -176,16 +179,23 @@ private void umountInternal() {
      }
    } else {
      try {
-        exitCode = new ProcessBuilder("fusermount", "-u", "-z", mountPath).start().waitFor();
+        Pair<Integer, String> output = Environment.runCommandAndCapture(


nit: I'm not a big fan to use Pair, it might be better to declare a class for the result explicitly

yeah this entire func is not used any more, these are some codes for debugging purposes. let me remove these changes.

beinan · 2023-10-06T19:31:38Z

dora/integration/jnifuse/fs/src/main/java/alluxio/jnifuse/utils/Environment.java

+        }
+      }
+    }
+    return Pair.of(exitCode, bout.toString());


I prefer to add a class for the result of commandAndCapture, I have seen quite a few places using a pair for this one. your call

sure will do

ssz1997

No break change on k8s

LuQQiu

LGTM, thanks for improving the logics to gracefully shutdown fuse!

LuQQiu · 2023-10-06T19:59:55Z

dora/integration/jnifuse/fs/src/main/java/alluxio/jnifuse/AbstractFuseFileSystem.java

+   Deprecating this as we shouldn't call the umount/fusermount ourselves, it should come from
+   user and stop the fuse_main_real from serving and then we exit from main thread
+   */
+  @Deprecated
  private void umountInternal() {


any place that we are still using this method or it's fully deprecated from this PR?

fully deprecated, shall i just get rid of it, no cmd of umount/fusermount should be run by us in java IMO

jiacheliu3

Thanks for the work! I added some comments on the java part. PTAL and hope that helps.

I don't know much about the C part of fuse so I'd leave that part of review to our experts :P

jiacheliu3 · 2023-10-07T02:51:24Z

dora/integration/fuse/src/main/java/alluxio/fuse/AlluxioFuse.java

@@ -264,8 +262,12 @@ public static AlluxioJniFuseFileSystem launchFuse(AlluxioJniFuseFileSystem fuseF

    String mountPoint = conf.getString(PropertyKey.FUSE_MOUNT_POINT);
    final boolean debugEnabled = conf.getBoolean(PropertyKey.FUSE_DEBUG_ENABLED);
+    LOG.info("Installing signal handler for SIGTERM and SIGINT to handle "


nit: wanna mention HUP as well?

done, good catch : )

jiacheliu3 · 2023-10-07T02:52:36Z

dora/integration/fuse/src/main/java/alluxio/fuse/FuseSignalHandler.java

+ * any existing signal handlers installed up front. Since jvm always install
+ * signal handlers therefore libfuse is never able to do so.
+ * But disabling JVM's signal handlers by flag -Xrs, fuse acts differently
+ * on receiving these signals on different platform(MacOs can receive/Linux can't)


nit

Suggested change

* on receiving these signals on different platform(MacOs can receive/Linux can't)

* on receiving these signals on different platform(MacOS can receive/Linux can't)

jiacheliu3 · 2023-10-07T02:53:03Z

dora/integration/fuse/src/main/java/alluxio/fuse/FuseSignalHandler.java

@@ -41,12 +51,12 @@ public void handle(Signal signal) {
    LOG.info("Receive signal name {}, number {}, system exiting",
        signal.getName(), signal.getNumber());
    int number = signal.getNumber();
-    if (number == 15) {
+    // SIGTERM - 15 SIGINT - 2 SIGHUP - 1


Suggested change

// SIGTERM - 15 SIGINT - 2 SIGHUP - 1

// SIGTERM - 15

// SIGINT - 2

// SIGHUP - 1

I think we should catch SIGHUP and then ignore it here (a.k.a empty handler)

oh i merely follow what fuse_set_signal_handlers does, fuse acts to sighup and stop serving as well, any reason to ignore SIGHUP?

jiacheliu3 · 2023-10-07T02:55:53Z

dora/integration/jnifuse/fs/src/main/java/alluxio/jnifuse/utils/Environment.java

@@ -124,4 +132,45 @@ public static String getJniLibraryExtension() {
    }
    return isMac() ? ".jnilib" : ".so";
  }
+
+
+  public static Pair<Integer, String> runCommandAndCapture(String... command)


I added a ShellCommand class long ago to do the same thing and redirect out/err, maybe use that?

ah i see i will use that

oh I can't without adding dependency on alluxio-core-common, i will just remove this as im anyway deprecating the function

jiacheliu3 · 2023-10-07T03:00:50Z

dora/integration/fuse/bin/alluxio-fuse

-    err "Failed to umount fuse mount point ${mount_point}"
+    err "Failed to umount fuse mount point ${mount_point}, try sending SIGTERM..."
+    kill "${fuse_pid}"
+    wait_for_fuse_process_killed "${fuse_pid}"


echo the return code from wait_for_fuse_process_killed before exiting

If kill -TERM still does not kill the process, we have to use kill -KILL here.

yeah if this does not work here, user will use alluxio-fuse -f unmount which automatically kill with 9
i'm modifying the logic of a graceful alluxio-fuse unmount shutdown, in any case user can always kill with 9, but the graceful shutdown shouldn't send 15 in the first place bcos the jvm signal handler thingy overwriting fuse set signal logics, that's what this PR aims to resolve

jiacheliu3

LGTM, thanks!

lucyge2022 · 2023-10-16T23:54:03Z

hey @huanghua78 i will merge this in ok?

Xenorith · 2023-10-31T18:34:31Z

@huanghua78 any comments about this PR?

huanghua78 · 2023-11-07T01:34:46Z

@huanghua78 any comments about this PR?

Sorry, I will take another look today.

dora/integration/fuse/src/main/java/alluxio/fuse/FuseSignalHandler.java

huanghua78

LGTM

lucyge2022 · 2023-11-13T22:05:46Z

hey @elega i made some changes to FuseHdfsIntegrationTest/AbstractFuseHdfsIntegrationTest to generate mount point to level of test methods instead of entire class.

lucyge2022 · 2023-11-13T22:07:00Z

alluxio-bot, merge this please.

alluxio-bot · 2023-11-13T22:07:02Z

merge failed:
Merge refused because check evaluation fail Check PR-EMAIL in PR 18241 for ref ef131a5 has state failure which is not success because Email not linked to a valid Github account

…m.umountInternal

2. removing umountInternal as we shouldn't call umount/fusermount from within AlluxioFuse program 3. modify alluxio-fuse umount, it shouldn't send SIGTERM it should instead using umount or fusermount to specifically instruct libfuse to stop serving and exit, 4. FuseSignalHandler should just clean up alluxio resources and stop under the condition that libfuse won't act to umount/fusermount

This reverts commit 819468b.

…every UT 2) use 'umount' cmd to unmount

lucyge2022 · 2023-11-14T00:23:44Z

alluxio-bot, merge this please.

### What changes are proposed in this pull request? clean up unmount of fuse ### Why are the changes needed? alluxio-fuse may cause AlluxioFuse continues to hang, and also the current flow to unmount is scattered and all over the place. ### Does this PR introduce any user facing changes? no pr-link: Alluxio#18241 change-id: cid-0246bd831466396697efe1977547d365ad63ba3d

lucyge2022 requested review from LuQQiu, apc999, huanghua78, jiacheliu3 and beinan October 5, 2023 18:07

lucyge2022 changed the title ~~Fuse unmount cleanup~~ Clean up fuse unmount workflow Oct 5, 2023

beinan approved these changes Oct 6, 2023

View reviewed changes

beinan reviewed Oct 6, 2023

View reviewed changes

ssz1997 approved these changes Oct 6, 2023

View reviewed changes

LuQQiu approved these changes Oct 6, 2023

View reviewed changes

jiacheliu3 requested changes Oct 7, 2023

View reviewed changes

jiacheliu3 approved these changes Oct 10, 2023

View reviewed changes

elega force-pushed the main branch from b597c61 to 4271e79 Compare October 17, 2023 15:08

apc999 force-pushed the main branch 2 times, most recently from 2fec0ec to b597c61 Compare October 17, 2023 19:11

huanghua78 reviewed Nov 8, 2023

View reviewed changes

dora/integration/fuse/src/main/java/alluxio/fuse/FuseSignalHandler.java Outdated Show resolved Hide resolved

lucyge2022 force-pushed the fuse-unmount-cleanup branch from de00e7a to 8d29fae Compare November 9, 2023 19:52

huanghua78 approved these changes Nov 10, 2023

View reviewed changes

lucyge2022 added the type-ease-of-use This issue is about of how to improve the ease of use of Alluxio product label Nov 13, 2023

lucyge2022 added 3 commits November 13, 2023 14:11

1) handle SIGINT as well 2) print cmd output of AbstractFuseFileSyste…

420a0eb

…m.umountInternal

compile error

377f87c

stash changes to reset signal hanlder

b897012

lucyge2022 added 14 commits November 13, 2023 14:11

implement destroy operation

608c5c4

use existing umount_fuse in alluxio-fuse

e3ada78

checstyle change

feb5528

review comments

2f42172

remove unused imports

60c4996

review comments

9e95c7c

review comments

ba33de0

update libjnifuse.dylib

2ffec90

try generating temp dir for fuse mnt point every time

e1a3ceb

Revert "try generating temp dir for fuse mnt point every time"

62722a8

This reverts commit 819468b.

fix AbstractFuseHdfsIntegrationTest to 1) create new mount point for …

bf204f1

…every UT 2) use 'umount' cmd to unmount

compile error

65bc519

update of libjnifuse.so and libjnifuse3.so

856c1a1

lucyge2022 force-pushed the fuse-unmount-cleanup branch from ef131a5 to 856c1a1 Compare November 13, 2023 22:15

alluxio-bot merged commit feb0116 into Alluxio:main Nov 14, 2023
12 checks passed

	* on receiving these signals on different platform(MacOs can receive/Linux can't)
	* on receiving these signals on different platform(MacOS can receive/Linux can't)

Clean up fuse unmount workflow #18241

Clean up fuse unmount workflow #18241

Conversation

lucyge2022 commented Oct 5, 2023

What changes are proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user facing changes?

alluxio-bot commented Oct 5, 2023

alluxio-bot commented Oct 5, 2023

beinan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssz1997 left a comment

Choose a reason for hiding this comment

LuQQiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiacheliu3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucyge2022 Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

jiacheliu3 left a comment

Choose a reason for hiding this comment

lucyge2022 commented Oct 16, 2023

Xenorith commented Oct 31, 2023

huanghua78 commented Nov 7, 2023

huanghua78 left a comment

Choose a reason for hiding this comment

lucyge2022 commented Nov 13, 2023

lucyge2022 commented Nov 13, 2023

alluxio-bot commented Nov 13, 2023

lucyge2022 commented Nov 14, 2023

lucyge2022 Oct 9, 2023 •

edited

Loading