New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fog::Mode.reload behaviour has changed! #236

Closed
gildub opened this Issue Jun 18, 2018 · 28 comments

Comments

Projects
None yet
3 participants
@gildub
Contributor

gildub commented Jun 18, 2018

The following used to work with fog-core 1.40:

https://github.com/fog/fog-openstack/blob/master/spec/image_v2_spec.rb#L142
https://github.com/fog/fog-openstack/blob/master/spec/network_spec.rb#L40

The issue seems to be that new attributes are not merged properly.

One work around:

def reload
    requires :identity

    object = collection.get(identity)

    return unless object

    merge_attributes(object.attributes)
    merge_attributes(object.all_associations)

    self
    rescue Excon::Errors::SocketError
    nil
end

@gildub gildub changed the title from `Fog::Mode.reload` behaviour has changed! to Fog::Mode.reload behaviour has changed! Jun 18, 2018

@geemus

This comment has been minimized.

Show comment
Hide comment
@geemus

geemus Jun 18, 2018

Member

Hmm. I would expect it still to work, at least in that we haven't really seen other reports around this.

I believe that:

merge_attributes(object.all_associations_and_attributes)

Should be equivalent to the two merge actions listed there, so I'm not sure where the discrepancy is sneaking in. @plribeiro3000 any chance you see something I'm missing? Thanks!

Member

geemus commented Jun 18, 2018

Hmm. I would expect it still to work, at least in that we haven't really seen other reports around this.

I believe that:

merge_attributes(object.all_associations_and_attributes)

Should be equivalent to the two merge actions listed there, so I'm not sure where the discrepancy is sneaking in. @plribeiro3000 any chance you see something I'm missing? Thanks!

@plribeiro3000

This comment has been minimized.

Show comment
Hide comment
@plribeiro3000

plribeiro3000 Jun 18, 2018

Member

I would expect the same @geemus

For what i can see in the report, manually calling one after the other (attributes and all_associations) works but not the method the returns both (all_associations_and_attributes).

I will take a look and see what i can find.

Member

plribeiro3000 commented Jun 18, 2018

I would expect the same @geemus

For what i can see in the report, manually calling one after the other (attributes and all_associations) works but not the method the returns both (all_associations_and_attributes).

I will take a look and see what i can find.

@plribeiro3000

This comment has been minimized.

Show comment
Hide comment
@plribeiro3000

plribeiro3000 Jun 18, 2018

Member

all_associations_and_attributes just do a merge on both hashes:

def all_associations_and_attributes
  all_attributes.merge(all_associations)
end

And looking at the actual implementation of reload:

def reload
  requires :identity

  object = collection.get(identity)

  return unless object

  merge_attributes(object.all_associations_and_attributes)

  self
rescue Excon::Errors::SocketError
  nil
end

and the workaround by @gildub i believe it is related to how all_attributes and all_associations works.

def all_attributes
  self.class.attributes.reduce({}) do |hash, attribute|
    hash[masks[attribute]] = send(attribute)
    hash
  end
end


def all_associations
  self.class.associations.keys.reduce({}) do |hash, association|
    hash[masks[association]] = associations[association] || send(association)
    hash
  end
end

The just call the method definitions without any type of cache so i believe it can only be related to the method setter and getter.

If we look at the associations, they don't do any kind of cache neither conditional setting so i believe your problem is related to attributes and not associations.

Going further:

def merge_attributes(new_attributes = {})
  new_attributes.each_pair do |key, value|
    next if self.class.ignored_attributes.include?(key)
    if self.class.aliases[key]
      send("#{self.class.aliases[key]}=", value)
    elsif self.respond_to?("#{key}=", true)
      send("#{key}=", value)
    else
      attributes[key] = value
    end
  end
  self
end

merge_attributes does not do any kind of cache either and attributes is as simples as:

def attributes
  @attributes ||= {}
end

So i can't really see whats wrong here.

Looking at the first code line you referenced i've found this:

def method_missing(method_sym, *arguments, &block)
  if attributes.key?(method_sym)
    attributes[method_sym]
  elsif attributes.key?(method_sym.to_s)
    attributes[method_sym.to_s]
  elsif method_sym.to_s.end_with?('=')
    attributes[method_sym.to_s.gsub(/=$/, '')] = arguments[0]
  else
    super
  end
end

which suggests me you are saving some keys using a symbol an some using a string. Maybe thats related to your problem?

And looking at the second it seems the method being called before doing a reload is this one:

