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

OCPVE-622: chore: remove deviceMinAge due to imperfect validation #380

Merged

Conversation

jakobmoellerdev
Copy link
Contributor

@jakobmoellerdev jakobmoellerdev commented Aug 11, 2023

This removes deviceMinAge as it is not a reliable way to filter devices created by other entities.

Case to cover:
Device is freshly attached by another entity and should not be picked up by LVM Operator

Reason it doesn't work:

  1. Device is attached at t0
  2. Device is looked up LVM Operator at t1
  3. External entity does not lookup Device in t0+30s
  4. Device is viewed as available by LVM operator at t1+30s and gets used anyhow

On top of this, if a device gets created and used by LVM, LVM puts a system lock in place so there cannot ever be concurrent calls to LVM. This makes this condition uninteresting and we can remove minDeviceAge

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 11, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 11, 2023

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

In response to this:

This removes deviceMinAge as it is not a reliable way to filter devices created by other entities.

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 openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 11, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 11, 2023

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

@openshift-ci openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 11, 2023
@jakobmoellerdev
Copy link
Contributor Author

/test all

@jakobmoellerdev jakobmoellerdev changed the title OCPVE-622: chore: remove deviceMinAge through imperfect validation OCPVE-622: chore: remove deviceMinAge due to imperfect validation Aug 11, 2023
@jakobmoellerdev jakobmoellerdev marked this pull request as ready for review August 11, 2023 11:45
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 11, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2023

Codecov Report

Merging #380 (e180edf) into main (a962b90) will increase coverage by 40.47%.
Report is 12 commits behind head on main.
The diff coverage is 75.64%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #380       +/-   ##
===========================================
+ Coverage   16.59%   57.06%   +40.47%     
===========================================
  Files          24       24               
  Lines        2061     2073       +12     
===========================================
+ Hits          342     1183      +841     
+ Misses       1693      807      -886     
- Partials       26       83       +57     
Files Changed Coverage Δ
pkg/vgmanager/vgmanager_controller.go 0.00% <0.00%> (ø)
pkg/cluster/leaderelection.go 57.57% <57.57%> (ø)
pkg/vgmanager/devices.go 73.77% <81.25%> (-0.43%) ⬇️
controllers/lvmcluster_controller.go 60.00% <100.00%> (+60.00%) ⬆️
controllers/topolvm_controller.go 94.14% <100.00%> (+94.14%) ⬆️

... and 9 files with indirect coverage changes

@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 11, 2023

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

In response to this:

This removes deviceMinAge as it is not a reliable way to filter devices created by other entities.

Case to cover:
Device is freshly attached by another entity and should not be picked up by LVM Operator

Reason it doesn't work:

  1. Device is attached at t0
  2. Device is looked up LVM Operator at t1
  3. External entity does not lookup Device in t0+30s
  4. Device is viewed as available by LVM operator at t1+30s and gets used anyhow

On top of this, if a device gets created and used by LVM, LVM puts a system lock in place so there cannot ever be concurrent calls to LVM. This makes this condition uninteresting and we can remove minDeviceAge

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
Copy link
Contributor

openshift-ci bot commented Aug 11, 2023

@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.

@suleymanakbas91
Copy link
Contributor

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 14, 2023

[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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 14, 2023
@openshift-merge-robot openshift-merge-robot merged commit 172fdfe into openshift:main Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants