OCPBUGS-13558: fix: ensure LVMVolumeGroupNodeStatus is removed by dedicated cleanup controller in case of multi-node

[jakobmoellerdev]

In case the operator gets deployed in a multi-node environemnt the LVMVolumeGroupNodeStatus is orphaned when a node is removed. This is unintended. We need a logic that can succesfully remove this NodeStatus whenever there is a removed Node.

Unfortunately since the status update of LVMCluster is currently assuming the correct presence of the LVMVolumeGroupNodeStatus, the only way I found to fix this issue would be to

1. delete the LVMVolumeGroupNodeStatus in the status update of LVMCluster in case the node does not exist anymore and the Status check finds an orphaned status without a node. potentially expensive since now LVMCluster has to compare all nodes and check if the LVMNodeVolumeStatus exists. Also the removal will be delayed from the Node.
2. delete the LVMVolumeGroupNodeStatus in a new reconcile loop that listens on node changes and uses a finalizer to protect the node deletion until we have the LVMVolumeGroupNodeStatus cleaned up. This is the "clean" solution to how we should handle deletion, however it should be noted This reconcile loop would only need to run outside of SNO, in SNO it can be disabled. Danger here is that if the finalizer is not removed properly, node removal can fail. Good thing is that in theory we can remove the created vgs from the node easily with that hook later on if necessary

Currently the PR uses a new controller.

Currently the main integration test was changed to now remove the node (which triggers the Status object removal) instead of removing the object directly which automatically covers this use case.

[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

[openshift-ci-robot]

@jakobmoellerdev: This pull request references Jira Issue OCPBUGS-13558, which is invalid:

• expected the bug to target the "4.14.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

In case the operator gets deployed in a multi-node environemnt the LVMVolumeGroupNodeStatus is orphaned when a node is removed. This is unintended. We need a logic that can succesfully remove this NodeStatus whenever there is a removed Node.

Currently the PR uses a new controller, but I am assuming I can also introduce a behavior based on NodeStatus instead. This would drop the NodeStatus in case the Node does no longer exist. The concept would be similar but we wouldn't need another controller

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 Jira Issue OCPBUGS-13558, which is invalid:

• expected the bug to target the "4.14.0" version, but it targets "4.15.0" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

In case the operator gets deployed in a multi-node environemnt the LVMVolumeGroupNodeStatus is orphaned when a node is removed. This is unintended. We need a logic that can succesfully remove this NodeStatus whenever there is a removed Node.

Unfortunately since the status update of LVMCluster is currently assuming the correct presence of the LVMVolumeGroupNodeStatus, the only way I found to fix this issue would be to

1. delete the LVMVolumeGroupNodeStatus in the status update of LVMCluster in case the node does not exist anymore and the Status check finds an orphaned status without a node. potentially expensive since now LVMCluster has to compare all nodes and check if the LVMNodeVolumeStatus exists. Also the removal will be delayed from the Node.
2. delete the LVMVolumeGroupNodeStatus in a new reconcile loop that listens on node changes and uses a finalizer to protect the node deletion until we have the LVMVolumeGroupNodeStatus cleaned up. This is the "clean" solution to how we should handle deletion, however it should be noted This reconcile loop would only need to run outside of SNO, in SNO it can be disabled. Danger here is that if the finalizer is not removed properly

Currently the PR uses a new controller.

During my investigation I had some trouble understanding the status update due to the "vgCount" being aggregated for the entire LVMCluster. I now rewrote it so it does the check per deviceClass (which can later be extended) and introduced some debug logs that can be activated when passing --zap-devel flag to the controller. The functionality / logic of the check is still the same.

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 Jira Issue OCPBUGS-13558, which is invalid:

• expected the bug to target the "4.14.0" version, but it targets "4.15.0" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

In case the operator gets deployed in a multi-node environemnt the LVMVolumeGroupNodeStatus is orphaned when a node is removed. This is unintended. We need a logic that can succesfully remove this NodeStatus whenever there is a removed Node.

Unfortunately since the status update of LVMCluster is currently assuming the correct presence of the LVMVolumeGroupNodeStatus, the only way I found to fix this issue would be to

1. delete the LVMVolumeGroupNodeStatus in the status update of LVMCluster in case the node does not exist anymore and the Status check finds an orphaned status without a node. potentially expensive since now LVMCluster has to compare all nodes and check if the LVMNodeVolumeStatus exists. Also the removal will be delayed from the Node.
2. delete the LVMVolumeGroupNodeStatus in a new reconcile loop that listens on node changes and uses a finalizer to protect the node deletion until we have the LVMVolumeGroupNodeStatus cleaned up. This is the "clean" solution to how we should handle deletion, however it should be noted This reconcile loop would only need to run outside of SNO, in SNO it can be disabled. Danger here is that if the finalizer is not removed properly, node removal can fail

Currently the PR uses a new controller.

During my investigation I had some trouble understanding the status update due to the "vgCount" being aggregated for the entire LVMCluster. I now rewrote it so it does the check per deviceClass (which can later be extended) and introduced some debug logs that can be activated when passing --zap-devel flag to the controller. The functionality / logic of the check is still the same.

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 Jira Issue OCPBUGS-13558, which is invalid:

• expected the bug to target the "4.14.0" version, but it targets "4.15.0" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

In case the operator gets deployed in a multi-node environemnt the LVMVolumeGroupNodeStatus is orphaned when a node is removed. This is unintended. We need a logic that can succesfully remove this NodeStatus whenever there is a removed Node.

Unfortunately since the status update of LVMCluster is currently assuming the correct presence of the LVMVolumeGroupNodeStatus, the only way I found to fix this issue would be to

1. delete the LVMVolumeGroupNodeStatus in the status update of LVMCluster in case the node does not exist anymore and the Status check finds an orphaned status without a node. potentially expensive since now LVMCluster has to compare all nodes and check if the LVMNodeVolumeStatus exists. Also the removal will be delayed from the Node.
2. delete the LVMVolumeGroupNodeStatus in a new reconcile loop that listens on node changes and uses a finalizer to protect the node deletion until we have the LVMVolumeGroupNodeStatus cleaned up. This is the "clean" solution to how we should handle deletion, however it should be noted This reconcile loop would only need to run outside of SNO, in SNO it can be disabled. Danger here is that if the finalizer is not removed properly, node removal can fail. Good thing is that in theory we can remove the created vgs from the node easily with that hook

Currently the PR uses a new controller.

During my investigation I had some trouble understanding the status update due to the "vgCount" being aggregated for the entire LVMCluster. I now rewrote it so it does the check per deviceClass (which can later be extended) and introduced some debug logs that can be activated when passing --zap-devel flag to the controller. The functionality / logic of the check is still the same.

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 Jira Issue OCPBUGS-13558, which is invalid:

• expected the bug to target the "4.14.0" version, but it targets "4.15.0" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

In case the operator gets deployed in a multi-node environemnt the LVMVolumeGroupNodeStatus is orphaned when a node is removed. This is unintended. We need a logic that can succesfully remove this NodeStatus whenever there is a removed Node.

Unfortunately since the status update of LVMCluster is currently assuming the correct presence of the LVMVolumeGroupNodeStatus, the only way I found to fix this issue would be to

1. delete the LVMVolumeGroupNodeStatus in the status update of LVMCluster in case the node does not exist anymore and the Status check finds an orphaned status without a node. potentially expensive since now LVMCluster has to compare all nodes and check if the LVMNodeVolumeStatus exists. Also the removal will be delayed from the Node.
2. delete the LVMVolumeGroupNodeStatus in a new reconcile loop that listens on node changes and uses a finalizer to protect the node deletion until we have the LVMVolumeGroupNodeStatus cleaned up. This is the "clean" solution to how we should handle deletion, however it should be noted This reconcile loop would only need to run outside of SNO, in SNO it can be disabled. Danger here is that if the finalizer is not removed properly, node removal can fail. Good thing is that in theory we can remove the created vgs from the node easily with that hook later on if necessary

Currently the PR uses a new controller.

During my investigation I had some trouble understanding the status update due to the "vgCount" being aggregated for the entire LVMCluster. I now rewrote it so it does the check per deviceClass (which can later be extended) and introduced some debug logs that can be activated when passing --zap-devel flag to the controller. The functionality / logic of the check is still the same.

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

[openshift-ci-robot]

@jakobmoellerdev: This pull request references Jira Issue OCPBUGS-13558, which is invalid:

• expected the bug to target the "4.14.0" version, but it targets "4.15.0" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

In case the operator gets deployed in a multi-node environemnt the LVMVolumeGroupNodeStatus is orphaned when a node is removed. This is unintended. We need a logic that can succesfully remove this NodeStatus whenever there is a removed Node.

Unfortunately since the status update of LVMCluster is currently assuming the correct presence of the LVMVolumeGroupNodeStatus, the only way I found to fix this issue would be to

1. delete the LVMVolumeGroupNodeStatus in the status update of LVMCluster in case the node does not exist anymore and the Status check finds an orphaned status without a node. potentially expensive since now LVMCluster has to compare all nodes and check if the LVMNodeVolumeStatus exists. Also the removal will be delayed from the Node.
2. delete the LVMVolumeGroupNodeStatus in a new reconcile loop that listens on node changes and uses a finalizer to protect the node deletion until we have the LVMVolumeGroupNodeStatus cleaned up. This is the "clean" solution to how we should handle deletion, however it should be noted This reconcile loop would only need to run outside of SNO, in SNO it can be disabled. Danger here is that if the finalizer is not removed properly, node removal can fail. Good thing is that in theory we can remove the created vgs from the node easily with that hook later on if necessary

Currently the PR uses a new controller.

During my investigation I had some trouble understanding the status update due to the "vgCount" being aggregated for the entire LVMCluster. I now rewrote it so it does the check per deviceClass (which can later be extended) and introduced some debug logs that can be activated when passing --zap-devel flag to the controller. The functionality / logic of the check is still the same.

TODO: an integration test mimicking the nodes (if thats even possible with envtest)

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 Jira Issue OCPBUGS-13558, which is invalid:

• expected the bug to target the "4.14.0" version, but it targets "4.15.0" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

In case the operator gets deployed in a multi-node environemnt the LVMVolumeGroupNodeStatus is orphaned when a node is removed. This is unintended. We need a logic that can succesfully remove this NodeStatus whenever there is a removed Node.

Unfortunately since the status update of LVMCluster is currently assuming the correct presence of the LVMVolumeGroupNodeStatus, the only way I found to fix this issue would be to

1. delete the LVMVolumeGroupNodeStatus in the status update of LVMCluster in case the node does not exist anymore and the Status check finds an orphaned status without a node. potentially expensive since now LVMCluster has to compare all nodes and check if the LVMNodeVolumeStatus exists. Also the removal will be delayed from the Node.
2. delete the LVMVolumeGroupNodeStatus in a new reconcile loop that listens on node changes and uses a finalizer to protect the node deletion until we have the LVMVolumeGroupNodeStatus cleaned up. This is the "clean" solution to how we should handle deletion, however it should be noted This reconcile loop would only need to run outside of SNO, in SNO it can be disabled. Danger here is that if the finalizer is not removed properly, node removal can fail. Good thing is that in theory we can remove the created vgs from the node easily with that hook later on if necessary

Currently the PR uses a new controller.

During my investigation I had some trouble understanding the status update due to the "vgCount" being aggregated for the entire LVMCluster. I now rewrote it so it does the check per deviceClass (which can later be extended) and introduced some debug logs that can be activated when passing --zap-devel flag to the controller. The functionality / logic of the check is still the same.

Currently the main integration test was changed to now remove the node (which triggers the Status object removal) instead of removing the object directly which automatically covers this use case.

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.

[codecov-commenter]

Codecov Report

Merging #372 (59b26bc) into main (a962b90) will increase coverage by 40.42%.
Report is 8 commits behind head on main.
The diff coverage is 64.93%.

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #372       +/-   ##
===========================================
+ Coverage   16.59%   57.01%   +40.42%     
===========================================
  Files          24       28        +4     
  Lines        2061     2138       +77     
===========================================
+ Hits          342     1219      +877     
+ Misses       1693      828      -865     
- Partials       26       91       +65     

Files Changed	Coverage Δ
controllers/node_removal_controller.go	56.00% <56.00%> (ø)
controllers/node_removal_controller_watches.go	61.53% <61.53%> (ø)
pkg/cluster/leaderelection.go	66.66% <66.66%> (ø)
pkg/cluster/sno.go	72.72% <72.72%> (ø)
controllers/lvmcluster_controller_watches.go	91.42% <100.00%> (+91.42%)	⬆️

... and 10 files with indirect coverage changes

[jakobmoellerdev]

/test all

[jakobmoellerdev]

/hold as testing revealed that I potentially introduce a delete issue

[jakobmoellerdev]

seems like there is an edge case sometimes where the vg is not removed. not sure why, but unrelated to the PR. when I can reproduce I will open a separate issue.

[jakobmoellerdev]

/hold

[jakobmoellerdev]

/jira refresh

[openshift-ci-robot]

@jakobmoellerdev: This pull request references Jira Issue OCPBUGS-13558, which is invalid:

• expected the bug to target the "4.14.0" version, but it targets "4.15.0" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/jira refresh

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 Jira Issue OCPBUGS-13558, which is invalid:

• expected the bug to target the "4.14.0" version, but it targets "4.15.0" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

In case the operator gets deployed in a multi-node environemnt the LVMVolumeGroupNodeStatus is orphaned when a node is removed. This is unintended. We need a logic that can succesfully remove this NodeStatus whenever there is a removed Node.

Unfortunately since the status update of LVMCluster is currently assuming the correct presence of the LVMVolumeGroupNodeStatus, the only way I found to fix this issue would be to

1. delete the LVMVolumeGroupNodeStatus in the status update of LVMCluster in case the node does not exist anymore and the Status check finds an orphaned status without a node. potentially expensive since now LVMCluster has to compare all nodes and check if the LVMNodeVolumeStatus exists. Also the removal will be delayed from the Node.
2. delete the LVMVolumeGroupNodeStatus in a new reconcile loop that listens on node changes and uses a finalizer to protect the node deletion until we have the LVMVolumeGroupNodeStatus cleaned up. This is the "clean" solution to how we should handle deletion, however it should be noted This reconcile loop would only need to run outside of SNO, in SNO it can be disabled. Danger here is that if the finalizer is not removed properly, node removal can fail. Good thing is that in theory we can remove the created vgs from the node easily with that hook later on if necessary

Currently the PR uses a new controller.

Currently the main integration test was changed to now remove the node (which triggers the Status object removal) instead of removing the object directly which automatically covers this use case.

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.

[qJkee]

/lgtm

[jakobmoellerdev]

/cc @suleymanakbas91 to get another opinion on that controller approach.

[jakobmoellerdev]

/test all

[jakobmoellerdev]

/test verify

[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

[suleymanakbas91]

/lgtm

[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: Jira Issue OCPBUGS-13558: All pull requests linked via external trackers have merged:

• openshift/lvm-operator#372

Jira Issue OCPBUGS-13558 has been moved to the MODIFIED state.

In response to this:

In case the operator gets deployed in a multi-node environemnt the LVMVolumeGroupNodeStatus is orphaned when a node is removed. This is unintended. We need a logic that can succesfully remove this NodeStatus whenever there is a removed Node.

Unfortunately since the status update of LVMCluster is currently assuming the correct presence of the LVMVolumeGroupNodeStatus, the only way I found to fix this issue would be to

1. delete the LVMVolumeGroupNodeStatus in the status update of LVMCluster in case the node does not exist anymore and the Status check finds an orphaned status without a node. potentially expensive since now LVMCluster has to compare all nodes and check if the LVMNodeVolumeStatus exists. Also the removal will be delayed from the Node.
2. delete the LVMVolumeGroupNodeStatus in a new reconcile loop that listens on node changes and uses a finalizer to protect the node deletion until we have the LVMVolumeGroupNodeStatus cleaned up. This is the "clean" solution to how we should handle deletion, however it should be noted This reconcile loop would only need to run outside of SNO, in SNO it can be disabled. Danger here is that if the finalizer is not removed properly, node removal can fail. Good thing is that in theory we can remove the created vgs from the node easily with that hook later on if necessary

Currently the PR uses a new controller.

Currently the main integration test was changed to now remove the node (which triggers the Status object removal) instead of removing the object directly which automatically covers this use case.

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.