def create
  merge_attributes(service.create_network(attributes).body['network'])
  self
end

which might also be related to why there are some differences in the reload.

I know it was working on a previous version of fog-core but it would help if we could remove those methods and see if the error persists.

Member

plribeiro3000 commented Jun 18, 2018

all_associations_and_attributes just do a merge on both hashes:

def all_associations_and_attributes
  all_attributes.merge(all_associations)
end

And looking at the actual implementation of reload:

def reload
  requires :identity

  object = collection.get(identity)

  return unless object

  merge_attributes(object.all_associations_and_attributes)

  self
rescue Excon::Errors::SocketError
  nil
end

and the workaround by @gildub i believe it is related to how all_attributes and all_associations works.

def all_attributes
  self.class.attributes.reduce({}) do |hash, attribute|
    hash[masks[attribute]] = send(attribute)
    hash
  end
end


def all_associations
  self.class.associations.keys.reduce({}) do |hash, association|
    hash[masks[association]] = associations[association] || send(association)
    hash
  end
end

The just call the method definitions without any type of cache so i believe it can only be related to the method setter and getter.

If we look at the associations, they don't do any kind of cache neither conditional setting so i believe your problem is related to attributes and not associations.

Going further:

def merge_attributes(new_attributes = {})
  new_attributes.each_pair do |key, value|
    next if self.class.ignored_attributes.include?(key)
    if self.class.aliases[key]
      send("#{self.class.aliases[key]}=", value)
    elsif self.respond_to?("#{key}=", true)
      send("#{key}=", value)
    else
      attributes[key] = value
    end
  end
  self
end

merge_attributes does not do any kind of cache either and attributes is as simples as:

def attributes
  @attributes ||= {}
end

So i can't really see whats wrong here.

Looking at the first code line you referenced i've found this:

def method_missing(method_sym, *arguments, &block)
  if attributes.key?(method_sym)
    attributes[method_sym]
  elsif attributes.key?(method_sym.to_s)
    attributes[method_sym.to_s]
  elsif method_sym.to_s.end_with?('=')
    attributes[method_sym.to_s.gsub(/=$/, '')] = arguments[0]
  else
    super
  end
end

which suggests me you are saving some keys using a symbol an some using a string. Maybe thats related to your problem?

And looking at the second it seems the method being called before doing a reload is this one:

def create
  merge_attributes(service.create_network(attributes).body['network'])
  self
end

which might also be related to why there are some differences in the reload.

I know it was working on a previous version of fog-core but it would help if we could remove those methods and see if the error persists.

@gildub

This comment has been minimized.

Show comment
Hide comment
@gildub

gildub Jun 20, 2018

Contributor

@plribeiro3000, I tried without success to remove the methods you mentioned. More investigation will be needed.

Contributor

gildub commented Jun 20, 2018

@plribeiro3000, I tried without success to remove the methods you mentioned. More investigation will be needed.

@plribeiro3000

This comment has been minimized.

Show comment
Hide comment
@plribeiro3000

plribeiro3000 Jun 20, 2018

Member

Hmmm. Does it break just for this models of for every model?

Also, 1.40.0 is quite old. Can you test against 1.45.0 which is the latest from the 1.x series and against 2.0.0 too?

EDIT:

Looking at the commits i found the changes related to this change in fog-core are in this commit: 85cfeae

Which was released on fog-core 2.0.0.

Member

plribeiro3000 commented Jun 20, 2018

Hmmm. Does it break just for this models of for every model?

Also, 1.40.0 is quite old. Can you test against 1.45.0 which is the latest from the 1.x series and against 2.0.0 too?

EDIT:

Looking at the commits i found the changes related to this change in fog-core are in this commit: 85cfeae

Which was released on fog-core 2.0.0.

@gildub

This comment has been minimized.

Show comment
Hide comment
@gildub

gildub Jun 21, 2018

Contributor

@plribeiro3000, BTW thanks for taking the time to investigate ;)

It fails the exact same way with 2.0.0
It works fine with 1.45.0
I've reversed to latter for now.

Contributor

gildub commented Jun 21, 2018

@plribeiro3000, BTW thanks for taking the time to investigate ;)

It fails the exact same way with 2.0.0
It works fine with 1.45.0
I've reversed to latter for now.

@gildub

This comment has been minimized.

Show comment
Hide comment
@gildub

gildub Jun 21, 2018

Contributor

@plribeiro3000, it fails only for the two models mentioned.
85cfeae was effectively the one introducing the change, prior to that it works fine.

