Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#3971 closed task (complete)

Use TimerMgr for lease file cleanup scheduling

Reported by: marcin Owned by: marcin
Priority: high Milestone: Kea1.0-beta
Component: dhcp Version: git
Keywords: expiration Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

The Design contains the description of the TimerMgr. The LFC must be triggered using this new mechanism.

Subtickets

Change History (10)

comment:1 Changed 5 years ago by marcin

  • Owner set to UnAssigned
  • Status changed from new to reviewing

This ticket can now be reviewed. The DHCPv4 and DHCPv6 use the TimerMgr to schedule LFC runs. I also improved some code in the Thread class to recognize whether we are in the current or child process. The child process doesn't inherit the threads of the main process, so the child process should not be surprised when the thread doesn't exist during Thread class destruction.

There is some room for improvement in the timers configuration to remember previously configured timers if the server reconfiguration fails. However, this is a general configuration issue in Kea, which also applies to IfaceMgr, StatsMgr, Logger, Hooks etc, and it should be resolved globally.

Proposed ChangeLog entry:

1XXX.	[func]		marcin
	New mechanism for scheduling lease file cleanup is used in the
	DHCPv4 and DHCPv6 servers.
	(Trac #3971, git abcd)

comment:2 Changed 5 years ago by tmark

  • Owner changed from UnAssigned to tmark

comment:3 follow-up: Changed 5 years ago by tmark

  • Owner changed from tmark to marcin

src/lib/dhcprsv/tests/memfile_lease_mgr_unittest.cc

TEST_F(MemfileLeaseMgrTest?, lfcTimer)

Technically, this following statement isn't accurate.
It's good form perhaps to call stopThread() explicitly,
but ~LFCSetup() calls it prior to unregistering it's timer.

    // Stop worker thread to make sure it is not running when lease
    // manager is destroyed. The lease manager will be unable to
    // unregster timer when the thread is active.

  • util/threads
  1. ForkDetector?'s purpose is provide a means to know if the process you are in is the one which created the instance of ForkDetector?.

You're using it to decide if failed calls to pthread_detach or pthread_join
should be handled as errors (i.e. this process created the thread) or
whether they should be ingored (i.e. this process did not create the thread).

Since you have the ability to know that the call is going to fail, (i.e.
the process does not own the thread), why make the call? Why not only make
the call when the process owns the thread?

Maybe I'm missing something.

  1. I think it would clearer to add something like this to directly to Thread:
    pid_t creator_pid_;

    bool amOwnerProcess() { return (created_by_pid_ == getpid()); } 

rather than an external class, ForkDetector?. I'm just not sure that class
is useful in any other circumstances and for me, it sort of clouds the
issue. Usually I am in favor of creating classes for things, in this
case I am not.

  1. There are no unit tests for failed forks. Since this does represent a threat to the servers now, shouldn't we have tests for this?

src/lib/testutils/dhcp_test_lib.sh.in

cleanup() -

What happens if ${LEASE_FILE} is "" for some reason?

       rm -rf ${LEASE_FILE}*

Of course it shouldn't be, but still maybe you
should test it first.


General:

Not necessarily part of this ticket, but it looks to me like we rely on
ProcessSpawn? to detect if LFC is still running, rather than checking
in lfcCallback first. Doesn't this seem a bit far down stream?
Why do we not use Memfile_LeaseMgr::isLFCRunning()? We appear to have
the knowledge on hand to know spawn won't work before we make the
call but don't use it. This seems wrong to me.

comment:4 in reply to: ↑ 3 ; follow-up: Changed 5 years ago by marcin

  • Owner changed from marcin to tmark

Replying to tmark:

src/lib/dhcprsv/tests/memfile_lease_mgr_unittest.cc

TEST_F(MemfileLeaseMgrTest?, lfcTimer)

Technically, this following statement isn't accurate.
It's good form perhaps to call stopThread() explicitly,
but ~LFCSetup() calls it prior to unregistering it's timer.

    // Stop worker thread to make sure it is not running when lease
    // manager is destroyed. The lease manager will be unable to
    // unregster timer when the thread is active.

It was a leftover from the earlier version for which it was true.


  • util/threads
  1. ForkDetector?'s purpose is provide a means to know if the process you are in is the one which created the instance of ForkDetector?.

You're using it to decide if failed calls to pthread_detach or pthread_join
should be handled as errors (i.e. this process created the thread) or
whether they should be ingored (i.e. this process did not create the thread).

Since you have the ability to know that the call is going to fail, (i.e.
the process does not own the thread), why make the call? Why not only make
the call when the process owns the thread?

Maybe I'm missing something.

In most cases it is true. It was even working like this in my first attempt to implement it. However, theoretically the child process may "recreate" threads, if it needs to. In that case, being in a child process doesn't need to mean anything. But, for sure it means something when you fail with a given error number. So, I think this is actually more generic approach.

  1. I think it would clearer to add something like this to directly to Thread:
    pid_t creator_pid_;

    bool amOwnerProcess() { return (created_by_pid_ == getpid()); } 

rather than an external class, ForkDetector?. I'm just not sure that class
is useful in any other circumstances and for me, it sort of clouds the
issue. Usually I am in favor of creating classes for things, in this
case I am not.

Again, this was my first attempt. But, I isolated it into a separate class simply because in the future there may be other classes that will need to defend against errors in child processes.

  1. There are no unit tests for failed forks. Since this does represent a threat to the servers now, shouldn't we have tests for this?

I think I have come up with a nice and clever test for it. :-)


