[Ovirt-devel] [PATCH server] network integration into ovirt server db and wui

Fri Oct 31 14:47:43 UTC 2008

Hey Darryl,

   Thanks for the feedback. Comments are also inline below.

Darryl Pierce wrote:
> NAK.
>
> Some of the first things I've noticed is that, by removing columns from
> the bondings and nics tables into the separate IP-related tables,
> updates need to be made to the ManagedNodeConfiguration class. Since the
> data itself is the same and just relocated, refactoring the
> configuration class to get data from the appropriate objects should take
> care of things initially.
>
> Before this patch can be pushed, it needs to fix that class. The unit
> test itself does not need changing since the data ought to still look
> the same once encoded. There may be changes needed for vlans, but that
> will be additional tests.
>
Did you say on irc you were going to take care of this
ManagedNodeConfiguration bit or did I mishear?

[snip]

> > diff --git a/src/app/models/bonding.rb b/src/app/models/bonding.rb
> > index 006c261..a3ad150 100644
> > --- a/src/app/models/bonding.rb
> > +++ b/src/app/models/bonding.rb
> > @@ -30,6 +30,16 @@
> >  # interface. They can be ignored if not used.
> >  #
> >  class Bonding < ActiveRecord::Base
> > +  belongs_to :host
> > +  belongs_to :bonding_type
> > +  belongs_to :vlan
> > +  has_many :ip_addresses, :dependent => :destroy
> > +
> > +  has_and_belongs_to_many :nics,
> > +    :join_table  => 'bondings_nics',
> > +    :foreign_key => :bonding_id
> > +
> > +
> >    validates_presence_of :name,
> >      :message => 'A name is required.'
>
> > @@ -42,18 +52,17 @@ class Bonding < ActiveRecord::Base
> >    validates_presence_of :interface_name,
> >      :message => 'An interface name is required.'
>
> > -  validates_presence_of :boot_type_id,
> > -    :message => 'A boot type must be specified.'
> > -
> >    validates_presence_of :bonding_type_id,
> >      :message => 'A bonding type must be specified.'
>
> > -  belongs_to :host
> > -  belongs_to :bonding_type
> > -  belongs_to :boot_type
> > + protected
> > +  def validate
> > +    errors.add("name", "must be specified") unless name
> > +    errors.add("interface_name", "must be specified") unless
> interface_name
> > +    errors.add("bonding_type_id", "must be specified") unless
> bonding_type_id
>
> This is duplicating the above validation with the validates_presence_of
> check.
The thing I noticed with validates_presence_of is that I found that it
doesn't seem to trigger the validation components of the interface. For
example if name isn't specified in the wui which is caught with a
validate_presence_of instead of in the validate function, the
corresponding errors is never added to the 'errors' array and thus the
'name' textbox on the interface is never highlighted so that the user
knows that he/she must provide that information.

[snip]

>
> > diff --git a/src/app/models/ip_v4_address.rb
> b/src/app/models/ip_v4_address.rb
> > new file mode 100644
> > index 0000000..40e8cf9
> > --- /dev/null
> > +++ b/src/app/models/ip_v4_address.rb
> > @@ -0,0 +1,48 @@
> > +# ip_v4_address.rb
> > +#
> > +# Copyright (C) 2008 Red Hat, Inc.
> > +# Written by Darryl L. Pierce <dpierce at redhat.com>,
> > +#            Mohammed Morsi   <mmorsi at redhat.com>
> > +#
> > +# 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.
> > +
> > +# +IpV4Address+ represents a single IPv4 address.
> > +#
> > +class IpV4Address < IpAddress
> > +  ADDRESS_TEST = %r{^(\d{1,3}\.){3}\d{1,3}$}
> > +
> > +  validates_presence_of :address,
> > +    :message => 'An address must be supplied.'
> > +  validates_format_of :address,
> > +    :with => ADDRESS_TEST
> > +
> > +  protected
> > +   def validate
> > +    unless address and address =~ ADDRESS_TEST
> > +      errors.add("address", "is of incorrect format")
> > +    end
> > +    unless !netmask or netmask == "" or netmask =~ ADDRESS_TEST
> > +      errors.add("netmask", "is of incorrect format")
> > +    end
> > +    unless !gateway or gateway == "" or gateway =~ ADDRESS_TEST
> > +      errors.add("gateway", "is of incorrect format")
> > +    end
> > +    unless !broadcast or broadcast == "" or broadcast =~ ADDRESS_TEST
> > +      errors.add("broadcast", "is of incorrect format")
> > +    end
>
> In the original patch I submitted each of these fields was tested using
> validates_format_of.
Thing is, since ip address is now being used for network, bonding, and
nic, some of those fields won't apply in all cases. Specifically only
the 'address' field should be set for ip addresses associated with nics
/ bondings (setting the others won't break anything, they just won't be
used) and only when the nic is associated with a statically assigned
network. The other fields are only used when an ip address is associated
with a network, thus it doesn't work to validate them for every instance.

