On Wed, 2010-08-25 at 11:49 -0400, 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
I think Chris covered everything.. my only question is the use of
'instance event' for the events.. I'm just wondering if that's the best
name for it. Will we always be logging only events related to instances
here? Are they really 'instance' events or 'job' events, or..?
Just a thought anyway.. we can leave it for now but I wanted to think
about this a bit.
Also, are we just feeding the event types directly into the db? Don't
we want to define some constants for them and name them more to our
scheme? eg when the match is done you'll get some kind of event which
we can name INSTANCE_EVENT_MATCH_COMPLETE or such..
Otherwise it looks really good. I like the use of inotify :).
Ian
> > @@ -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.
>
> <snip>
>
> > diff --git a/src/dbomatic/dbomatic.rb b/src/dbomatic/dbomatic.rb
> > new file mode 100644
> > index 0000000..8a05362
> > --- /dev/null
> > +++ b/src/dbomatic/dbomatic.rb
> > @@ -0,0 +1,98 @@
> ...
> > + # Create a new entry for events which we have all the neccessary data for
> > + def end_element(element)
> > + if element == "c" && !...@event_cmd.nil?
> > + # Condor may write to event log before condormatic returns and
> > instance
> > + # table is updated. Extract instance name from event_cmd and query
> > on that
> > + inst_name = @event_cmd[4,@event_cmd.size-4].gsub(/_[0-9]*$/, '')
> > + inst = Instance.find(:first, :conditions => ['name = ?', inst_name])
> > + #puts "Instance event #{inst.name} #...@event_type} #...@event_time}"
>
> Same here.
>
> > + InstanceEvent.create! :instance => inst,
> > + :event_type => @event_type,
> > + :event_time => @event_time
> > + @tag = @event_type = @event_cmd = @event_time = nil
> > + end
> > + end
> > +end
> > +parser = Nokogiri::XML::SAX::PushParser.new(CondorEventLog.new)
> > +
> > +# XXX hack, condor event log doesn't seem to have a top level element
> > +# enclosing everything else in the doc (as standards conforming xml must).
> > +# Create one for parsing purposes.
> > +parser << "<events>"
> > +
> > +# last time the event log was modified
> > +#event_log_timestamp = nil
>
> Same here.
>
> > +
> > +# 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
>
> > +
> > +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.
>
> > + 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.
>
_______________________________________________
deltacloud-devel mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/deltacloud-devel
