Skip to content
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

Remove serialization of non-existing classes/objects #390

Merged

Conversation

jrafanie
Copy link
Member

@jrafanie jrafanie commented Jul 9, 2019

Rails 5.0.0 - 5.0.6 serialized internal classes that don't exist in rails
5.1+.

For example, ActiveModel::Type::Text was moved to ActiveRecord so
anything that serialized the raw_attributes, which contain lots of
bloated information, and this class was in the raw_attributes, we would
be unable to deserialize it using rails 5.1+.

Additionally, if the notificiation has a subject in the options column
that no longer exists, therefore we have to also remove these.

See also:
rails/rails#25145

@miq-bot miq-bot added the wip label Jul 9, 2019
@jrafanie jrafanie force-pushed the fix_unserializable_notification_options branch 4 times, most recently from 47e4e0f to 5093aa6 Compare July 10, 2019 19:32
@carbonin
Copy link
Member

Is this solving two distinct issues or is there some relation between the serialization issue and the subject not existing?

@jrafanie
Copy link
Member Author

Is this solving two distinct issues or is there some relation between the serialization issue and the subject not existing?

Yes, there are two distinct issues. In the database I had...

  • A few rows with serialized classes in the options column that fail to deserialize because they no longer exists in rails 5.1
  • A few rows with both a subject association and a options[:subject] containing a serialized object (Vm, CloudVolume, etc.) that doesn't exist anymore (the record was removed). I need to open a PR to fix the notification model / notification UI code to handle this record not existing, and figure out why we're serializing objects in options[:subject] instead of just a name to be used in the notification.... but fixing this in the database seemed appropriate... what do you think?

@jrafanie jrafanie force-pushed the fix_unserializable_notification_options branch from 5093aa6 to 06ff77d Compare July 16, 2019 20:31
@jrafanie
Copy link
Member Author

Ok, @bdunne @carbonin and I discussed this:

  • 2 rows from several hundreds were affected in an internal testing database
  • we normally purge notifications older than 1 week (starting with hammer-1 / 5.10) here
  • rails 5.0.0-5.0.6 releases were only in gaprindashvili 7 and older (< 5.9.9.0)

It seems best to just clear these bad options columns instead of trying to instantiate and possibly fix them. So, now we just mark them as "Unreadable" or Removed".

@jrafanie jrafanie changed the title [WIP] Remove serialization of non-existing classes/objects Remove serialization of non-existing classes/objects Jul 16, 2019
@jrafanie jrafanie removed the wip label Jul 16, 2019
@jrafanie jrafanie changed the title Remove serialization of non-existing classes/objects [WIP] Remove serialization of non-existing classes/objects Jul 16, 2019
@jrafanie jrafanie added the wip label Jul 16, 2019
@jrafanie jrafanie force-pushed the fix_unserializable_notification_options branch from 06ff77d to 115c90a Compare July 16, 2019 21:42
@jrafanie jrafanie removed the wip label Jul 16, 2019
@jrafanie jrafanie changed the title [WIP] Remove serialization of non-existing classes/objects Remove serialization of non-existing classes/objects Jul 16, 2019
@jrafanie
Copy link
Member Author

Ok, @carbonin @bdunne ready for your 🍅

@jrafanie jrafanie added the bug label Jul 16, 2019
@jrafanie jrafanie force-pushed the fix_unserializable_notification_options branch from 115c90a to 8c6041b Compare July 17, 2019 14:15
Rails 5.0.0 - 5.0.6 serialized internal classes that don't exist in rails
5.1+.

For example, ActiveModel::Type::Text was moved to ActiveRecord so
anything that serialized the raw_attributes, which contain lots of
bloated information, and this class was in the raw_attributes, we would
be unable to deserialize it using rails 5.1+.

Additionally, if the notificiation has a subject in the options column
that no longer exists, therefore we have to also remove these.

See also:
rails/rails#25145
@jrafanie jrafanie force-pushed the fix_unserializable_notification_options branch from 8c6041b to 217f5c9 Compare July 17, 2019 14:39
@miq-bot
Copy link
Member

miq-bot commented Jul 17, 2019

Checked commit jrafanie@217f5c9 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 9 offenses detected

db/migrate/20190708192323_fix_unserializable_notification_options.rb

spec/migrations/20190708192323_fix_unserializable_notification_options_spec.rb

Copy link
Member

@carbonin carbonin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@carbonin carbonin self-assigned this Jul 17, 2019
@carbonin carbonin merged commit 393a342 into ManageIQ:master Jul 17, 2019
@carbonin carbonin added this to the Sprint 116 Ending Jul 22, 2019 milestone Jul 17, 2019
@carbonin
Copy link
Member

@jrafanie were you able to track down where we were creating a notification with an AR object in the options? It doesn't really matter for this PR, but I'm curious.

@jrafanie
Copy link
Member Author