>
> > +   end
> > +
> > +end
> > diff --git a/src/app/models/ip_v6_address.rb
> b/src/app/models/ip_v6_address.rb
> > new file mode 100644
> > index 0000000..9720179
> > --- /dev/null
> > +++ b/src/app/models/ip_v6_address.rb
> > @@ -0,0 +1,44 @@
> > +# ip_v6_address.rb
> > +#
> > +# Copyright (C) 2008 Red Hat, Inc.
> > +# Written by Darryl L. Pierce <dpierce at redhat.com>,
> > +#            Mohammed Morsi   <mmorsi at redhat.com>
> > +#
> > +# 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.
> > +
> > +# +IpV6Address+ represents a single IPv6 address.
> > +#
> > +class IpV6Address < IpAddress
> > +  ADDRESS_TEST = %r{^([0-9a-fA-F]{0,4}|0)(\:([0-9a-fA-F]{0,4}|0)){7}$}
> > +
> > +  validates_presence_of :address,
> > +    :message => 'An address must be provided.'
> > +  validates_format_of :address,
> > +    :with => ADDRESS_TEST
> > +
> > +  protected
> > +   def validate
> > +    unless address =~ ADDRESS_TEST
> > +      errors.add("address", "is of incorrect format")
> > +    end
> > +    unless !prefix or prefix == "" or prefix =~ ADDRESS_TEST
> > +      errors.add("prefix", "is of incorrect format")
> > +    end
> > +    unless !gateway or gateway == "" or gateway =~ ADDRESS_TEST
> > +      errors.add("gateway", "is of incorrect format")
> > +    end
>
> See note in the IpV4Address regarding validations. The Rails pattern is
> to use validates_format_of for checking content.
It is also a valid rails approach to use the validate method for
validations.

[snip]

>
> > diff --git a/src/app/models/nic.rb b/src/app/models/nic.rb
> > index baf7095..25d3ac2 100644
> > --- a/src/app/models/nic.rb
> > +++ b/src/app/models/nic.rb
> > @@ -19,7 +19,19 @@
>
> >  class Nic < ActiveRecord::Base
> >    belongs_to :host
> > -  belongs_to :boot_type
> > +  belongs_to :physical_network
> > +  has_many :ip_addresses, :dependent => :destroy
>
> > -  has_and_belongs_to_many :bonding, :join_table => 'bondings_nics'
> > +  has_and_belongs_to_many :bondings, :join_table => 'bondings_nics'
> > +
> > +  protected
> > +   def validate
> > +     errors.add("host_id", "must be specified") unless host_id
> > +     errors.add("physical_network_id", "must be specified") unless
> physical_network_id
>
> These should be done with validates_presence_of to keep the code
> consistent throughout the project. The error message added here is the
> default from Rails, so need not be specified.
>
> > +     if physical_network.boot_type.id ==
> > +         BootType.find(:first, :conditions => { :proto => 'static'
> }).id and
>
> You can test this without the second DB call using:
>
> if physical_network.boot_type.proto == static'

Good catch on this boot_type bit.

[snip]

I'll see what I can change based on this feedback. Once again I'm not
sure that the validation bit will be able to be changed due to the
aforementioned reasons.

  -Mo