Bug #2560
closed
Cpu speed search is not working
Description
When looking for cpu speed, we don't have result exepected.
When looking for a CPU speed greater or lesser than a value, it can't find any result
(i.e. I have a cpu speed of "3100.0", when looking for cpu speed >= 1000.0 , i got no result)
Updated by Jonathan CLARKE almost 12 years ago
- Status changed from New to 2
- Priority changed from N/A to 4
- Target version set to 2.3.8
This also happens in 2.3.
Updated by Jonathan CLARKE almost 12 years ago
- Priority changed from 4 to 3
This bug needs fixing ASAP please!
Updated by Vincent MEMBRÉ almost 12 years ago
- Status changed from 2 to Discussion
- Assignee changed from Vincent MEMBRÉ to Jonathan CLARKE
I found the origin of that problem.
It came from the inventory ldap schema. cpu speed has no Ordering Value, it can't search for greater/lower than values.
I'm looking for an ordering value which would go well with that kind of value (we have float ... )
I tried to change syntax of that attribute (and equality and ordering value) with string, numericString and Integer, but the results are not very good...
but at least search for greater and lower than are working and we could get results!
Which type do you think we could use to do that ??
Updated by Vincent MEMBRÉ almost 12 years ago
What if we truncated speed ? a speed of xxxx.yy mhz would became xxxx mhz ?
whith xxxx we could use ldap integer and comparison would work.
I don't think having such a precise information is useful if we can't look for it. but what do you think about that ??
Updated by Jonathan CLARKE almost 12 years ago
- Assignee changed from Jonathan CLARKE to Vincent MEMBRÉ
Vincent MEMBRÉ wrote:
What if we truncated speed ? a speed of xxxx.yy mhz would became xxxx mhz ?
whith xxxx we could use ldap integer and comparison would work.
I don't think having such a precise information is useful if we can't look for it. but what do you think about that ??
Sounds good to me. I'm not very worried about 1/10 or 1/100 MHz ;-)
Just for the record: another idea would be to store the value as an integer but in Hz (ie, 2 GHz is stored as 2 000 000 000 Hz). Then we could keep the precision. But I don't really see the need... Let's stick with the simple solution for now.
Updated by Vincent MEMBRÉ almost 12 years ago
- Status changed from Discussion to Pending technical review
- % Done changed from 0 to 100
Applied in changeset 03570f37d355005b6d12cd374678dc6d80937a07.
Updated by Nicolas CHARLES almost 12 years ago
- Status changed from Pending technical review to Discussion
- % Done changed from 100 to 90
This is valid Vincent
However, you could have probably have foregone the toFloat in the speed = optText(p\"FREQUENCY").map(_.toFloat.toInt)
I have a real question : Doesn't changing the syntax in the LDAP requieres changing the data beforehand ?
We have already LDAP full of CPU speed which are floating point; I'm not sure that changing the schema will automagically change the data to Integer, and thus wouldn't we have some consistency error ?
(note that this cannot be checked by the CI that empty the LDAP when the schema changes)
Updated by François ARMAND almost 12 years ago
The toFoat.toInt is mandatory to take care of inputs like "1.1": "1.1".toInt => exception, "1.1".toFloat.toInt => 1
For the consistancy part, it is something that seems to have been overlooked, but the problem should be self-healing: as long as an inventory has float data for CPU speed, OpenLDAP will happily send the inconsistent data to Rudder (yes...), and that will be caught by the "getAsInt" and return "none".
Then, on the next inventory, the CPU speed will be updated to an integer, and CPU speed will start to be displayed again.
That can be improved with a "speed = e.getAsFloat(A_PROCESSOR_SPEED).map( _.toInt )" in the CPU entry mapping, but I'm not convinced of the enhancement (it hides invalid data in the LDAP, but on the other hand, user won't notice the modification).
Any thoughts ?
Updated by Vincent MEMBRÉ almost 12 years ago
Francois told everything i would say, but i tried the different cases before!
No speed is displayed if it's a float in LDAP (Returning a None, there's no error)
receiving a new report will overwrite floating datas.
what Francois proposes us hides the fact that datas are wrong in LDAP, but it can display datas which is better than display nothing.
besides Research on those wrong datas would not work. which could be disturbing as the data is displayed correctly
Display an error message (such as bad format in process started date) is another alternative as it won't disturb the user.
but with a new inventory received his problems would be resolved.
Updated by Jonathan CLARKE almost 12 years ago
This is an interesting point - I think the CPU speed in itself is a small detail, but it's a good chance to recall some of our principles: Rudder should be as easy to use and "automatic" as possible, therefore we do not want to display an error message to the user about ints/floats, and ideally we don't want to break a working feature.
I think the best solution here is to include a migration script that changes all float values in LDAP to int values during the upgrade to 2.3.8.
Thoughts?
Updated by François ARMAND almost 12 years ago
It seems to be the best from the user point of view, but I don't how much it cost, and so if the cost worth the benefits (at most one information missing until next inventory).
If the price is low, I'm for it.
Updated by Vincent MEMBRÉ almost 12 years ago
Like Francois I think it's the best solution if time required to make the migration script is not too long
Updated by Jonathan CLARKE almost 12 years ago
- Assignee changed from Vincent MEMBRÉ to Jonathan CLARKE
OK, this shouldn't be too long. I've started working on the subject, but won't be able to finish tonight. I'll do it tomorrow.
Updated by Matthieu CERDA almost 12 years ago
- Status changed from Discussion to In progress
- Assignee changed from Jonathan CLARKE to Matthieu CERDA
Me and NPE are taking this bug over.
Updated by Matthieu CERDA almost 12 years ago
- Status changed from In progress to Pending technical review
- % Done changed from 90 to 100
Applied in changeset commit:0e24ea3bc4e647cedfd8f587eb220485fc2e042f.
Updated by Jonathan CLARKE almost 12 years ago
- Status changed from Pending technical review to Discussion
Matthieu CERDA wrote:
Applied in changeset commit:0e24ea3bc4e647cedfd8f587eb220485fc2e042f.
This looks pretty good! It seems nice and efficient.
I have a comments to improve this, though: The bind DN and password you use are the default values. There is no guarantee that they will work since users are encouraged to change them. Instead, you should read in the actual bind DN and password from config files. Since the /opt/rudder/etc/rudder-passwords.conf file has not been introduced in 2.3, I think the safest is just to read the properties "ldap.authdn" and "ldap.authpw" from /opt/rudder/etc/inventory-web.properties or rudder-web.properties.
Updated by Nicolas PERRON almost 12 years ago
- Status changed from Discussion to Pending technical review
Updated by Jonathan CLARKE over 11 years ago
- Status changed from Pending technical review to Released
OK, this looks good to me. However, the migration script was only commited on the 2.4 branch, although the original fix here applies to 2.3... I've backported the change to 2.3.
Updated by Jonathan CLARKE over 11 years ago
- Status changed from Released to Pending technical review
- Target version changed from 2.3.8 to 2.3.9
So, actually this will be kind of only be effective in 2.3.9. And I would like a Technical review of the commits I just made please.
Updated by Jonathan CLARKE over 11 years ago
Jonathan CLARKE wrote:
So, actually this will be kind of only be effective in 2.3.9. And I would like a Technical review of the commits I just made please.
FYI, they appear as being made by Matthieu CERDA, because I cherry-picked his commits from the 2.4 branch.
Updated by Jonathan CLARKE over 11 years ago
- Assignee changed from Matthieu CERDA to Jonathan CLARKE
Updated by Jonathan CLARKE over 11 years ago
- Status changed from Pending technical review to Released
Thanks Nicolas
Updated by Benoît PECCATTE about 9 years ago
- Category changed from 26 to Web - Nodes & inventories