[GitHub] storm pull request #2893: STORM-3272 allow worker-launcher to delete dead sy...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/storm/pull/2893


---

[GitHub] storm pull request #2893: STORM-3272 allow worker-launcher to delete dead sy...

[d2r]

Github user d2r commented on a diff in the pull request:

https://github.com/apache/storm/pull/2893#discussion_r227445654
  
--- Diff: storm-core/src/native/worker-launcher/impl/worker-launcher.c ---
@@ -593,14 +593,17 @@ int recursive_delete(const char *path, int 
supervisor_owns_dir) {
 return UNABLE_TO_BUILD_PATH;
   }
 
+  struct stat file_stat;
+
   if(access(path, F_OK) != 0) {
 if(errno == ENOENT) {
-  return 0;
-}
-// Can probably return here, but we'll try to lstat anyway.
-  }
+   // we need to handle symlinks that target missing files.
+   if((lstat(path, _stat) != 0) || ((file_stat.st_mode & S_IFMT) 
!= S_IFLNK)) {
+ return 0;
+   }
--- End diff --

Thanks. And I like that you have text that differentiates this from the 
other `lstat` error case.


---

[GitHub] storm pull request #2893: STORM-3272 allow worker-launcher to delete dead sy...

[agresch]

Github user agresch commented on a diff in the pull request:

https://github.com/apache/storm/pull/2893#discussion_r227434073
  
--- Diff: storm-core/src/native/worker-launcher/impl/worker-launcher.c ---
@@ -593,14 +593,17 @@ int recursive_delete(const char *path, int 
supervisor_owns_dir) {
 return UNABLE_TO_BUILD_PATH;
   }
 
+  struct stat file_stat;
+
   if(access(path, F_OK) != 0) {
 if(errno == ENOENT) {
-  return 0;
-}
-// Can probably return here, but we'll try to lstat anyway.
-  }
+   // we need to handle symlinks that target missing files.
+   if((lstat(path, _stat) != 0) || ((file_stat.st_mode & S_IFMT) 
!= S_IFLNK)) {
+ return 0;
+   }
--- End diff --

@d2r @revans2 - added logging


---

[GitHub] storm pull request #2893: STORM-3272 allow worker-launcher to delete dead sy...

[revans2]

Github user revans2 commented on a diff in the pull request:

https://github.com/apache/storm/pull/2893#discussion_r227418284
  
--- Diff: storm-core/src/native/worker-launcher/impl/worker-launcher.c ---
@@ -593,14 +593,17 @@ int recursive_delete(const char *path, int 
supervisor_owns_dir) {
 return UNABLE_TO_BUILD_PATH;
   }
 
+  struct stat file_stat;
+
   if(access(path, F_OK) != 0) {
 if(errno == ENOENT) {
-  return 0;
-}
-// Can probably return here, but we'll try to lstat anyway.
-  }
+   // we need to handle symlinks that target missing files.
+   if((lstat(path, _stat) != 0) || ((file_stat.st_mode & S_IFMT) 
!= S_IFLNK)) {
+ return 0;
+   }
--- End diff --

+1 to logging the failure.  The only time we get an ENOENT and lstat fails 
is either there was a race that the file was deleted out from under us (which 
should be rare so logging it is fine) or if something truly bad happened, and 
we have another problem we need to debug.


---

[GitHub] storm pull request #2893: STORM-3272 allow worker-launcher to delete dead sy...

[d2r]

Github user d2r commented on a diff in the pull request:

https://github.com/apache/storm/pull/2893#discussion_r227412685
  
--- Diff: storm-core/src/native/worker-launcher/impl/worker-launcher.c ---
@@ -593,14 +593,17 @@ int recursive_delete(const char *path, int 
supervisor_owns_dir) {
 return UNABLE_TO_BUILD_PATH;
   }
 
+  struct stat file_stat;
+
   if(access(path, F_OK) != 0) {
 if(errno == ENOENT) {
-  return 0;
-}
-// Can probably return here, but we'll try to lstat anyway.
-  }
+   // we need to handle symlinks that target missing files.
+   if((lstat(path, _stat) != 0) || ((file_stat.st_mode & S_IFMT) 
!= S_IFLNK)) {
+ return 0;
+   }
--- End diff --

If we fail to `lstat` the path, do we want to log a message as we do below?


---

[GitHub] storm pull request #2893: STORM-3272 allow worker-launcher to delete dead sy...

[agresch]

GitHub user agresch opened a pull request:

https://github.com/apache/storm/pull/2893

STORM-3272 allow worker-launcher to delete dead symlinks

When a topology folder contains a symlink to a missing file, the 
worker-launcher would not remove the symlink.  This prevents force delete from 
deleting the folder when cleaning up a topology.  

When a cluster is short on resources, this can cause all sorts of issues on 
the supervisor when rescheduling the same topology when this folder still 
exists.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/agresch/storm agresch_storm-3272

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/storm/pull/2893.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #2893


commit 131344777ae4cbc4ca47952b3a95f35b11713f23
Author: Aaron Gresch 
Date:   2018-10-23T13:57:52Z

STORM-3272 allow worker-launcher to delete dead symlinks




---