OCPVE-677: chore: refactor vgmanager into different interfaces and introduce tests

[jakobmoellerdev]

This refactors vgmanager by using a new different Interface: lvm.LVM encapsulates ALL lvm commands in the entire code and simultaneously enables unit testing.

This allows the mocking / replacement / redirection of these interfaces at will. Also there is a new suite_test.go with the accompanying test in pkg/vgmanager. The tests are powered by mockery, a configuration generator for mocks using testify. (used instead of gomock because gomock has been moved to uber and we already have testify so I didnt need to introduce new direct dependencies). The mock generation is configured via a central .mockery.yaml in the root of the repo.

[openshift-ci-robot]

@jakobmoellerdev: This pull request references OCPVE-677 which is a valid jira issue.

In response to this:

This refactors vgmanager by using 3 new different Interfaces:

1. lvm.LVM encapsulates ALL lvm commands in the entire code
2. lsblk.LSBLK encapsulates ALL block device related commands in the entire code
3. lvmd.LVMDConfig encapsulates the Read/Write/Delete for the lvmd config file.

This allows the mocking / replacement / redirection of these interfaces at will for testing. Also there is a new suite_test.go with the accompanying test in pkg/vgmanager which now tests vgmanager functionality natively in case the suite is run as root. If it is not run as root, the tests currently get skipped. We have to see if its feasible to run them within our existing pipelines. If its not feasible to run tests with root, we need to either give permissions to the host commands or we need to completely stub the local lvm implementation.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[jakobmoellerdev]

/test all

[jakobmoellerdev]

/test all

[openshift-ci-robot]

@jakobmoellerdev: This pull request references OCPVE-677 which is a valid jira issue.

In response to this:

This refactors vgmanager by using 3 new different Interfaces:

1. lvm.LVM encapsulates ALL lvm commands in the entire code
2. lsblk.LSBLK encapsulates ALL block device related commands in the entire code
3. lvmd.LVMDConfig encapsulates the Read/Write/Delete for the lvmd config file.

This allows the mocking / replacement / redirection of these interfaces at will for testing. Also there is a new suite_test.go with the accompanying test in pkg/vgmanager which now tests vgmanager functionality natively in case the suite is run as root. If it is not run as root, the tests currently get skipped. We have to see if its feasible to run them within our existing pipelines. If its not feasible to run tests with root, we need to either give permissions to the host commands or we need to completely stub the local lvm implementation.

NOTE: Requires #404 as the vg/pvremove command is necessary to be extended with lvmdevices --deldev by pid to have proper losetup device cleanup without orphaned devices.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[jakobmoellerdev]

/test all

[codecov-commenter]

Codecov Report

Merging #406 (1221854) into main (d147fcb) will increase coverage by 0.76%.
The diff coverage is 12.14%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #406      +/-   ##
==========================================
+ Coverage   54.11%   54.87%   +0.76%     
==========================================
  Files          27       28       +1     
  Lines        2360     2657     +297     
==========================================
+ Hits         1277     1458     +181     
- Misses        997     1067      +70     
- Partials       86      132      +46     

Files Changed	Coverage Δ
pkg/lvm/LVM_mock.go	0.00% <0.00%> (ø)
pkg/lvm/lvm.go	43.71% <33.89%> (+5.46%)	⬆️
pkg/vgmanager/vgmanager_controller.go	44.85% <45.16%> (+34.85%)	⬆️
pkg/vgmanager/devices.go	85.93% <57.14%> (+10.15%)	⬆️
pkg/filter/filter.go	62.74% <61.53%> (ø)
controllers/topolvm_node.go	95.14% <100.00%> (ø)
pkg/vgmanager/status.go	55.78% <100.00%> (+55.78%)	⬆️

[jakobmoellerdev]

/test all

[jakobmoellerdev]

/test lvm-operator-e2e-aws

[jakobmoellerdev]

/test all

[jakobmoellerdev]

/test all

[jakobmoellerdev]

/test all

[jakobmoellerdev]

/test all

[jakobmoellerdev]

/test all

[jakobmoellerdev]

/test all

[jakobmoellerdev]

/test all

[jakobmoellerdev]

/test all

[jakobmoellerdev]

/test all

[jakobmoellerdev]

/test all

[openshift-ci]

@jakobmoellerdev: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[openshift-ci-robot]

@jakobmoellerdev: This pull request references OCPVE-677 which is a valid jira issue.

In response to this:

This refactors vgmanager by using a new different Interface: lvm.LVM encapsulates ALL lvm commands in the entire code and simultaneously enables unit testing.

This allows the mocking / replacement / redirection of these interfaces at will. Also there is a new suite_test.go with the accompanying test in pkg/vgmanager. The tests are powered by mockery, a configuration generator for mocks using testify. (used instead of gomock because gomock has been moved to uber and we already have testify so I didnt need to introduce new direct dependencies). The mock generation is configured via a central .mockery.yaml in the root of the repo.

NOTE: Requires #404 as the vg/pvremove command is necessary to be extended with lvmdevices --deldev by pid to have proper losetup device cleanup without orphaned devices.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[openshift-ci-robot]

@jakobmoellerdev: This pull request references OCPVE-677 which is a valid jira issue.

In response to this:

This refactors vgmanager by using a new different Interface: lvm.LVM encapsulates ALL lvm commands in the entire code and simultaneously enables unit testing.

This allows the mocking / replacement / redirection of these interfaces at will. Also there is a new suite_test.go with the accompanying test in pkg/vgmanager. The tests are powered by mockery, a configuration generator for mocks using testify. (used instead of gomock because gomock has been moved to uber and we already have testify so I didnt need to introduce new direct dependencies). The mock generation is configured via a central .mockery.yaml in the root of the repo.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[suleymanakbas91]

/lgtm
/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jakobmoellerdev, suleymanakbas91

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [suleymanakbas91]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment