subject:"\[devel\] \[PATCH 3 of 3\] dtm\: Use inotify to improve response time for transport monitor process V2 \[#2091\]"

Re: [devel] [PATCH 3 of 3] dtm: Use inotify to improve response time for transport monitor process V2 [#2091]

2016-10-21 Thread Anders Widell

Another possibility is to use cgroups, but I think it is a bit overkill 
here:

http://stackoverflow.com/questions/35878292/get-notification-on-cgroup-process-change

However, for AMF I think cgroup monitoring could be useful. I wrote 
ticket [#2135] to track this.

For this use case, since we can modify osafdtmd, I think the FIFO 
monitoring approach would be easiest.

/ Anders Widell


On 10/21/2016 03:40 PM, Anders Widell wrote:
> Hi!
>
> I have identified a problem with this change: inotify doesn't work 
> together with the /proc file system. The thing that happens when I 
> kill osafdtmd is that it takes up to 15 seconds for inotify to time 
> out, and then in the next iteration of the loop it will detect that 
> the directory is already deleted (ENOENT). So it is OK to wait for the 
> creation of the pid file, but the code that monitors the process until 
> it dies needs to be reconsidered.
>
> Maybe we can look at some of the suggestions presented here:
>
> http://stackoverflow.com/questions/1157700/how-to-wait-for-exit-of-non-children-processes
>  
>
>
> / Anders Widell
>
> On 10/17/2016 06:25 PM, Hans Nordeback wrote:
>> osaf/services/infrastructure/dtms/transport/tests/Makefile.am |   3 +-
>> osaf/services/infrastructure/dtms/transport/tests/transport_monitor_test.cc 
>> |   4 +-
>> osaf/services/infrastructure/dtms/transport/transport_monitor.cc |  
>> 38 ++---
>>   3 files changed, 30 insertions(+), 15 deletions(-)
>>
>>
>> diff --git 
>> a/osaf/services/infrastructure/dtms/transport/tests/Makefile.am 
>> b/osaf/services/infrastructure/dtms/transport/tests/Makefile.am
>> --- a/osaf/services/infrastructure/dtms/transport/tests/Makefile.am
>> +++ b/osaf/services/infrastructure/dtms/transport/tests/Makefile.am
>> @@ -42,4 +42,5 @@ transport_test_SOURCES = \
>> transport_test_LDADD = \
>>   $(GTEST_DIR)/lib/libgtest.la \
>> -$(GTEST_DIR)/lib/libgtest_main.la
>> +$(GTEST_DIR)/lib/libgtest_main.la \
>> +$(top_builddir)/osaf/libs/core/libopensaf_core.la
>> diff --git 
>> a/osaf/services/infrastructure/dtms/transport/tests/transport_monitor_test.cc
>>  
>> b/osaf/services/infrastructure/dtms/transport/tests/transport_monitor_test.cc
>>  
>>
>> --- 
>> a/osaf/services/infrastructure/dtms/transport/tests/transport_monitor_test.cc
>> +++ 
>> b/osaf/services/infrastructure/dtms/transport/tests/transport_monitor_test.cc
>> @@ -82,14 +82,14 @@ class TransportMonitorTest : public ::te
>>   TEST_F(TransportMonitorTest, WaitForNonexistentDaemonName) {
>> pid_t pid = monitor_->WaitForDaemon("name_does_not_exist", 17);
>> EXPECT_EQ(pid, pid_t{-1});
>> -  EXPECT_EQ(mock_osaf_poll.invocations, 17);
>> +  EXPECT_EQ(mock_osaf_poll.invocations, 0);
>>   }
>> TEST_F(TransportMonitorTest, WaitForNonexistentDaemonPid) {
>> CreatePidFile("pid_does_not_exist", 1234567890, false);
>> pid_t pid = monitor_->WaitForDaemon("pid_does_not_exist", 1);
>> EXPECT_EQ(pid, pid_t{-1});
>> -  EXPECT_EQ(mock_osaf_poll.invocations, 1);
>> +  EXPECT_EQ(mock_osaf_poll.invocations, 0);
>>   }
>> TEST_F(TransportMonitorTest, WaitForExistingPid) {
>> diff --git 
>> a/osaf/services/infrastructure/dtms/transport/transport_monitor.cc 
>> b/osaf/services/infrastructure/dtms/transport/transport_monitor.cc
>> --- a/osaf/services/infrastructure/dtms/transport/transport_monitor.cc
>> +++ b/osaf/services/infrastructure/dtms/transport/transport_monitor.cc
>> @@ -23,8 +23,11 @@
>>   #include 
>>   #include 
>>   #include "./configmake.h"
>> +#include "logtrace.h"
>> +#include "osaf_time.h"
>>   #include "osaf/libs/core/common/include/osaf_poll.h"
>>   #include "osaf/libs/core/cplusplus/base/getenv.h"
>> +#include "osaf/libs/core/cplusplus/base/file_notify.h"
>> TransportMonitor::TransportMonitor(int term_fd)
>>   : term_fd_{term_fd},
>> @@ -43,19 +46,23 @@ pid_t TransportMonitor::WaitForDaemon(co
>> int64_t seconds_to_wait) {
>> std::string pidfile = pkgpiddir_ + "/" + daemon_name + ".pid";
>> pid_t pid = pid_t{-1};
>> -  for (int64_t count = 0; count != seconds_to_wait; ++count) {
>> -if (!(std::ifstream{pidfile} >> pid).fail()
>> -&& IsDir(proc_path_ + std::to_string(pid)))
>> -  break;
>> -pid = pid_t{-1};
>> -if (Sleep(1))
>> -  return pid_t{-1};
>> +  base::FileNotify file_notify;
>> +  base::FileNotify::FileNotifyErrors rc =
>> +  file_notify.WaitForFileCreation(pidfile, seconds_to_wait * 
>> kMillisPerSec,
>> +  term_fd_);
>> +
>> +  if (rc == base::FileNotify::FileNotifyErrors::kOK) {
>> +if (!(std::ifstream{pidfile} >> pid).fail()) {
>> +  if (IsDir(proc_path_ + std::to_string(pid))) {
>> +return pid;
>> +  }
>> +}
>> }
>> -  return pid;
>> +  return pid_t{-1};
>>   }
>> bool TransportMonitor::Sleep(int64_t seconds_to_wait) {
>> -  return osaf_poll_one_fd(term_fd_, seconds_to_wait * 1000) != 0;
>> +  return osaf_poll_one_fd(term_fd_,

Re: [devel] [PATCH 3 of 3] dtm: Use inotify to improve response time for transport monitor process V2 [#2091]

2016-10-21 Thread Anders Widell

Hi!

I have identified a problem with this change: inotify doesn't work 
together with the /proc file system. The thing that happens when I kill 
osafdtmd is that it takes up to 15 seconds for inotify to time out, and 
then in the next iteration of the loop it will detect that the directory 
is already deleted (ENOENT). So it is OK to wait for the creation of the 
pid file, but the code that monitors the process until it dies needs to 
be reconsidered.

Maybe we can look at some of the suggestions presented here:

http://stackoverflow.com/questions/1157700/how-to-wait-for-exit-of-non-children-processes

/ Anders Widell

On 10/17/2016 06:25 PM, Hans Nordeback wrote:
>   osaf/services/infrastructure/dtms/transport/tests/Makefile.am   
> |   3 +-
>   osaf/services/infrastructure/dtms/transport/tests/transport_monitor_test.cc 
> |   4 +-
>   osaf/services/infrastructure/dtms/transport/transport_monitor.cc
> |  38 ++---
>   3 files changed, 30 insertions(+), 15 deletions(-)
>
>
> diff --git a/osaf/services/infrastructure/dtms/transport/tests/Makefile.am 
> b/osaf/services/infrastructure/dtms/transport/tests/Makefile.am
> --- a/osaf/services/infrastructure/dtms/transport/tests/Makefile.am
> +++ b/osaf/services/infrastructure/dtms/transport/tests/Makefile.am
> @@ -42,4 +42,5 @@ transport_test_SOURCES = \
>   
>   transport_test_LDADD = \
>   $(GTEST_DIR)/lib/libgtest.la \
> - $(GTEST_DIR)/lib/libgtest_main.la
> + $(GTEST_DIR)/lib/libgtest_main.la \
> + $(top_builddir)/osaf/libs/core/libopensaf_core.la
> diff --git 
> a/osaf/services/infrastructure/dtms/transport/tests/transport_monitor_test.cc 
> b/osaf/services/infrastructure/dtms/transport/tests/transport_monitor_test.cc
> --- 
> a/osaf/services/infrastructure/dtms/transport/tests/transport_monitor_test.cc
> +++ 
> b/osaf/services/infrastructure/dtms/transport/tests/transport_monitor_test.cc
> @@ -82,14 +82,14 @@ class TransportMonitorTest : public ::te
>   TEST_F(TransportMonitorTest, WaitForNonexistentDaemonName) {
> pid_t pid = monitor_->WaitForDaemon("name_does_not_exist", 17);
> EXPECT_EQ(pid, pid_t{-1});
> -  EXPECT_EQ(mock_osaf_poll.invocations, 17);
> +  EXPECT_EQ(mock_osaf_poll.invocations, 0);
>   }
>   
>   TEST_F(TransportMonitorTest, WaitForNonexistentDaemonPid) {
> CreatePidFile("pid_does_not_exist", 1234567890, false);
> pid_t pid = monitor_->WaitForDaemon("pid_does_not_exist", 1);
> EXPECT_EQ(pid, pid_t{-1});
> -  EXPECT_EQ(mock_osaf_poll.invocations, 1);
> +  EXPECT_EQ(mock_osaf_poll.invocations, 0);
>   }
>   
>   TEST_F(TransportMonitorTest, WaitForExistingPid) {
> diff --git a/osaf/services/infrastructure/dtms/transport/transport_monitor.cc 
> b/osaf/services/infrastructure/dtms/transport/transport_monitor.cc
> --- a/osaf/services/infrastructure/dtms/transport/transport_monitor.cc
> +++ b/osaf/services/infrastructure/dtms/transport/transport_monitor.cc
> @@ -23,8 +23,11 @@
>   #include 
>   #include 
>   #include "./configmake.h"
> +#include "logtrace.h"
> +#include "osaf_time.h"
>   #include "osaf/libs/core/common/include/osaf_poll.h"
>   #include "osaf/libs/core/cplusplus/base/getenv.h"
> +#include "osaf/libs/core/cplusplus/base/file_notify.h"
>   
>   TransportMonitor::TransportMonitor(int term_fd)
>   : term_fd_{term_fd},
> @@ -43,19 +46,23 @@ pid_t TransportMonitor::WaitForDaemon(co
> int64_t seconds_to_wait) {
> std::string pidfile = pkgpiddir_ + "/" + daemon_name + ".pid";
> pid_t pid = pid_t{-1};
> -  for (int64_t count = 0; count != seconds_to_wait; ++count) {
> -if (!(std::ifstream{pidfile} >> pid).fail()
> -&& IsDir(proc_path_ + std::to_string(pid)))
> -  break;
> -pid = pid_t{-1};
> -if (Sleep(1))
> -  return pid_t{-1};
> +  base::FileNotify file_notify;
> +  base::FileNotify::FileNotifyErrors rc =
> +  file_notify.WaitForFileCreation(pidfile, seconds_to_wait * 
> kMillisPerSec,
> +  term_fd_);
> +
> +  if (rc == base::FileNotify::FileNotifyErrors::kOK) {
> +if (!(std::ifstream{pidfile} >> pid).fail()) {
> +  if (IsDir(proc_path_ + std::to_string(pid))) {
> +return pid;
> +  }
> +}
> }
> -  return pid;
> +  return pid_t{-1};
>   }
>   
>   bool TransportMonitor::Sleep(int64_t seconds_to_wait) {
> -  return osaf_poll_one_fd(term_fd_, seconds_to_wait * 1000) != 0;
> +  return osaf_poll_one_fd(term_fd_, seconds_to_wait * kMillisPerSec) != 0;
>   }
>   
>   bool TransportMonitor::IsDir(const std::string& path) {
> @@ -65,6 +72,7 @@ bool TransportMonitor::IsDir(const std::
>   }
>   
>   void TransportMonitor::RotateMdsLogs(pid_t pid_to_watch) {
> +  base::FileNotify file_notify;
> std::string pid_path{proc_path_ + std::to_string(pid_to_watch)};
> for (;;) {
>   if (FileSize(mds_log_file_) > kMaxFileSize) {
> @@ -72,9 +80,15 @@ void TransportMonitor::RotateMdsLogs(pid
> rename(mds_log_file(),

[devel] [PATCH 3 of 3] dtm: Use inotify to improve response time for transport monitor process V2 [#2091]

2016-10-17 Thread Hans Nordeback

 osaf/services/infrastructure/dtms/transport/tests/Makefile.am   |  
 3 +-
 osaf/services/infrastructure/dtms/transport/tests/transport_monitor_test.cc |  
 4 +-
 osaf/services/infrastructure/dtms/transport/transport_monitor.cc|  
38 ++---
 3 files changed, 30 insertions(+), 15 deletions(-)


diff --git a/osaf/services/infrastructure/dtms/transport/tests/Makefile.am 
b/osaf/services/infrastructure/dtms/transport/tests/Makefile.am
--- a/osaf/services/infrastructure/dtms/transport/tests/Makefile.am
+++ b/osaf/services/infrastructure/dtms/transport/tests/Makefile.am
@@ -42,4 +42,5 @@ transport_test_SOURCES = \
 
 transport_test_LDADD = \
$(GTEST_DIR)/lib/libgtest.la \
-   $(GTEST_DIR)/lib/libgtest_main.la
+   $(GTEST_DIR)/lib/libgtest_main.la \
+   $(top_builddir)/osaf/libs/core/libopensaf_core.la
diff --git 
a/osaf/services/infrastructure/dtms/transport/tests/transport_monitor_test.cc 
b/osaf/services/infrastructure/dtms/transport/tests/transport_monitor_test.cc
--- 
a/osaf/services/infrastructure/dtms/transport/tests/transport_monitor_test.cc
+++ 
b/osaf/services/infrastructure/dtms/transport/tests/transport_monitor_test.cc
@@ -82,14 +82,14 @@ class TransportMonitorTest : public ::te
 TEST_F(TransportMonitorTest, WaitForNonexistentDaemonName) {
   pid_t pid = monitor_->WaitForDaemon("name_does_not_exist", 17);
   EXPECT_EQ(pid, pid_t{-1});
-  EXPECT_EQ(mock_osaf_poll.invocations, 17);
+  EXPECT_EQ(mock_osaf_poll.invocations, 0);
 }
 
 TEST_F(TransportMonitorTest, WaitForNonexistentDaemonPid) {
   CreatePidFile("pid_does_not_exist", 1234567890, false);
   pid_t pid = monitor_->WaitForDaemon("pid_does_not_exist", 1);
   EXPECT_EQ(pid, pid_t{-1});
-  EXPECT_EQ(mock_osaf_poll.invocations, 1);
+  EXPECT_EQ(mock_osaf_poll.invocations, 0);
 }
 
 TEST_F(TransportMonitorTest, WaitForExistingPid) {
diff --git a/osaf/services/infrastructure/dtms/transport/transport_monitor.cc 
b/osaf/services/infrastructure/dtms/transport/transport_monitor.cc
--- a/osaf/services/infrastructure/dtms/transport/transport_monitor.cc
+++ b/osaf/services/infrastructure/dtms/transport/transport_monitor.cc
@@ -23,8 +23,11 @@
 #include 
 #include 
 #include "./configmake.h"
+#include "logtrace.h"
+#include "osaf_time.h"
 #include "osaf/libs/core/common/include/osaf_poll.h"
 #include "osaf/libs/core/cplusplus/base/getenv.h"
+#include "osaf/libs/core/cplusplus/base/file_notify.h"
 
 TransportMonitor::TransportMonitor(int term_fd)
 : term_fd_{term_fd},
@@ -43,19 +46,23 @@ pid_t TransportMonitor::WaitForDaemon(co
   int64_t seconds_to_wait) {
   std::string pidfile = pkgpiddir_ + "/" + daemon_name + ".pid";
   pid_t pid = pid_t{-1};
-  for (int64_t count = 0; count != seconds_to_wait; ++count) {
-if (!(std::ifstream{pidfile} >> pid).fail()
-&& IsDir(proc_path_ + std::to_string(pid)))
-  break;
-pid = pid_t{-1};
-if (Sleep(1))
-  return pid_t{-1};
+  base::FileNotify file_notify;
+  base::FileNotify::FileNotifyErrors rc =
+  file_notify.WaitForFileCreation(pidfile, seconds_to_wait * kMillisPerSec,
+  term_fd_);
+
+  if (rc == base::FileNotify::FileNotifyErrors::kOK) {
+if (!(std::ifstream{pidfile} >> pid).fail()) {
+  if (IsDir(proc_path_ + std::to_string(pid))) {
+return pid;
+  }
+}
   }
-  return pid;
+  return pid_t{-1};
 }
 
 bool TransportMonitor::Sleep(int64_t seconds_to_wait) {
-  return osaf_poll_one_fd(term_fd_, seconds_to_wait * 1000) != 0;
+  return osaf_poll_one_fd(term_fd_, seconds_to_wait * kMillisPerSec) != 0;
 }
 
 bool TransportMonitor::IsDir(const std::string& path) {
@@ -65,6 +72,7 @@ bool TransportMonitor::IsDir(const std::
 }
 
 void TransportMonitor::RotateMdsLogs(pid_t pid_to_watch) {
+  base::FileNotify file_notify;
   std::string pid_path{proc_path_ + std::to_string(pid_to_watch)};
   for (;;) {
 if (FileSize(mds_log_file_) > kMaxFileSize) {
@@ -72,9 +80,15 @@ void TransportMonitor::RotateMdsLogs(pid
   rename(mds_log_file(), old_mds_log_file());
 }
 if (pid_to_watch != pid_t{0}) {
-  for (int64_t i = 0; i != kLogRotationIntervalInSeconds; ++i) {
-if (!IsDir(pid_path) || Sleep(1))
-  return;
+  base::FileNotify::FileNotifyErrors rc =
+  file_notify.WaitForFileDeletion(
+  pid_path,
+  kLogRotationIntervalInSeconds * kMillisPerSec,
+  term_fd_);
+  if (rc != base::FileNotify::FileNotifyErrors::kTimeOut) {
+return;
+  } else {
+TRACE("Timeout received, continuing...");
   }
 } else {
   if (Sleep(kLogRotationIntervalInSeconds))

--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
Opensaf-devel mailing list

Re: [devel] [PATCH 3 of 3] dtm: Use inotify to improve response time for transport monitor process V2 [#2091]

Re: [devel] [PATCH 3 of 3] dtm: Use inotify to improve response time for transport monitor process V2 [#2091]

[devel] [PATCH 3 of 3] dtm: Use inotify to improve response time for transport monitor process V2 [#2091]

3 matches

Site Navigation

Mail list logo

Footer information