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

[RFC] Test determinism for proposal.generate #1260

Closed
wants to merge 4 commits into from

Conversation

jschmid1
Copy link
Contributor

@jschmid1 jschmid1 commented Aug 2, 2018

Signed-off-by: Joshua Schmid [email protected]

Addresses #1215 ( Adding verify tests for this )

Description:

I'm not sure if that actually proves it. Please review thoroughly!

        Theory:
        The fear is that in a 'replace-disk' operation we _may_ end up with
        different osd - wal/db mappings than before. This would mean that
        a user will have to edit the profiles manually or face a migration.
        We never introduced pre-sorting of disk information that we get from
        hwinfo, which means that we can't guarantee a consistently sorted
        input.

        In order to test this, I try to do the following:

        1) Generate a profile
        2) 'replace' a disk ( replaced.disk() )
        2.1) Generate a 'new' disk ( suffixed with '_GENERATED' )
        2.2) Inject it a at a random index in the output from hwinfo
        2.3) Find the old, replaced disk to compare the wal/db pointer
        3) Compare the new and the old proposal for a match of
           old wal/db pointer and the new wal/db pointer

        To eliminate the factor of 'luck', repeat if 1000 times.

        Caveat:
          * Currently we are only testing if the newly replaced disk
            is matching it's old entry.
            Maybe we should test it for _every_ disk.
EDIT:    ->> I do. <<-
          * The number of repetitions could me reduced by adding more disks
            and therefore an increase in probability of a 'missplacement'
          * We have 3k tests more :/

Please find more information on how to run integration tests here

@jschmid1 jschmid1 changed the title Test determinism for proposal.generate [RFC] Test determinism for proposal.generate Aug 2, 2018
@jschmid1 jschmid1 added the DNM label Aug 2, 2018
@jschmid1
Copy link
Contributor Author

jschmid1 commented Aug 7, 2018

retest this please

@SUSE SUSE deleted a comment from kshtsk Aug 7, 2018
@jschmid1 jschmid1 requested a review from agraul August 8, 2018 11:18
Copy link
Member

@agraul agraul left a comment

Choose a reason for hiding this comment

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

I am still wrapping my head around to exact comparisons, will comment on them later.

# ensure that its a proposal with dedicated wal/db
if disk_set.keys()[0] in replaced_hdd['Device Files']:
if prop_name in ['ssd-spinner', 'nvme-ssd-spinner', 'nvme-ssd', 'nvme-spinner']:
wal_db_replaced = disk_set.values()[0]
Copy link
Member

Choose a reason for hiding this comment

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

As we discussed yesterday: this overwrites wal_db_replaced, turning it into a dict with prop_name as the key should be better.

if matching_disk_from_old_prop:
assert osd_disk == matching_disk_from_old_prop[0].keys()[0]
assert wal_db == matching_disk_from_old_prop[0].values()[0]
return True, [], osd_disk, wal_db, wal_db_replaced
Copy link
Member

Choose a reason for hiding this comment

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

This return is too early, only one disk_set in old_prop[prop_name] gets checked.

old_prop = p.create()

# replace a HDD
hwinfo_out_new = hwinfo_out
Copy link
Member

Choose a reason for hiding this comment

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

this is a bit misleading, hwinfo_out_new is not a copy hwinfo_out, just a second reference to the same list. It does not matter here because hwinfo_out is only used for the max index when inserting the new disk at a random spot, but can be confusing.

Since only one hwinfo_out is needed, we don't need to create a second reference. If we want to make the code easier to understand by having a second list, we can use list slicing to copy all values: hwinfo_out_new = hwinfo_out[:]

Joshua Schmid added 3 commits August 8, 2018 16:57
@jschmid1 jschmid1 force-pushed the test_sorting_profiles branch from 0c3db9e to eb18e64 Compare August 8, 2018 14:58
Signed-off-by: Joshua Schmid <[email protected]>
@jschmid1
Copy link
Contributor Author

@jschmid1
Copy link
Contributor Author

Closing this PR as we moved to another way of replacing disks. Introduced #1302

@jschmid1 jschmid1 closed this Aug 27, 2018
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.

2 participants