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

Write unit test for LVM device manager's creteNS() functionality #692

Open
avalluri opened this issue Jun 25, 2020 · 3 comments
Open

Write unit test for LVM device manager's creteNS() functionality #692

avalluri opened this issue Jun 25, 2020 · 3 comments
Labels
future needs to be fixed in some future release

Comments

@avalluri
Copy link
Contributor

As pointed in #688 (comment), currently, there are no tests for validating if the LVM device manager uses only requested pmemPercentage out of available PMEM space of a region. This is a bit complicated as the testing this requires underlined PMEM region/namespace properties.

It would be good if we could find a way to develop a test case for this. As suggest by @pohly, one way is making createNS() work on an interface, so that tests could have a mock implementation.

@pohly
Copy link
Contributor

pohly commented Aug 25, 2020

We already had two bugs in that code (fixed by #714):

  • did not ignore regions in the wrong mode (not sure how relevant)
  • caused restart of driver (did happen each time the driver ran for the first time on a node)

@okartau
Copy link
Contributor

okartau commented Aug 28, 2020

I just detected a third bug, fix in #717
I agree, unit test code would help significantly. One of the bugs was regression caused by omitted code lines in function move where original code was correct.
We cant have really same what comes form HW, but we can use some pre-defined sets of typical value combinations.

@pohly
Copy link
Contributor

pohly commented Apr 23, 2021

We do have infrastructure for mocking libndctl now in https://github.com/intel/pmem-csi/tree/devel/pkg/ndctl/fake

That could be used for this.

@pohly pohly added the future needs to be fixed in some future release label Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
future needs to be fixed in some future release
Projects
None yet
Development

No branches or pull requests

3 participants