@carbonin first one is from here (serialized classes that no longer exist): https://github.com/ManageIQ/manageiq-providers-openstack/blob/ac0b2d4283eb1e0ee98a03949416574674f16d6b/app/models/manageiq/providers/openstack/cloud_manager/vm/operations.rb#L13

second one is here (options[:subject] was removed):
https://github.com/ManageIQ/manageiq-providers-openstack/blob/ac0b2d4283eb1e0ee98a03949416574674f16d6b/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume.rb#L70

It's still a problem.

It looks like it was added here and has never been changed.

Note, the first notification (vm_destroy_success) has a subject (vm). the second one (cloud_volume_delete_success), doesn't. Note, I don't see them setting the subject association in most places.

Does anyone work on openstack to fix this? I'm not sure if they're using options[:subject] at all. It would be great to just use the association and not store the options[:subject => self.

02:19:11 ~/Code/manageiq (master) (2.5.5) - grep -r -A 3 with_notification /Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_tenant.rb:    with_notification(:cloud_tenant_delete, :options => {:subject => self}) do
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_tenant.rb-      ext_management_system.with_provider_connection(connection_options) do |service|
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_tenant.rb-        service.delete_tenant(ems_ref)
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_tenant.rb-      end
--
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume/operations.rb:    with_notification(:cloud_volume_attach,
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume/operations.rb-                      :options => {
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume/operations.rb-                        :subject =>       self,
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume/operations.rb-                        :instance_name => server_ems_ref,
--
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume/operations.rb:    with_notification(:cloud_volume_detach,
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume/operations.rb-                      :options => {
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume/operations.rb-                        :subject =>       self,
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume/operations.rb-                        :instance_name => server_ems_ref,
--
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume.rb:    with_notification(:cloud_volume_create,
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume.rb-                      :options => {
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume.rb-                        :volume_name => options[:name],
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume.rb-                      }) do
--
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume.rb:    with_notification(:cloud_volume_update,
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume.rb-                      :options => {
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume.rb-                        :subject => self,
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume.rb-                      }) do
--
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume.rb:    with_notification(:cloud_volume_delete,
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume.rb-                      :options => {
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume.rb-                        :subject => self,
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume.rb-                      }) do
--
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume_snapshot.rb:    with_notification(:cloud_volume_snapshot_create,
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume_snapshot.rb-                      :options => {
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume_snapshot.rb-                        :snapshot_name => options[:name],
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume_snapshot.rb-                        :volume_name   => cloud_volume.name,
--
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume_snapshot.rb:    with_notification(:cloud_volume_snapshot_delete,
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume_snapshot.rb-                      :options => {
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume_snapshot.rb-                        :subject       => self,
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/cloud_volume_snapshot.rb-                        :volume_name   => cloud_volume.name,
--
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/vm/operations.rb:    with_notification(:vm_destroy,
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/vm/operations.rb-                      :options => {
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/vm/operations.rb-                        :subject => self,
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/cloud_manager/vm/operations.rb-                      }) do
--
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/helper_methods.rb:  def with_notification(type, options: {}, &block)
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/helper_methods.rb:    self.class.with_notification(type, :options => options, &block)
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/helper_methods.rb-  end
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/helper_methods.rb-
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/helper_methods.rb-  module ClassMethods
--
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/helper_methods.rb:    def with_notification(type, options: {})
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/helper_methods.rb-      # extract success and error options from options
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/helper_methods.rb-      # :success and :error keys respectively
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/helper_methods.rb-      # with all other keys common for both cases
--
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/network_manager/cloud_network.rb:    with_notification(:cloud_network_delete,
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/network_manager/cloud_network.rb-                      :options => {
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/network_manager/cloud_network.rb-                        :subject => self,
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/network_manager/cloud_network.rb-                      }) do
--
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/network_manager/cloud_subnet.rb:    with_notification(:cloud_subnet_delete,
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/network_manager/cloud_subnet.rb-                      :options => {
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/network_manager/cloud_subnet.rb-                        :subject => self,
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/network_manager/cloud_subnet.rb-                      }) do
--
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/network_manager/network_router.rb:    with_notification(:network_router_delete,
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/network_manager/network_router.rb-                      :options => {
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/network_manager/network_router.rb-                        :subject => self,
/Users/joerafaniello/.gem/ruby/2.5.5/bundler/gems/manageiq-providers-openstack-ac0b2d4283eb/app/models/manageiq/providers/openstack/network_manager/network_router.rb-                      }) do

@jrafanie jrafanie deleted the fix_unserializable_notification_options branch July 17, 2019 18:39
@carbonin
Copy link
Member

@agrare do you know who we can ping about ☝️ ?

@jrafanie
Copy link
Member Author

@agrare to summarize: We have a subject association and a serialized options[:subject] and using the latter caused a data integrity problem... so we'd like to use the subject association

@agrare
Copy link
Member

agrare commented Jul 18, 2019

ping @aufi ^^

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

Successfully merging this pull request may close these issues.

6 participants