Skip to content

Conversation

@kashifest
Copy link
Member

Metal3Machine should not own BMH. It should consume it and release it when not needed. we already put a M3M consumer reference on BMH. This also would mean that for pivoting use cases we must add the clusterctl move labels in the CRDs. Otherwise BMH wont be pivoted to target clusters.

⚠️ Now, BMH object/CRD needs to have The object has the clusterctl.cluster.x-k8s.io/move label or the clusterctl.cluster.x-k8s.io/move-hierarchy label to make sure BMH is pivoted to target cluster and removed from the source. clusterctl.cluster.x-k8s.io/move and clusterctl.cluster.x-k8s.io/move-hierarchy labels could be applied to single objects or at the CRD level (the label applies to all the objects).

We still keep the removal of owner reference code for one minor release cycle to facilitate upgrade scenario where a BMH could still have an owner reference set from previous version. Starting from v1.11.x minor cycle we will remove this code also.
Signed-off-by: Kashif Khan [email protected]

@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 Apr 22, 2025
@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign tuminoid for approval. For more information see the Code Review Process.

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

Details 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 size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 22, 2025
@kashifest
Copy link
Member Author

/hold
This is still WIP, pushed here only for testing, not ready to be merged yet.

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 22, 2025
@kashifest kashifest changed the title WIP: Remove Metal3Machine owner reference from BMH WIP: ⚠️ Remove Metal3Machine owner reference from BMH Apr 22, 2025
@kashifest
Copy link
Member Author

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

1 similar comment
@kashifest
Copy link
Member Author

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

@kashifest kashifest force-pushed the remove/bmh-ownerRef branch from 4def180 to e92061b Compare April 23, 2025 06:46
@kashifest
Copy link
Member Author

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

@kashifest
Copy link
Member Author

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

@kashifest
Copy link
Member Author

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

@kashifest
Copy link
Member Author

/hold

@kashifest
Copy link
Member Author

/hold cancel

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 23, 2025
@kashifest
Copy link
Member Author

/hold

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 23, 2025
@kashifest
Copy link
Member Author

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

@kashifest
Copy link
Member Author

@kashifest
Copy link
Member Author

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

@kashifest
Copy link
Member Author

@kashifest
Copy link
Member Author

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

@kashifest kashifest force-pushed the remove/bmh-ownerRef branch from e92061b to 1a836b0 Compare April 24, 2025 07:17
@kashifest
Copy link
Member Author

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

@kashifest kashifest force-pushed the remove/bmh-ownerRef branch from 1a836b0 to 74bbadf Compare April 24, 2025 12:12
@kashifest
Copy link
Member Author

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

@kashifest kashifest force-pushed the remove/bmh-ownerRef branch from 74bbadf to 24a8ff0 Compare April 25, 2025 06:17
@kashifest
Copy link
Member Author

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

@metal3-io-bot metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Jul 24, 2025
@lentzi90
Copy link
Member

/hold cancel

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 21, 2025
@lentzi90
Copy link
Member

/hold

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 21, 2025
@lentzi90
Copy link
Member

/override build

@metal3-io-bot
Copy link
Contributor

@lentzi90: Overrode contexts on behalf of lentzi90: build

Details

In response to this:

/override build

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-sigs/prow repository.

@tuminoid tuminoid marked this pull request as draft October 21, 2025 11:09
@tuminoid
Copy link
Member

Converted to draft in attempt to stop Prow.

@tuminoid
Copy link
Member

@kashifest: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
metal3-centos-e2e-integration-test-main 7e055ce link true /test metal3-centos-e2e-integration-test-main
build 7e055ce link true /test build
Full PR test history. Your PR dashboard.

This should be the last one if the draft conversion worked.

@tuminoid
Copy link
Member

It kept spamming :(

kashifest and others added 3 commits October 22, 2025 09:53
Metal3Machine should not own BMH. It should consume it and release it when not needed. we already put a M3M consumer reference on BMH. This also would mean that for pivoting use cases we must add the clusterctl move labels in the CRDs. Otherwise BMH wont be pivoted to target clusters.

⚠️ Now, BMH object/CRD needs to have The object has the clusterctl.cluster.x-k8s.io/move label or the clusterctl.cluster.x-k8s.io/move-hierarchy  label to make sure BMH is pivoted to target cluster and removed from the source. clusterctl.cluster.x-k8s.io/move and clusterctl.cluster.x-k8s.io/move-hierarchy labels could be applied to single objects or at the CRD level (the label applies to all the objects).

We still keep the removal of owner reference code for one minor release cycle to facilitate upgrade scenario where a BMH could still have an owner reference set from previous version. Starting from v1.11.x minor cycle we will remove this code also.
Signed-off-by: Kashif Khan <[email protected]>
Signed-off-by: Kashif Khan <[email protected]>
@kashifest kashifest force-pushed the remove/bmh-ownerRef branch from 7e055ce to 16b96fc Compare October 22, 2025 07:03
@metal3-io-bot metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2025
@kashifest
Copy link
Member Author

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

@kashifest kashifest marked this pull request as ready for review October 22, 2025 07:04
@kashifest kashifest force-pushed the remove/bmh-ownerRef branch 2 times, most recently from 633c68c to 346d48a Compare October 22, 2025 07:37
@kashifest
Copy link
Member Author

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

Signed-off-by: Kashif Khan <[email protected]>
@kashifest kashifest force-pushed the remove/bmh-ownerRef branch from 346d48a to 4fe2f81 Compare October 29, 2025 07:25
@kashifest
Copy link
Member Author

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

1 similar comment
@kashifest
Copy link
Member Author

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

@metal3-io-bot
Copy link
Contributor

@kashifest: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
unit 4fe2f81 link true /test unit
metal3-centos-e2e-integration-test-main 4fe2f81 link true /test metal3-centos-e2e-integration-test-main

Full PR test history. Your PR dashboard.

Details

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-sigs/prow repository. I understand the commands that are listed here.

@metal3-io-bot
Copy link
Contributor

@kashifest: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
unit 4fe2f81 link true /test unit
metal3-centos-e2e-integration-test-main 4fe2f81 link true /test metal3-centos-e2e-integration-test-main
Details

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-sigs/prow repository. I understand the commands that are listed here.

@metal3-io-bot metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Nov 3, 2025
@metal3-io-bot
Copy link
Contributor

PR needs rebase.

Details

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-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. 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.

4 participants