src/lib/testutils/dhcp_test_lib.sh.in

cleanup() -

What happens if ${LEASE_FILE} is "" for some reason?

       rm -rf ${LEASE_FILE}*

Of course it shouldn't be, but still maybe you
should test it first.

I am checking it for being empty now.


General:

Not necessarily part of this ticket, but it looks to me like we rely on
ProcessSpawn? to detect if LFC is still running, rather than checking
in lfcCallback first. Doesn't this seem a bit far down stream?
Why do we not use Memfile_LeaseMgr::isLFCRunning()? We appear to have
the knowledge on hand to know spawn won't work before we make the
call but don't use it. This seems wrong to me.

Which part of code you're referring to when saying "we rely on ProcessSpawn? to detect if LFC is still running"?

comment:5 in reply to: ↑ 4 ; follow-up: Changed 5 years ago by tmark

Replying to marcin:

Replying to tmark:

src/lib/dhcprsv/tests/memfile_lease_mgr_unittest.cc

TEST_F(MemfileLeaseMgrTest?, lfcTimer)

Technically, this following statement isn't accurate.
It's good form perhaps to call stopThread() explicitly,
but ~LFCSetup() calls it prior to unregistering it's timer.

    // Stop worker thread to make sure it is not running when lease
    // manager is destroyed. The lease manager will be unable to
    // unregster timer when the thread is active.

It was a leftover from the earlier version for which it was true.

That's what I figured.


  • util/threads
  1. ForkDetector?'s purpose is provide a means to know if the process you are in is the one which created the instance of ForkDetector?.

You're using it to decide if failed calls to pthread_detach or pthread_join
should be handled as errors (i.e. this process created the thread) or
whether they should be ingored (i.e. this process did not create the thread).

Since you have the ability to know that the call is going to fail, (i.e.
the process does not own the thread), why make the call? Why not only make
the call when the process owns the thread?

Maybe I'm missing something.

In most cases it is true. It was even working like this in my first attempt to implement it. However, theoretically the child process may "recreate" threads, if it needs to. In that case, being in a child process doesn't need to mean anything. But, for sure it means something when you fail with a given error number. So, I think this is actually more generic approach.

If the child process creates a thread, that thread's creator_pid will be that of the child
process - not the original process, so the logic should still work. You're really checking whether the process you are in is the same process which created the PID.
But leave it as you have it.

  1. I think it would clearer to add something like this to directly to Thread:
    pid_t creator_pid_;

    bool amOwnerProcess() { return (created_by_pid_ == getpid()); } 

rather than an external class, ForkDetector?. I'm just not sure that class
is useful in any other circumstances and for me, it sort of clouds the
issue. Usually I am in favor of creating classes for things, in this
case I am not.

Again, this was my first attempt. But, I isolated it into a separate class simply because in the future there may be other classes that will need to defend against errors in child processes.

  1. There are no unit tests for failed forks. Since this does represent a threat to the servers now, shouldn't we have tests for this?

I think I have come up with a nice and clever test for it. :-)

Very clever indeed. Nicely done. Now how about success case?
You can make one by calling "pwd" and looking for an exit of success.


src/lib/testutils/dhcp_test_lib.sh.in

cleanup() -

What happens if ${LEASE_FILE} is "" for some reason?

       rm -rf ${LEASE_FILE}*

Of course it shouldn't be, but still maybe you
should test it first.

I am checking it for being empty now.


General:

Not necessarily part of this ticket, but it looks to me like we rely on
ProcessSpawn? to detect if LFC is still running, rather than checking
in lfcCallback first. Doesn't this seem a bit far down stream?
Why do we not use Memfile_LeaseMgr::isLFCRunning()? We appear to have
the knowledge on hand to know spawn won't work before we make the
call but don't use it. This seems wrong to me.

