Skip to content

skip delete disk partition event when GPTBasedUUID is disable#639

Merged
akhilerm merged 12 commits intoopenebs-archive:developfrom
liuminjian:develop
Sep 18, 2021
Merged

skip delete disk partition event when GPTBasedUUID is disable#639
akhilerm merged 12 commits intoopenebs-archive:developfrom
liuminjian:develop

Conversation

@liuminjian
Copy link
Copy Markdown
Contributor

@liuminjian liuminjian commented Sep 5, 2021

Pull Request template

Why is this PR required? What issue does it fix?:

I have a situation,GPTBasedUUID feature is disable,i use the block device by local pv and i partition the disk. the block device will be set inactived when receive a event for partition remove, because the disk and the disk partition have the same block device uuid which generate by ID_WWN ID_VENDOR ID_MODEL ID_SERIAL. so it should skip delete disk partition event when GPTBasedUUID is disable.

What this PR does?:

Does this PR require any upgrade changes?:

If the changes in this PR are manually verified, list down the scenarios covered::

Any additional information for your reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs

Checklist:

  • Fixes #
  • PR Title follows the convention of <type>(<scope>): <subject>
  • Has the change log section been updated?
  • Commit has unit tests
  • Commit has integration tests
  • (Optional) Are upgrade changes included in this PR? If not, mention the issue/PR to track:
  • (Optional) If documentation changes are required, which issue on https://github.com/openebs/openebs-docs is used to track them:

PLEASE REMOVE BELOW INFORMATION BEFORE SUBMITTING

The PR title message must follow convention:
<type>(<scope>): <subject>.

Where:
Most common types are:
* feat - for new features, not a new feature for build script
* fix - for bug fixes or improvements, not a fix for build script
* chore - changes not related to production code
* docs - changes related to documentation
* style - formatting, missing semi colons, linting fix etc; no significant production code changes
* test - adding missing tests, refactoring tests; no production code change
* refactor - refactoring production code, eg. renaming a variable or function name, there should not be any significant production code changes
* cherry-pick - if PR is merged in master branch and raised to release branch(like v0.4.x)

IMPORTANT: Please review the CONTRIBUTING.md file for detailed contributing guidelines.

@liuminjian
Copy link
Copy Markdown
Contributor Author

image

Signed-off-by: liuminjian <liuminjian@d-uni.com>
@liuminjian liuminjian changed the title determine the device path for deleteBlockDeviceEvent skip delete block device when GPTBasedUUID is disable Sep 6, 2021
@liuminjian liuminjian changed the title skip delete block device when GPTBasedUUID is disable skip delete disk partition event when GPTBasedUUID is disable Sep 6, 2021
liuminjian added 2 commits September 6, 2021 08:51
Signed-off-by: liuminjian <liuminjian@d-uni.com>
Signed-off-by: liuminjian <liuminjian@d-uni.com>
@liuminjian liuminjian closed this Sep 9, 2021
@liuminjian liuminjian reopened this Sep 9, 2021
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Sep 9, 2021

Codecov Report

Merging #639 (87735c9) into develop (6745dcf) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #639      +/-   ##
===========================================
- Coverage    46.93%   46.89%   -0.04%     
===========================================
  Files           78       78              
  Lines         3814     3817       +3     
===========================================
  Hits          1790     1790              
- Misses        1866     1869       +3     
  Partials       158      158              
Impacted Files Coverage Δ
cmd/ndm_daemonset/probe/eventhandler.go 37.33% <0.00%> (-1.56%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6745dcf...87735c9. Read the comment docs.

Comment thread cmd/ndm_daemonset/probe/eventhandler.go Outdated
Copy link
Copy Markdown
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

Seeing a change in the default disabled feature gate, I am thinking if the legacy method of disk identification is still used?? Are there any use cases for which you need the legacy method?

Comment thread cmd/ndm_daemonset/controller/controller.go Outdated
Copy link
Copy Markdown
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

@liuminjian Can you separate the changes for adding zpool label into a different PR.

The changes in the skipping delete partition event are good.

Copy link
Copy Markdown
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

@liuminjian Given one more comment

@liuminjian
Copy link
Copy Markdown
Contributor Author

OK,i separate the changes for adding zpool label into a different PR

@liuminjian liuminjian requested a review from akhilerm September 15, 2021 09:48
liuminjian added 3 commits September 15, 2021 18:01
Comment thread cmd/ndm_daemonset/probe/eventhandler.go Outdated
@liuminjian liuminjian requested a review from akhilerm September 17, 2021 04:42
akhilerm
akhilerm previously approved these changes Sep 17, 2021
Copy link
Copy Markdown
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @liuminjian for fixing this.

@akhilerm
Copy link
Copy Markdown
Contributor

@liuminjian Can you also add a changelog as mentioned here

Signed-off-by: liuminjian <liuminjian@chinatelecom.cn>
Copy link
Copy Markdown
Contributor

@z0marlin z0marlin left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing this @liuminjian

Copy link
Copy Markdown
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

LGTM

@akhilerm akhilerm merged commit 2554170 into openebs-archive:develop Sep 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants