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.

Sign up for GitHub

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

Jump to bottom

🐛 Protect Metal3Data and Metal3DataClaim with Finalizers #1478

Merged
merged 2 commits into from
May 28, 2024
Merged

🐛 Protect Metal3Data and Metal3DataClaim with Finalizers #1478

merged 2 commits into from
May 28, 2024

Conversation

maxrantil
Copy link

What this PR does / why we need it:
As the title describes.

Which issue(s) this PR fixes
Fixes #1356

@metal3-io-bot metal3-io-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 28, 2024
@maxrantil maxrantil force-pushed the metal3data-deleteable/max branch 2 times, most recently from b4c9431 to b7cad4e Compare February 28, 2024 07:49
@maxrantil maxrantil changed the title 🌱 Add extra finalizers to stop metal3data/dataClaim from being deletable while in use. WIP: 🌱 Add extra finalizers to stop metal3data/dataClaim from being deletable while in use. Feb 28, 2024
@metal3-io-bot metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 28, 2024
@metal3-io-bot metal3-io-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 29, 2024
@maxrantil maxrantil changed the title WIP: 🌱 Add extra finalizers to stop metal3data/dataClaim from being deletable while in use. WIP: 🌱 Add metal3machine finalizer to metal3data/dataClaim to stop them from being deletable while in use. Feb 29, 2024
@metal3-io-bot
Copy link
Contributor

@maxrantil: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test build
  • /test generate
  • /test gofmt
  • /test gomod
  • /test govet
  • /test manifestlint
  • /test markdownlint
  • /test shellcheck
  • /test test
  • /test unit

Use /test all to run the following jobs that were automatically triggered:

  • generate
  • gofmt
  • gomod
  • govet
  • manifestlint
  • unit

In response to this:

/test test-centos-e2e-integration-main
/test test-ubuntu-integration-main

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.

@maxrantil
Copy link
Author

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

@maxrantil
Copy link
Author

/test-ubuntu-integration-main

1 similar comment
@maxrantil
Copy link
Author

/test-ubuntu-integration-main

@maxrantil maxrantil force-pushed the metal3data-deleteable/max branch 3 times, most recently from c6aea95 to 1d96317 Compare March 14, 2024 10:01
@maxrantil
Copy link
Author

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

@metal3-io-bot metal3-io-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 2, 2024
@maxrantil maxrantil force-pushed the metal3data-deleteable/max branch 4 times, most recently from dffc5b7 to d2a3290 Compare April 2, 2024 06:57
lentzi90
lentzi90 reviewed Apr 11, 2024
View reviewed changes
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

Just a few issues with the tests. The rest of it looks good

baremetal/metal3datatemplate_manager_test.go Outdated Show resolved Hide resolved
baremetal/metal3datatemplate_manager_test.go Outdated Show resolved Hide resolved
baremetal/metal3datatemplate_manager_test.go Outdated Show resolved Hide resolved
baremetal/metal3datatemplate_manager_test.go Outdated Show resolved Hide resolved
baremetal/metal3datatemplate_manager_test.go Outdated Show resolved Hide resolved
@maxrantil maxrantil force-pushed the metal3data-deleteable/max branch 2 times, most recently from 3557159 to 6aa659a Compare April 11, 2024 12:14
@maxrantil maxrantil changed the title WIP: 🌱 Add metal3machine finalizer to metal3data/dataClaim to stop them from being deletable while in use. 🌱 Add metal3machine finalizer to metal3data/dataClaim to stop them from being deletable while in use. Apr 11, 2024
@metal3-io-bot metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 11, 2024
@lentzi90
Protect Metal3Data and Metal3DataClaim with Finalizers
fe486bb 
Signed-off-by: Lennart Jern <[email protected]>
Co-authored-by: Max Rantil <[email protected]>
@lentzi90
Copy link
Member

lentzi90 commented May 20, 2024 •
edited
Loading

The Metal3Machine manager is responsible for creating and deleting the Metal3DataClaims used by the Metal3Machines. To make sure that the Metal3DataClaim is protected (and cleaned up properly), we now add a finalizer when the Metal3DataClaim is created and remove it when cleaning up.

Similarly, the Metal3DataTemplate manager is responsible for the Metal3Data. It now also adds a finalizer when creating Metal3Data and removes it when cleaning up.

I have checked manually that this solves the issue. There is still something wrong. Deleting the M3DC deleted the M3D.

@lentzi90
Copy link
Member

/test metal3-ubuntu-e2e-integration-test-main
/test metal3-centos-e2e-integration-test-main

@metal3-io-bot metal3-io-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 21, 2024
@lentzi90
Copy link
Member

/test metal3-ubuntu-e2e-integration-test-main
/test metal3-centos-e2e-integration-test-main

@lentzi90
Copy link
Member

Updated with a second commit that checks if the consumer is gone before proceeding with the deletion.
For the Metal3Data it is quite straight forward to, but for the Metal3DataClaim I had to go through the owner references... That is not ideal but I could not find another way to check the Metal3Machine. Any suggestions?

@lentzi90
Copy link
Member

/test metal3-ubuntu-e2e-integration-test-main
/test metal3-centos-e2e-integration-test-main

1 similar comment
@lentzi90
Copy link
Member

/test metal3-ubuntu-e2e-integration-test-main
/test metal3-centos-e2e-integration-test-main

@lentzi90
Ensure M3DC is gone before cleaning up M3D
acb35f3 
Before cleaning up Metal3Data, check that the Metal3DataClaim
(consumer) is gone.
This commit also renames some of the test objects and variables to make them easier to
understand and adds a constant for "Metal3Machine".

Signed-off-by: Lennart Jern <[email protected]>
@lentzi90
Copy link
Member

Discussed offline and decided to skip going through owner references for the Metal3DataClaim. We are checking that the claim is gone before deleting the data though.

@lentzi90
Copy link
Member

/test metal3-ubuntu-e2e-integration-test-main
/test metal3-centos-e2e-integration-test-main

@lentzi90
Copy link
Member

/cc @Sunnat @kashifest

@metal3-io-bot
Copy link
Contributor

@lentzi90: GitHub didn't allow me to request PR reviews from the following users: sunnat.

Note that only metal3-io members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @Sunnat @kashifest

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.

@lentzi90
Copy link
Member

/cc @Sunnatillo

Sunnatillo
Sunnatillo reviewed May 22, 2024
View reviewed changes
Copy link
Member

@Sunnatillo Sunnatillo left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label May 22, 2024
@lentzi90
Copy link
Member

Centos tests are unfortunately broken at the moment...

@Sunnatillo
Copy link
Member

/test metal3-centos-e2e-integration-test-main

kashifest
kashifest reviewed May 28, 2024
View reviewed changes
Copy link
Member

@kashifest kashifest left a comment

Choose a reason for hiding this comment

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

/approve

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kashifest

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

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 28, 2024
@metal3-io-bot metal3-io-bot merged commit ee02e92 into metal3-io:main May 28, 2024
14 of 15 checks passed
@metal3-io-bot metal3-io-bot deleted the metal3data-deleteable/max branch May 28, 2024 05:41
@tmmorin tmmorin mentioned this pull request Sep 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kashifest kashifest kashifest left review comments

@Sunnatillo Sunnatillo Sunnatillo left review comments

@lentzi90 lentzi90 lentzi90 left review comments

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Metal3Data deletable while in-use
5 participants
@maxrantil @metal3-io-bot @lentzi90 @Sunnatillo @kashifest