Problem with Extended Model

[rc9000]

I wanted to make an extended ios class that can add some more command output, e.g.:

admin@myvm:~/oxidized $ cat model/iosplus.rb
require 'oxidized/model/ios.rb'
class IOSPlus < IOS
    cmd 'show lldp neighbors' do |cfg|
        comment cfg
    end
end

And I would like to selectively apply this to only certain devices:

admin@myvm:~/oxidized $ cat router.db
swx032.example.org:ios
swx036.example.org:ios
swvce1.example.org:iosplus

At first, it seemed to work great for the swvce1 device:

admin@myvm:~/oxidized $ cat configs/swvce1.example.org | tail 
 transport http
!
wsma profile listener httpslistener
 transport https
!
end
!     (R) Router, (B) Bridge, (T) Telephone, (C) DOCSIS Cable Device
!     (W) WLAN Access Point, (P) Repeater, (S) Station, (O) Other
!
! Device ID           Local Intf     Hold-time  Capability      Port ID
! swx001.nTe2/0/12       120        B               Te2/1/3
! swx001.nTe1/1/4        120        B               Te1/1/3
! swx003.nGi1/1/2        120        B               Gi1/1/1

But unfortunately, it is now also included in my output of devices that should use the plain ios model:

admin@myvm:~/oxidized $ cat configs/swx036.example.org | tail 
ntp access-group peer 5
ntp server 131.102.36.15 key 5
ntp server 131.102.136.15 key 5 prefer
!
end
!     (R) Router, (B) Bridge, (T) Telephone, (C) DOCSIS Cable Device
!     (W) WLAN Access Point, (P) Repeater, (S) Station, (O) Other
!
! Device ID           Local Intf     Hold-time  Capability      Port ID
! swvce3.nTe2/1/3        120        B,R             Te3/1/3
! swvce3.nTe1/1/3        120        B,R             Te2/1/3
!
! Total entries displayed: 2

And I can't figure out why. When logging OX_NODE_MODEL, it seems correct:

admin@myvm:~/oxidized $ cat /tmp/ox_node_success.log
Node success swvce1.example.org success g:  m: IOSPlus
Node success swx032.example.org success g:  m: IOS
Node success swx036.example.org success g:  m: IOS

This is the full config:

admin@myvm:~/oxidized $ cat config 
---
username: admin
password: topsecret
resolve_dns: true
interval: 3600
use_syslog: false
debug: false
threads: 30
use_max_threads: true
timeout: 20
retries: 3
rest: 127.0.0.1:8888
next_adds_job: false
vars: {}
groups: {}
models: {}
pid: "/home/admin/.config/oxidized/pid"
crash:
  directory: "/home/admin/.config/oxidized/crashes"
  hostnames: false
stats:
  history_size: 10
input:
  default: ssh
  debug: false
  ssh:
    secure: false
  ftp:
    passive: true
  utf8_encoded: true
output:
  default: file
  file:
    directory: "/home/admin/oxidized/configs"
source:
  default: csv
  csv:
    file: "/home/admin/oxidized/router.db"
    delimiter: !ruby/regexp /:/
    map:
      name: 0
      model: 1
    gpg: false
model_map:
hooks:
  example_hook1:
    type: exec
    events: [node_success]
    cmd: 'echo "Node success $OX_NODE_NAME $OX_JOB_STATUS g: $OX_NODE_GROUP m: $OX_NODE_MODEL" >> /tmp/ox_node_success.log'

I have started using oxidized this afternoon and I last touched Ruby when Rails was the hot new tech on the dotcom block, so most likely this is not a bug but me doing something wrong. But can you spot what? Thanks a bunch!

[ytti]

This is not intended behaviour, and I can't immediately see from manager.rb and node.rb how this would transpire, but also it seems plausible you're observing real problem and that we're somehow overwriting also the parent class, I just don't understand how.

The most probable explanation is in model/model.rb we are manipulating the same @cmd object, and we would have needed to reassign with @cmd.dup, to avoid changing the parents @cmd object.

Problem is most likely to exist here:
https://github.com/ytti/oxidized/blob/master/lib/oxidized/model/model.rb#L17-L20

Changing this to value = instance_variable_get(var); instance_variable_set var, value.dup or just instance_variable_set var, instance_variable_get(var).dup might do the trick.

[rc9000]

Thanks @ytti, after some trial and error it looks like beside @cmd also @cmd[:cmd] needs to be dup'ed. I tried myself at a PR which might be a bit clumsy, sorry about that.

[ytti]

Thanks @ytti, after some trial and error it looks like beside @cmd also @cmd[:cmd] needs to be dup'ed. I tried myself at a PR which might be a bit clumsy, sorry about that.

Good work. I'm not against the solution you ended up with but not necessarily what I would have chosen as I'm not sure if we have same problem elsewhere there. In most modern OO languages copies are shallow, because there is no obvious cheap way to do deep copies. #copy makes shallow copy, which meant, while @cmd is new, its keys are still pointing to same objects are originals.

The solution I would have likely gone for, would have been to indiscriminately create deep copies of each of the parents variables. For example: newvar = Marshal.load(Marshal.dump(oldvar)). I'm not 100% sure if our variables contain only marshable objects or not, but it will complain if not. This causes the oldvar to be serialised (for example storing into the disk) and then we deserialize it back into object, this will cause the copy to be deep. It is of course far more expensive than shallow copy, but in our case cost is immaterial and if it works, we can be confident we've fixed the same problem for all cases, without having to think about what the cases might be.

[rc9000]

I tried dump/load also but it's not possible to (un)marshal Proc code blocks, so it doesn't work on @cmd.

I was also a bit worried about impacting other functionality. My gut feeling is that more stuff would need to be duped, but the @cmd[:cmd] was the minimum that made my test work so I left it at that.

Since apparently in many years of oxidized nobody wanted to extend a class like I did, I'm also not mad if you don't apply the PR. It just looked like it should work in OO typically, but if it doesn't it's no big deal and I will just duplicate the whole ios.rb. Maybe we could just use touch up the doc then since it mentions

https://github.com/ytti/oxidized/blob/master/docs/Creating-Models.md
Create a named variation of an existing model, by creating a file with a new name (such as ~/.config/oxidized/model/junos-extra.rb), Then require the original model and extend its base class as in the above example. The named variation can then be specified as an OS type for specific devices that can benefit from the extra functionality. This allows for preservation of the base functionality for the default model types.

which is how I encountered the problem in the first place :)

[ytti]

I think this is definitely legitimate problem that needs to be fixed.

I think we should shallow copy them all, and in addition shallow copy the @cmd[:cmd], any other issues, we fix when we find them?

[rc9000]

I gave it a quick try with the above PR, still seems to work AFAICT :)

[ytti]

Good enough to me, many thanks.