Bug #2560: Cpu speed search is not working - Rudder - Issue Tracker

Custom queries

8.1 beta
Bugs triage
My TR
Need technical review
Open bugs (all)
Open bugs (UI - UX)
Open UX defects (all)
Opened by users

Actions

Copy link

Bug #2560

closed

Cpu speed search is not working

Added by Vincent MEMBRÉ almost 12 years ago. Updated about 9 years ago.

Status:

Released

Priority:

Assignee:

Jonathan CLARKE

Category:

Web - Nodes & inventories

Target version:

2.3.9

Pull Request:

Severity:

UX impact:

User visibility:

Effort required:

Priority:

Name check:

Fix check:

Regression:

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)

History
Notes
Property changes
Associated revisions

Actions

Copy link

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.

Actions

Copy link

Updated by Jonathan CLARKE almost 12 years ago

Priority changed from 4 to 3

This bug needs fixing ASAP please!

Actions

Copy link

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 ??

Actions

Copy link

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 ??

Actions

Copy link

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.

Actions

Copy link

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.

Actions

Copy link

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)

Actions

Copy link

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 ?

Actions

Copy link

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.

Actions

Copy link

#10

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?

Actions

Copy link

#11

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.

Actions

Copy link

#12

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

Actions

Copy link

#13

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.

Actions

Copy link

#14

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.

Actions

Copy link

#15

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.

Actions

Copy link

#16

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.

Actions

Copy link

#17

Updated by Nicolas PERRON almost 12 years ago

Now, it seems OK for me.

Actions

Copy link

#18

Updated by Nicolas PERRON almost 12 years ago

Status changed from Discussion to Pending technical review

Actions

Copy link

#20

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.

Actions

Copy link

#21