On 08/25/2010 11:49 AM, Chris Lalancette wrote:
> On 08/24/10 - 12:19:07AM, Mohammed Morsi wrote:
>> diff --git a/src/app/models/instance_event.rb
>> b/src/app/models/instance_event.rb
>> new file mode 100644
>> index 0000000..cc1eaa0
>> --- /dev/null
>> +++ b/src/app/models/instance_event.rb
>> @@ -0,0 +1,30 @@
>> +# Copyright (C) 2010 Red Hat, Inc.
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; version 2 of the License.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program; if not, write to the Free Software
>> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
>> +# MA 02110-1301, USA. A copy of the GNU General Public License is
>> +# also available at http://www.gnu.org/copyleft/gpl.html.
>> +
>> +# Filters added to this controller apply to all controllers in the
>> application.
>> +# Likewise, all the methods added will be available for all controllers.
>> +
>> +class InstanceEvent< ActiveRecord::Base
>> + belongs_to :instance
>> +
>> + validates_presence_of :instance_id
>> + validates_presence_of :event_type
>> +
>> + #validates_inclusion_of :event_type,
>> + # :in => TYPES
> Just drop the commented out code, we can include it later when we need it.
All of these have been removed.
> <snip>
>
>> +
>> +# last position we've read in the log
>> +event_log_position = 0
>> +
>> +# persistantly store log position in filesystem
>> +# incase of dbomatic restarts
>> +if File.exists?(EVENT_LOG_POS_FILE)
>> + event_log_position = File.open(EVENT_LOG_POS_FILE, 'r').read.to_i
>> +end
> I think this is going to leave the file descriptor for EVENT_LOG_POS_FILE.
> It's probably not a huge deal, but given that you are going to be writing data
> to it later, it's probably best to close it out:
>
> file = File.open(EVENT_LOG_POS_FILE, 'r')
> event_log_position = file.read.to_i
> file.close
>
Done
>> +
>> +log_file = File.open(CONDOR_EVENT_LOG_FILE)
>> +log_file.pos = event_log_position
>> +
>> +# Setup inotify watch for condor event log
>> +notifier = INotify::Notifier.new
>> +notifier.watch(CONDOR_EVENT_LOG_FILE, :modify){ |event|
> I think you may also have to watch for the "new" event as well, for the first
> time a job is submitted.
This was a little tricker. The only way to do this was to add a watch to
the inotify 'create' event on the log directory and handle the EventLog
creation event when it occurs. Furthermore a watch can't be put on a
file that doesn't exist, so if the log file doesn't, the modify watch
gets added after the create watch is triggered. This seems like it would
potentially lead to a race condition, where if condor writes additional
data to the log file inbetween it being created and the modify watch
being added, dbomatic would not receive the change.
An alternate and simpler solution would be just to create an empty log
file at the beginning of dbomatic if it doesn't exist. The only drawback
would be is if condor is executing its first event, writing it to the
log file, at the very same time dbomatic is starting up, we would miss
that event. I'm not sure this is something we should care about, most
likely condor and dbomatic will both be started sometime before the
first event is received, and won't be an issue.
>> + while s = log_file.gets
>> + parser<< s
>> + end
>> + event_log_position = log_file.pos
>> + File.open(EVENT_LOG_POS_FILE, 'w') { |f| f.write event_log_position.to_s }
>> +}
>> +notifier.run
>> +
>> +parser<< "</events>"
>> +parser.finish
> Otherwise, it looks good. We'll also want to write a daemon initscript for
> this. Once you fix those pretty minor nits above, I can ACK and we can get
> this into the tree.
>
As a precursor to the sysv init script, I attempted to interface
dbomatic with the 'daemons' gems, as we had with oVirt, but ran into
some trouble. After quite a bit of debugging, apparently Daemonizing the
process at hand doesn't work too well when making use of FFI as
rb-inotify does. This will need to be looked into further, though we
don't necessarily need to interface with the daemons gem, it does make
things a bit nicer. Also there are a couple of other Ruby/inotify
wrappers, though rb-inotify seems to be the one most used / widespread,
I can look into whether or not this is an issue with the others
(rubygem-rb-inotify needs to be packaged up for Fedora in anycase). This
issue can be resolved as we go along though, for testing / development
purposes, it's a non-blocker.
Updated patch sent to deltacloud-devel.
-Mo
_______________________________________________
deltacloud-devel mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/deltacloud-devel
