Bug #12714
Mcollective::Facts::Base wrongly calls .to_s on fact values
| Status: | Rejected | Start date: | 02/17/2012 | |
|---|---|---|---|---|
| Priority: | Normal | Due date: | ||
| Assignee: |  R.I. Pienaar | % Done: | 0% | |
| Category: | Core | |||
| Target version: | 2.1.x | |||
| Keywords: | Affected mCollective version: | 1.3.0 | ||
| Branch: | ||||
Description
We’re upgrading from 1.0.1 to 1.2.1 and part of our registration agent uses the mongo database. We also have added on a latitude and longitude fact where the system knows its location. This gets merged into a @coordinates@ hash in the body of the meta agent and then encoded with the rest of the registration message and then sent to the registration-mongodb agent which then drops it into mongo. From here, we use @search_nodes@ and use mongo’s built in nearest-search ability to find “closest” services. For instance, we can say, find the closest N servers that have a puppet class of @irc::server@ and automatically use that as the local IRC server.
The problem is that this appeared to break in commit:0a2b3ecc185715c84db8ae1f59c84f7ebca2b425 when fact values were forced to a string. The following message appears when loading it into the mongo database:
exception 13026 geo values have to be numbers: { lat: "NN.093451", long: "-NN.030135" }
The facter plugin in MCollective prior to commit:0a2b3ecc185715c84db8ae1f59c84f7ebca2b425 did not coerce the fact value into a string so it worked correctly. Once the @@@facts[key.to_s] = value.to_s@ was added to @mcollective/facts/base.rb@, it stopped working. Right now we have a workaround of running @.to_i@ on the registration coordinated before the encodemsg in the @meta.rb@ registration plugin but I think the better solution is not to coerce the fact value into a string. At least that’s my impression from my perspective, but there may be a perfectly valid reason for doing that, I don’t know. In any case, I hope this gets fixed, if not, we’ll just keep coercing the fact back to an integer.
Related issues
History
#1
Updated by Andrew Forgue over 1 year ago
Looks like the reason for this change was #5377.
#2
Updated by R.I. Pienaar over 1 year ago
- Category set to Core
- Status changed from Unreviewed to Accepted
- Assignee set to R.I. Pienaar
- Target version set to 1.3.x
OK bit of a rock and hard place, but as Facter is getting rich data some time anyway this assumption is wrong, will remove
#3
Updated by R.I. Pienaar over 1 year ago
- Status changed from Accepted to Needs More Information
I’ve thought about this some more and not really sure whats going to be best.
Today in order to do discovery we need to be able to type facts and their values on the command line. Non string facts will break that.
The reason the fact plugin foces it to_s is because facter was badly returning some things like Time objects and Numbers. The time objects seem to be fixed but its still returning numbers:
irb(main):006:0> Facter.to_hash.each_pair{|k,v| puts "%-40s %s" % [k, v.class] unless v.class == String}
physicalprocessorcount Fixnum
blockdevice_vda_size Fixnum
uptime_hours Fixnum
uptime_seconds Fixnum
uptime_days Fixnum
SO these facts will be unusable on the cli if I remove this to_s
In future we will have rich facts and facts will legitimately be able to be numbers but at that point we will also put in a proper solution – no idea what that might be – into mcollective, and I think removing the to_s behaviour would only make sense then
What do you think?
#4
Updated by R.I. Pienaar about 1 year ago
- Target version changed from 1.3.x to 2.1.x
#5
Updated by R.I. Pienaar about 1 year ago
- Status changed from Needs More Information to Rejected
Going to close this one, there’s no doubt going to be lots of change if/when we support complex facts and we will have a better picture then. Till then we have to to_s them