Which part of code you're referring to when saying "we rely on ProcessSpawn? to detect if LFC is still running"?

Disregard. I see that we are relying on the presence of interim lease file(s) in
Memfile_LeaseMgr::lfcExecute() to decide if LFC is running or not. It does seem odd
that we don't call isLFCRunning() first. It seems to work so leave it.

comment:6 Changed 5 years ago by tmark

  • Owner changed from tmark to marcin

comment:7 in reply to: ↑ 5 Changed 5 years ago by marcin

  • Owner changed from marcin to tmark

Replying to tmark:

Replying to marcin:

Replying to tmark:

src/lib/dhcprsv/tests/memfile_lease_mgr_unittest.cc

TEST_F(MemfileLeaseMgrTest?, lfcTimer)

Technically, this following statement isn't accurate.
It's good form perhaps to call stopThread() explicitly,
but ~LFCSetup() calls it prior to unregistering it's timer.

    // Stop worker thread to make sure it is not running when lease
    // manager is destroyed. The lease manager will be unable to
    // unregster timer when the thread is active.

It was a leftover from the earlier version for which it was true.

That's what I figured.


  • util/threads
  1. ForkDetector?'s purpose is provide a means to know if the process you are in is the one which created the instance of ForkDetector?.

You're using it to decide if failed calls to pthread_detach or pthread_join
should be handled as errors (i.e. this process created the thread) or
whether they should be ingored (i.e. this process did not create the thread).

Since you have the ability to know that the call is going to fail, (i.e.
the process does not own the thread), why make the call? Why not only make
the call when the process owns the thread?

Maybe I'm missing something.

In most cases it is true. It was even working like this in my first attempt to implement it. However, theoretically the child process may "recreate" threads, if it needs to. In that case, being in a child process doesn't need to mean anything. But, for sure it means something when you fail with a given error number. So, I think this is actually more generic approach.

If the child process creates a thread, that thread's creator_pid will be that of the child
process - not the original process, so the logic should still work. You're really checking whether the process you are in is the same process which created the PID.
But leave it as you have it.

  1. I think it would clearer to add something like this to directly to Thread:
    pid_t creator_pid_;

    bool amOwnerProcess() { return (created_by_pid_ == getpid()); } 

rather than an external class, ForkDetector?. I'm just not sure that class
is useful in any other circumstances and for me, it sort of clouds the
issue. Usually I am in favor of creating classes for things, in this
case I am not.

Again, this was my first attempt. But, I isolated it into a separate class simply because in the future there may be other classes that will need to defend against errors in child processes.

  1. There are no unit tests for failed forks. Since this does represent a threat to the servers now, shouldn't we have tests for this?

I think I have come up with a nice and clever test for it. :-)

Very clever indeed. Nicely done. Now how about success case?
You can make one by calling "pwd" and looking for an exit of success.

I could do that, but there are two considerations here:

  • If execvp is successful the child process is replaced by a new image and no thread destructors will be called whatsoever. So, this would not really test behavior of the Thread class.
  • If we run unix command line 'pwd' there will be voices that this is not portable. So, we should probably create a new application/script that returns error code of 0. Is it worth it given first consideration?

src/lib/testutils/dhcp_test_lib.sh.in

cleanup() -

What happens if ${LEASE_FILE} is "" for some reason?

       rm -rf ${LEASE_FILE}*

Of course it shouldn't be, but still maybe you
should test it first.

I am checking it for being empty now.


General:

Not necessarily part of this ticket, but it looks to me like we rely on
ProcessSpawn? to detect if LFC is still running, rather than checking
in lfcCallback first. Doesn't this seem a bit far down stream?
Why do we not use Memfile_LeaseMgr::isLFCRunning()? We appear to have
the knowledge on hand to know spawn won't work before we make the
call but don't use it. This seems wrong to me.

Which part of code you're referring to when saying "we rely on ProcessSpawn? to detect if LFC is still running"?

Disregard. I see that we are relying on the presence of interim lease file(s) in
Memfile_LeaseMgr::lfcExecute() to decide if LFC is running or not. It does seem odd
that we don't call isLFCRunning() first. It seems to work so leave it.

comment:8 Changed 5 years ago by tmark

  • Owner changed from tmark to marcin

Don't worry about the test. Please merge your changes.

comment:9 Changed 5 years ago by marcin

  • Resolution set to complete
  • Status changed from reviewing to closed

Merged with commit 431d515fc3d64aa82369c8eaf48d03339f12dc69

comment:10 Changed 4 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.