Contributor

gildub commented Jun 21, 2018

@plribeiro3000, it fails only for the two models mentioned.
85cfeae was effectively the one introducing the change, prior to that it works fine.

@plribeiro3000

This comment has been minimized.

Show comment
Hide comment
@plribeiro3000

plribeiro3000 Jul 4, 2018

Member

@gildub Since is just for this two models, i believe they are the culprit.

Since i don't have enough knowledge of fog-openstack could you figure out the main difference between both and the other models?

Member

plribeiro3000 commented Jul 4, 2018

@gildub Since is just for this two models, i believe they are the culprit.

Since i don't have enough knowledge of fog-openstack could you figure out the main difference between both and the other models?

@gildub

This comment has been minimized.

Show comment
Hide comment
@gildub

gildub Aug 20, 2018

Contributor

@plribeiro3000,

The "Image" model class is calling 'lib/fog/core/model.rb' directly, so there is nothing in between.

Actually I've found that https://github.com/fog/fog-core/blob/master/lib/fog/core/attributes.rb#L80 is not returning the attributes that have been created dynamically.

For a given instance of Image:

[3] pry(#<Fog::Image::OpenStack::V2::Image>)> object
=>   <Fog::Image::OpenStack::V2::Image
    id="bc3cfc5a-0c88-47b6-82c2-c6957c30d886",
    name="reloaded_image",
    visibility="private",
    tags=[],
    self="/v2/images/bc3cfc5a-0c88-47b6-82c2-c6957c30d886",
    size=nil,
    virtual_size=nil,
    disk_format=nil,
    container_format=nil,
    id="bc3cfc5a-0c88-47b6-82c2-c6957c30d886",
    checksum=nil,
    self="/v2/images/bc3cfc5a-0c88-47b6-82c2-c6957c30d886",
    file="/v2/images/bc3cfc5a-0c88-47b6-82c2-c6957c30d886/file",
    min_disk=0,
    created_at="2016-08-25T17:09:06Z",
    updated_at="2016-08-25T17:09:07Z",
    protected=false,
    status="queued",
    min_ram=0,
    owner="7b7688e5eecd4d6aa5b5b2cb3e93a778",
    properties=nil,
    metadata=nil,
    location=nil,
    network_allocated=nil,
    base_image_ref=nil,
    image_type=nil,
    instance_uuid=nil,
    user_id=nil
  >
[4] pry(#<Fog::Image::OpenStack::V2::Image>)> object.attributes
=> {:status=>"queued",
 :name=>"reloaded_image",
 :tags=>[],
 :container_format=>nil,
 :created_at=>"2016-08-25T17:09:06Z",
 :size=>nil,
 :disk_format=>nil,
 :locations=>[],
 :visibility=>"private",
 :updated_at=>"2016-08-25T17:09:07Z",
 :self=>"/v2/images/bc3cfc5a-0c88-47b6-82c2-c6957c30d886",
 :owner=>"7b7688e5eecd4d6aa5b5b2cb3e93a778",
 :protected=>false,
 :min_ram=>0,
 :file=>"/v2/images/bc3cfc5a-0c88-47b6-82c2-c6957c30d886/file",
 :checksum=>nil,
 :id=>"bc3cfc5a-0c88-47b6-82c2-c6957c30d886",
 :min_disk=>0,
 :virtual_size=>nil,
 :reloadable_property=>"updated",
 :schema=>"/v2/schemas/image"}

#all_attributes is missing fields such as :reloadable_property, :schema

[5] pry(#<Fog::Image::OpenStack::V2::Image>)> object.all_attributes
=> {:id=>"bc3cfc5a-0c88-47b6-82c2-c6957c30d886",
 :name=>"reloaded_image",
 :visibility=>"private",
 :tags=>[],
 :self=>"/v2/images/bc3cfc5a-0c88-47b6-82c2-c6957c30d886",
 :size=>nil,
 :virtual_size=>nil,
 :disk_format=>nil,
 :container_format=>nil,
 :checksum=>nil,
 :file=>"/v2/images/bc3cfc5a-0c88-47b6-82c2-c6957c30d886/file",
 :min_disk=>0,
 :created_at=>"2016-08-25T17:09:06Z",
 :updated_at=>"2016-08-25T17:09:07Z",
 :protected=>false,
 :status=>"queued",
 :min_ram=>0,
 :owner=>"7b7688e5eecd4d6aa5b5b2cb3e93a778",
 :properties=>nil,
 :metadata=>nil,
 :location=>nil,
 :network_allocated=>nil,
 :base_image_ref=>nil,
 :image_type=>nil,
 :instance_uuid=>nil,
 :user_id=>nil}

That is why https://github.com/fog/fog-core/blob/master/lib/fog/core/attributes.rb#L94 is not returning everything as it used to in fog-core 1.45.0

Contributor

gildub commented Aug 20, 2018

@plribeiro3000,

The "Image" model class is calling 'lib/fog/core/model.rb' directly, so there is nothing in between.

Actually I've found that https://github.com/fog/fog-core/blob/master/lib/fog/core/attributes.rb#L80 is not returning the attributes that have been created dynamically.

For a given instance of Image:

[3] pry(#<Fog::Image::OpenStack::V2::Image>)> object
=>   <Fog::Image::OpenStack::V2::Image
    id="bc3cfc5a-0c88-47b6-82c2-c6957c30d886",
    name="reloaded_image",
    visibility="private",
    tags=[],
    self="/v2/images/bc3cfc5a-0c88-47b6-82c2-c6957c30d886",
    size=nil,
    virtual_size=nil,
    disk_format=nil,
    container_format=nil,
    id="bc3cfc5a-0c88-47b6-82c2-c6957c30d886",
    checksum=nil,
    self="/v2/images/bc3cfc5a-0c88-47b6-82c2-c6957c30d886",
    file="/v2/images/bc3cfc5a-0c88-47b6-82c2-c6957c30d886/file",
    min_disk=0,
    created_at="2016-08-25T17:09:06Z",
    updated_at="2016-08-25T17:09:07Z",
    protected=false,
    status="queued",
    min_ram=0,
    owner="7b7688e5eecd4d6aa5b5b2cb3e93a778",
    properties=nil,
    metadata=nil,
    location=nil,
    network_allocated=nil,
    base_image_ref=nil,
    image_type=nil,
    instance_uuid=nil,
    user_id=nil
  >
[4] pry(#<Fog::Image::OpenStack::V2::Image>)> object.attributes
=> {:status=>"queued",
 :name=>"reloaded_image",
 :tags=>[],
 :container_format=>nil,
 :created_at=>"2016-08-25T17:09:06Z",
 :size=>nil,
 :disk_format=>nil,
 :locations=>[],
 :visibility=>"private",
 :updated_at=>"2016-08-25T17:09:07Z",
 :self=>"/v2/images/bc3cfc5a-0c88-47b6-82c2-c6957c30d886",
 :owner=>"7b7688e5eecd4d6aa5b5b2cb3e93a778",
 :protected=>false,
 :min_ram=>0,
 :file=>"/v2/images/bc3cfc5a-0c88-47b6-82c2-c6957c30d886/file",
 :checksum=>nil,
 :id=>"bc3cfc5a-0c88-47b6-82c2-c6957c30d886",
 :min_disk=>0,
 :virtual_size=>nil,
 :reloadable_property=>"updated",
 :schema=>"/v2/schemas/image"}

#all_attributes is missing fields such as :reloadable_property, :schema

[5] pry(#<Fog::Image::OpenStack::V2::Image>)> object.all_attributes
=> {:id=>"bc3cfc5a-0c88-47b6-82c2-c6957c30d886",
 :name=>"reloaded_image",
 :visibility=>"private",
 :tags=>[],
 :self=>"/v2/images/bc3cfc5a-0c88-47b6-82c2-c6957c30d886",
 :size=>nil,
 :virtual_size=>nil,
 :disk_format=>nil,
 :container_format=>nil,
 :checksum=>nil,
 :file=>"/v2/images/bc3cfc5a-0c88-47b6-82c2-c6957c30d886/file",
 :min_disk=>0,
 :created_at=>"2016-08-25T17:09:06Z",
 :updated_at=>"2016-08-25T17:09:07Z",
 :protected=>false,
 :status=>"queued",
 :min_ram=>0,
 :owner=>"7b7688e5eecd4d6aa5b5b2cb3e93a778",
 :properties=>nil,
 :metadata=>nil,
 :location=>nil,
 :network_allocated=>nil,
 :base_image_ref=>nil,
 :image_type=>nil,
 :instance_uuid=>nil,
 :user_id=>nil}

That is why https://github.com/fog/fog-core/blob/master/lib/fog/core/attributes.rb#L94 is not returning everything as it used to in fog-core 1.45.0

@plribeiro3000

This comment has been minimized.

Show comment
Hide comment
@plribeiro3000

plribeiro3000 Aug 20, 2018

Member

Hmm. We are getting somewhere now.

So the problem isn't necessarily with #all_attributes but with the hash being different of the original one.

Can we get the api result for the last call on #all_attributes?

I wonder if the data being result is exactly like the one you had previously.

Member

plribeiro3000 commented Aug 20, 2018

Hmm. We are getting somewhere now.

So the problem isn't necessarily with #all_attributes but with the hash being different of the original one.

Can we get the api result for the last call on #all_attributes?

I wonder if the data being result is exactly like the one you had previously.

@plribeiro3000

This comment has been minimized.

Show comment
Hide comment
@plribeiro3000

plribeiro3000 Aug 20, 2018

Member

Actually, i just notice something. You don't have reloadable_property neither schema defined as attributes here

Member

plribeiro3000 commented Aug 20, 2018

Actually, i just notice something. You don't have reloadable_property neither schema defined as attributes here

@plribeiro3000

This comment has been minimized.

Show comment
Hide comment
@plribeiro3000

plribeiro3000 Aug 20, 2018

Member

And you are setting reloadable_property manually here, thus why you have different hashes.

Member

plribeiro3000 commented Aug 20, 2018

And you are setting reloadable_property manually here, thus why you have different hashes.

@plribeiro3000

This comment has been minimized.

Show comment
Hide comment
@plribeiro3000

plribeiro3000 Aug 20, 2018

Member

It all makes sense now. Your test needs a fix or either the model needs some attributes defined.

Member

plribeiro3000 commented Aug 20, 2018

It all makes sense now. Your test needs a fix or either the model needs some attributes defined.

@gildub

This comment has been minimized.

Show comment
Hide comment
@gildub

gildub Aug 20, 2018

Contributor

@plribeiro3000, yes some of those attributes are created dynamically and maybe for the purpose of testing but I'm not 100% sure if it's the only case (I didn't implement it).

So from that my question is: Do 'attributes' have to be declared as such?

Because before fog-core 2.0.0 it wasn't necessary, as we can see in this code:
https://github.com/fog/fog-openstack/blob/236d5667040903bb12118c89a4d78e65c6c5878f/lib/fog/image/openstack/v2/models/image.rb#L50

Contributor

gildub commented Aug 20, 2018

@plribeiro3000, yes some of those attributes are created dynamically and maybe for the purpose of testing but I'm not 100% sure if it's the only case (I didn't implement it).

So from that my question is: Do 'attributes' have to be declared as such?

Because before fog-core 2.0.0 it wasn't necessary, as we can see in this code:
https://github.com/fog/fog-openstack/blob/236d5667040903bb12118c89a4d78e65c6c5878f/lib/fog/image/openstack/v2/models/image.rb#L50

@plribeiro3000

This comment has been minimized.

Show comment
Hide comment
@plribeiro3000

plribeiro3000 Aug 21, 2018

Member

Yes. Every attribute should be declared using the DSL.

I'm not sure why it was working previously and does not work now but i believe it is related.

If we do go over the attribute DSL:

def attribute(name, options = {})
  type = options.fetch(:type, "default").to_s.capitalize
  Fog::Attributes.const_get(type).new(self, name, options)
end

It does more then just define a setter. You can typecast its value like:

module Fog
  module Attributes
    # = Fog Timestamp Attribute
    #
    # This class handles Timestamp attributes from the providers,
    # converting Integer and String values as a real Timestamp objects
    class Timestamp < Default
      def create_setter
        model.class_eval <<-EOS, __FILE__, __LINE__
            def #{name}=(new_#{name})
              if new_#{name}.respond_to?(:to_i)
                attributes[:#{name}] = Fog::Time.at(new_#{name}.to_i)
              else
                attributes[:#{name}] = Fog::Time.parse(new_#{name}.to_s)
              end
            end
        EOS
      end
    end
  end
end
Member

plribeiro3000 commented Aug 21, 2018

Yes. Every attribute should be declared using the DSL.

I'm not sure why it was working previously and does not work now but i believe it is related.

If we do go over the attribute DSL:

def attribute(name, options = {})
  type = options.fetch(:type, "default").to_s.capitalize
  Fog::Attributes.const_get(type).new(self, name, options)
end

It does more then just define a setter. You can typecast its value like:

module Fog
  module Attributes
    # = Fog Timestamp Attribute
    #
    # This class handles Timestamp attributes from the providers,
    # converting Integer and String values as a real Timestamp objects
    class Timestamp < Default
      def create_setter
        model.class_eval <<-EOS, __FILE__, __LINE__
            def #{name}=(new_#{name})
              if new_#{name}.respond_to?(:to_i)
                attributes[:#{name}] = Fog::Time.at(new_#{name}.to_i)
              else
                attributes[:#{name}] = Fog::Time.parse(new_#{name}.to_s)
              end
            end
        EOS
      end
    end
  end
end
@plribeiro3000

This comment has been minimized.

Show comment
Hide comment
@plribeiro3000

plribeiro3000 Aug 21, 2018

Member

Ok. I got it now.

The problem is indeed because the attribute is not defined using the DSL.

With the inclusion of the mask feature, we did break it for providers that are not using the DSL.

See:

module Fog
  module Attributes
    # = Fog Default Attribute
    #
    # This class handles the attributes without a type force.
    # The attributes returned from the provider will keep its original values.
    class Default
      attr_reader :model, :name, :squash, :aliases, :default, :as

      def initialize(model, name, options)
        .
        .
        .
        @as = options.fetch(:as, name)
        .
        .
        .
        .
        create_mask
      end

      def create_mask
        model.masks[name] = as
      end
    end
  end
end

So when no mask is defined using the as option, the mask is defaulted to the attribute itself.

Then on Fog::Model it does use the masks without checking if it does exists because it does believe all attributes were defined using the DSL:

def all_attributes
  self.class.attributes.reduce({}) do |hash, attribute|
    hash[masks[attribute]] = send(attribute)
    hash
  end
end


def all_associations
  self.class.associations.keys.reduce({}) do |hash, association|
    hash[masks[association]] = associations[association] || send(association)
    hash
  end
end

So i believe we have 2 options here:

  • Check the existence of the mask prior to use it so attributes not defined using the DSL will work
  • Define all attributes using the DSL

The first option is the easiest one but it does not help in the long term where we will have providers not using the DSL because we did leave it loose.

Member

plribeiro3000 commented Aug 21, 2018

Ok. I got it now.

The problem is indeed because the attribute is not defined using the DSL.

With the inclusion of the mask feature, we did break it for providers that are not using the DSL.

See:

module Fog
  module Attributes
    # = Fog Default Attribute
    #
    # This class handles the attributes without a type force.
    # The attributes returned from the provider will keep its original values.
    class Default
      attr_reader :model, :name, :squash, :aliases, :default, :as

      def initialize(model, name, options)
        .
        .
        .
        @as = options.fetch(:as, name)
        .
        .
        .
        .
        create_mask
      end

      def create_mask
        model.masks[name] = as
      end
    end
  end
end

So when no mask is defined using the as option, the mask is defaulted to the attribute itself.

Then on Fog::Model it does use the masks without checking if it does exists because it does believe all attributes were defined using the DSL:

def all_attributes
  self.class.attributes.reduce({}) do |hash, attribute|
    hash[masks[attribute]] = send(attribute)
    hash
  end
end


def all_associations
  self.class.associations.keys.reduce({}) do |hash, association|
    hash[masks[association]] = associations[association] || send(association)
    hash
  end
end

So i believe we have 2 options here:

  • Check the existence of the mask prior to use it so attributes not defined using the DSL will work
  • Define all attributes using the DSL

The first option is the easiest one but it does not help in the long term where we will have providers not using the DSL because we did leave it loose.

@plribeiro3000

This comment has been minimized.

Show comment
Hide comment
@plribeiro3000

plribeiro3000 Aug 21, 2018

Member

Hmmmm, another options would be to add the fix on fog-core and add a deprecation notice in case it happens. That would be an easy fix for now with a notice for every provider not using the DSL to change it.

What do you guys think?

@geemus @gildub

Member

plribeiro3000 commented Aug 21, 2018

Hmmmm, another options would be to add the fix on fog-core and add a deprecation notice in case it happens. That would be an easy fix for now with a notice for every provider not using the DSL to change it.

What do you guys think?

@geemus @gildub

@gildub

This comment has been minimized.

Show comment
Hide comment
@gildub

gildub Aug 21, 2018

Contributor

@plribeiro3000,

Thanks for the thorough investigation.

Out of the 2 cases breaking Rspec tests:

  • I fixed the image service [1] by removing the dynamic attribute (not class defined) which was used only for the purpose of the test. See [2] for more details.
  • For the network service [3], I haven't found a better solution other than monkey patch .reload.

Beyond that the question remains for the end users using reload so I believe the safest is to provide a deprecation message.

[1] https://github.com/fog/fog-openstack/blob/master/spec/image_v2_spec.rb#L142
[2] fog/fog-openstack#415
[3] https://github.com/fog/fog-openstack/blob/236d5667040903bb12118c89a4d78e65c6c5878f/spec/network_spec.rb#L56

Contributor

gildub commented Aug 21, 2018

@plribeiro3000,

Thanks for the thorough investigation.

Out of the 2 cases breaking Rspec tests:

  • I fixed the image service [1] by removing the dynamic attribute (not class defined) which was used only for the purpose of the test. See [2] for more details.
  • For the network service [3], I haven't found a better solution other than monkey patch .reload.

Beyond that the question remains for the end users using reload so I believe the safest is to provide a deprecation message.

[1] https://github.com/fog/fog-openstack/blob/master/spec/image_v2_spec.rb#L142
[2] fog/fog-openstack#415
[3] https://github.com/fog/fog-openstack/blob/236d5667040903bb12118c89a4d78e65c6c5878f/spec/network_spec.rb#L56

@geemus

This comment has been minimized.

Show comment
Hide comment
@geemus

geemus Aug 22, 2018

Member

@plribeiro3000 fixing with deprecation sounds reasonable, I think. I'm not totally sure I understand when it would occur and/or what it would look like. I think a more concrete example might help drive it home for me, could you give a particular example of what it might look like? Thanks!

Member

geemus commented Aug 22, 2018

@plribeiro3000 fixing with deprecation sounds reasonable, I think. I'm not totally sure I understand when it would occur and/or what it would look like. I think a more concrete example might help drive it home for me, could you give a particular example of what it might look like? Thanks!

@plribeiro3000

This comment has been minimized.

Show comment
Hide comment
@plribeiro3000

plribeiro3000 Aug 22, 2018

Member

Something like:

def all_attributes
  self.class.attributes.reduce({}) do |hash, attribute|
    if masks[attribute].nil?
      Fog::Logger.deprecation("Please define #{attribute} using the Fog DSL")
      hash[attribute] = send(attribute)
    else
      hash[masks[attribute]] = send(attribute)
    end

    hash
  end
end


def all_associations
  self.class.associations.keys.reduce({}) do |hash, association|
    if masks[attribute].nil?
      Fog::Logger.deprecation("Please define #{attribute} using the Fog DSL")
      hash[association] = associations[association] || send(association)
    else
      hash[masks[association]] = associations[association] || send(association)
    end
    
    hash
  end
end
Member

plribeiro3000 commented Aug 22, 2018

Something like:

def all_attributes
  self.class.attributes.reduce({}) do |hash, attribute|
    if masks[attribute].nil?
      Fog::Logger.deprecation("Please define #{attribute} using the Fog DSL")
      hash[attribute] = send(attribute)
    else
      hash[masks[attribute]] = send(attribute)
    end

    hash
  end
end


def all_associations
  self.class.associations.keys.reduce({}) do |hash, association|
    if masks[attribute].nil?
      Fog::Logger.deprecation("Please define #{attribute} using the Fog DSL")
      hash[association] = associations[association] || send(association)
    else
      hash[masks[association]] = associations[association] || send(association)
    end
    
    hash
  end
end
@geemus

This comment has been minimized.

Show comment
Hide comment
@geemus

geemus Aug 22, 2018

Member

@plribeiro3000 got it, that clarifies a ton, thanks! Sounds good to me. Thanks for talking through this.

Member

geemus commented Aug 22, 2018

@plribeiro3000 got it, that clarifies a ton, thanks! Sounds good to me. Thanks for talking through this.

@gildub

This comment has been minimized.

Show comment
Hide comment
@gildub

gildub Aug 28, 2018

Contributor

@plribeiro3000, that's great. Thanks

Contributor

gildub commented Aug 28, 2018

@plribeiro3000, that's great. Thanks

@plribeiro3000

This comment has been minimized.

Show comment
Hide comment
@plribeiro3000

plribeiro3000 Sep 3, 2018

Member

@geemus Could you release a fix please?

Member

plribeiro3000 commented Sep 3, 2018

@geemus Could you release a fix please?

@geemus

This comment has been minimized.

Show comment
Hide comment
@geemus

geemus Sep 4, 2018

Member

released as v2.1.1

Member

geemus commented Sep 4, 2018

released as v2.1.1

@gildub

This comment has been minimized.

Show comment
Hide comment
@gildub

gildub Sep 7, 2018

Contributor

@plribeiro3000, could you please re-open this issue, unfortunately the behavior still breaks fog-openstack.

This is still happening in line:
https://github.com/fog/fog-openstack/blob/master/spec/network_spec.rb#L56

 :router_external=>false,
 "availability_zone_hints"=>[],
 "availability_zones"=>[],
 "description"=>"",
 :provider_physical_network=>nil,
 :subnets=>[],
 :shared=>false,
 :name=>"foo-net23",
 "created_at"=>"2016-05-11T12:05:18",
 "tags"=>[],
 "ipv6_address_scope"=>nil,
 :provider_network_type=>"vxlan",
 "updated_at"=>"2016-05-11T12:05:18",
 :tenant_id=>"dc4b3b173ee8467e9e2ae99e5c321d0c",
 :admin_state_up=>true,
 "ipv4_address_scope"=>nil,
 "port_security_enabled"=>true,
 "mtu"=>1450,
 :id=>"9bd99e12-499e-4f66-80c8-3239d03849bc",
 :provider_segmentation_id=>1087}
Contributor

gildub commented Sep 7, 2018

@plribeiro3000, could you please re-open this issue, unfortunately the behavior still breaks fog-openstack.

This is still happening in line:
https://github.com/fog/fog-openstack/blob/master/spec/network_spec.rb#L56

 :router_external=>false,
 "availability_zone_hints"=>[],
 "availability_zones"=>[],
 "description"=>"",
 :provider_physical_network=>nil,
 :subnets=>[],
 :shared=>false,
 :name=>"foo-net23",
 "created_at"=>"2016-05-11T12:05:18",
 "tags"=>[],
 "ipv6_address_scope"=>nil,
 :provider_network_type=>"vxlan",
 "updated_at"=>"2016-05-11T12:05:18",
 :tenant_id=>"dc4b3b173ee8467e9e2ae99e5c321d0c",
 :admin_state_up=>true,
 "ipv4_address_scope"=>nil,
 "port_security_enabled"=>true,
 "mtu"=>1450,
 :id=>"9bd99e12-499e-4f66-80c8-3239d03849bc",
 :provider_segmentation_id=>1087}

@plribeiro3000 plribeiro3000 reopened this Sep 10, 2018

@plribeiro3000

This comment has been minimized.

Show comment
Hide comment
@plribeiro3000

plribeiro3000 Sep 10, 2018

Member

Ok. I think this sounds like another misuse of the DSL.

You are defining the attribute subnets here:

attribute :subnets

But then you are overwriting the getter defined by the DSL here:

def subnets
  service.subnets.select { |s| s.network_id == id }
end

Since all_attributes is doing a send here:

def all_attributes
  self.class.attributes.reduce({}) do |hash, attribute|
    if masks[attribute].nil?
      Fog::Logger.deprecation("Please define #{attribute} using the Fog DSL")
      hash[attribute] = send(attribute)
    else
      hash[masks[attribute]] = send(attribute)
    end


    hash
  end
end

it will trigger the GET request.

Since it used to work before we can treat this as a bug but i don't believe is the same bug.

Shall we open a new issue for this?

Member

plribeiro3000 commented Sep 10, 2018

Ok. I think this sounds like another misuse of the DSL.

You are defining the attribute subnets here:

attribute :subnets

But then you are overwriting the getter defined by the DSL here:

def subnets
  service.subnets.select { |s| s.network_id == id }
end

Since all_attributes is doing a send here:

def all_attributes
  self.class.attributes.reduce({}) do |hash, attribute|
    if masks[attribute].nil?
      Fog::Logger.deprecation("Please define #{attribute} using the Fog DSL")
      hash[attribute] = send(attribute)
    else
      hash[masks[attribute]] = send(attribute)
    end


    hash
  end
end

it will trigger the GET request.

Since it used to work before we can treat this as a bug but i don't believe is the same bug.

Shall we open a new issue for this?

@gildub

This comment has been minimized.

Show comment
Hide comment
@gildub

gildub Sep 13, 2018

Contributor

@plribeiro3000, thanks for looking into this legacy code. I've created #244

Contributor

gildub commented Sep 13, 2018

@plribeiro3000, thanks for looking into this legacy code. I've created #244

@plribeiro3000

This comment has been minimized.

Show comment
Hide comment
@plribeiro3000
Member

plribeiro3000 commented Sep 13, 2018

@gildub 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment