OCPVE-627: fix: concurrent apply / status checks -> LVMCluster

[jakobmoellerdev]

This PR introduces concurrent processing for the ensureCreated operations for each resourceManager in the LVMClusterReconciler controller, which significantly improves the overall reconciliation time for the LVMCluster custom resource.

The newly added multierror.go groups multiple errors into a single error, thereby helping to manage multiple errors generated from concurrent ensureCreated calls. This helps the concurrent apply since errors are not cancelled when one call fails, and multiple failures can occur at once.

Moreover, readiness checks have been added after the creation of Deployment and DaemonSet resources in the topolvm_controller.go, topolvm_node.go, and vgmanager.go files respectively.
These checks ensure that the deployment or DaemonSet has been completely initialized and is ready for use.
It also adds verifyDeploymentReadiness and verifyDaemonSetReadiness functions in utils.go to verify the readiness of Deployment and DaemonSet resources respectively.

Note: The checks currently do not get propagated to LVMCluster yet because the LVMCluster State is independant from the Readiness checks done during reconciliation. We will need an API Change to propagate this correctly (in a later change) as otherwise we would set LVMCluster to Degraded/Failed but couldnt show why (because the only status message fields in LVMCluster are for volume group failures, not any of the basic deployed components, and there is no vg present at the time of the readiness check)

[openshift-ci-robot]

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

In response to this:

This PR introduces concurrent processing for the ensureCreated operations for each resourceManager in the LVMClusterReconciler controller, which significantly improves the overall reconciliation time for the LVMCluster custom resource.

The newly added multierror.go groups multiple errors into a single error, thereby helping to manage multiple errors generated from concurrent ensureCreated calls. This helps the concurrent apply since errors are not cancelled when one call fails, and multiple failures can occur at once.

Moreover, readiness checks have been added after the creation of Deployment and DaemonSet resources in the topolvm_controller.go, topolvm_node.go, and vgmanager.go files respectively.
These checks ensure that the deployment or DaemonSet has been completely initialized and is ready for use.
It also adds verifyDeploymentReadiness and verifyDaemonSetReadiness functions in utils.go to verify the readiness of Deployment and DaemonSet resources respectively.

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 OCPVE-627 which is a valid jira issue.

In response to this:

This PR introduces concurrent processing for the ensureCreated operations for each resourceManager in the LVMClusterReconciler controller, which significantly improves the overall reconciliation time for the LVMCluster custom resource.

The newly added multierror.go groups multiple errors into a single error, thereby helping to manage multiple errors generated from concurrent ensureCreated calls. This helps the concurrent apply since errors are not cancelled when one call fails, and multiple failures can occur at once.

Moreover, readiness checks have been added after the creation of Deployment and DaemonSet resources in the topolvm_controller.go, topolvm_node.go, and vgmanager.go files respectively.
These checks ensure that the deployment or DaemonSet has been completely initialized and is ready for use.
It also adds verifyDeploymentReadiness and verifyDaemonSetReadiness functions in utils.go to verify the readiness of Deployment and DaemonSet resources respectively.

TODO:

• add test adjustments because envtest does not have a running controller for deployments and we will need to mock the readiness of the created resources

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]

what if something goes wrong and we will stuck there forever?

[jakobmoellerdev]

If you look at the code path there should not be a way to get stuck here. This will always return exactly the amount of times that a resource is attempted to be created. This means that there will always be the correct amount of results, no matter the amount of failures in the goroutines.

[qJkee]

what about using https://pkg.go.dev/golang.org/x/sync/errgroup ?

[jakobmoellerdev]

Errgroup cancel the context on the first error. We dont want the creation of other resources to abort in case of an error on one of the resources.

[codecov-commenter]

Codecov Report

Merging #391 (8c5e2bc) into main (a962b90) will increase coverage by 39.78%.
Report is 28 commits behind head on main.
The diff coverage is 63.98%.

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #391       +/-   ##
===========================================
+ Coverage   16.59%   56.37%   +39.78%     
===========================================
  Files          24       26        +2     
  Lines        2061     2290      +229     
===========================================
+ Hits          342     1291      +949     
+ Misses       1693      904      -789     
- Partials       26       95       +69     

Files Changed	Coverage Δ
controllers/lvmcluster_controller_watches.go	90.32% <ø> (+90.32%)	⬆️
controllers/topolvm_node.go	91.76% <0.00%> (+91.76%)	⬆️
controllers/vgmanager.go	68.00% <0.00%> (+29.70%)	⬆️
pkg/vgmanager/lvm.go	52.03% <ø> (+4.06%)	⬆️
controllers/topolvm_snapshotclass.go	61.22% <14.28%> (+61.22%)	⬆️
controllers/utils.go	45.09% <34.61%> (+5.09%)	⬆️
pkg/vgmanager/vgmanager_controller.go	10.40% <43.10%> (+10.40%)	⬆️
pkg/cluster/leaderelection.go	66.66% <66.66%> (ø)
controllers/lvmcluster_controller.go	60.00% <71.42%> (+60.00%)	⬆️
pkg/cluster/sno.go	72.72% <72.72%> (ø)
... and 3 more

... and 5 files with indirect coverage changes

[jakobmoellerdev]

/test ci-index-lvm-operator-bundle
/test images

[qJkee]

/lgtm

[openshift-ci-robot]

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

In response to this:

This PR introduces concurrent processing for the ensureCreated operations for each resourceManager in the LVMClusterReconciler controller, which significantly improves the overall reconciliation time for the LVMCluster custom resource.

The newly added multierror.go groups multiple errors into a single error, thereby helping to manage multiple errors generated from concurrent ensureCreated calls. This helps the concurrent apply since errors are not cancelled when one call fails, and multiple failures can occur at once.

Moreover, readiness checks have been added after the creation of Deployment and DaemonSet resources in the topolvm_controller.go, topolvm_node.go, and vgmanager.go files respectively.
These checks ensure that the deployment or DaemonSet has been completely initialized and is ready for use.
It also adds verifyDeploymentReadiness and verifyDaemonSetReadiness functions in utils.go to verify the readiness of Deployment and DaemonSet resources respectively.

Note: The checks currently do not get propagated to LVMCluster yet because the LVMCluster State is independant from the Readiness checks done during reconciliation. We will need an API Change to propagate this correctly (in a later change) as otherwise we would set LVMCluster to Degraded/Failed but couldnt show why (because the only status message fields in LVMCluster are for volume group failures, not any of the basic deployed components, and there is no vg present at the time of the readiness check)

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 lvm-operator-e2e-aws

[jakobmoellerdev]

/test lvm-operator-e2e-aws

[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

[jakobmoellerdev]

/test lvm-operator-e2e-aws

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