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.

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

AzureStackHCIVirtualMachine Deletion Bug fix, tests, mocks and fixes for issues including linting, codeql, tsa, etc #250

Merged

Conversation

PavanNeerudu
Copy link
Contributor

@PavanNeerudu PavanNeerudu commented Jan 22, 2024

AzureStackHCIVirtualMachine Deletion Bug fix, tests, mocks and fixes for issues including linting, codeql, tsa, etc

The initial goal of the PR was to fix an issue in the reconcile deletion loop for azurestackhcivirtualmachine(controllers/azurestackhcimachine_controller.go). The deletion of azurestackhcivirtualmachine was being handled in a bit wrong way where the deltion of this VM was assumed as a synchronous operation which is not true. This is because, when we issue a delete to a custom resource, it has to go through the logic in the reconciliation loop for this specific resource and post that the resource is deleted. This PR updates the implementation by doing the following:

  • If the Get call for the VM results in an err saying that the vm is not present, then we are good(the machine is already deleted).
  • If the Get call for the VM results in other errors, the request is reconciled immediately(a non nil error is send)
  • If we are able to get the machine and the deletion timestamp on the machine is not zero(the delete was issue, but the deletion did not complete), then the resource is requeued after a short amount of time. However, if the deletion timestamp is not nil, we go ahead and issue delete for this resource and requeue post successful operation.

Along, with fixing this bug, this PR does the following:

  • Adds initial testing framework with gingko based tests and mocks.
  • Adds ginkgo based tests covering different scenarios and edge cases for the PR change specific funtion(reconcileVirtualMachineDelete) by using a mock client generated using mockgen for controller-runtime's client.
    image
  • Fixes linting issues(some bugs in the code were found and addressed) by calling lint target in the test and build recipes. Removes the unused cloudtest package. Skipped linting using markers when not sure of the dependencies downstream.
  • Fixes kustomization.yaml by replacing the deprecated feature(patchesStrategicMerge). kustomize edit fix was recursively run against the kustomization.yaml files.
  • Removes external docker image references for the image used in the Dockerfile(replaced with cbl mariner's distroless minimal image), kube-rbac-proxy, etc.
    image
  • Fixes CodeQL and TSA issues failing on the master.
    image
  • Removes the unused versions v1alpha3 and v1alpha4 from the controller-gen calls and updates these CRDs.
  • Enhancements to make recipes of test and manager builder by calling linting, fmt, vet, module cleaning, etc.
  • Pipeline build stage enhancements by calling the test recipe as well as a stage to make sure the go generation doesn't generate any excess files/changes than the ones checked in.

@PavanNeerudu PavanNeerudu added bug Something isn't working enhancement New feature or request labels Jan 22, 2024
@PavanNeerudu PavanNeerudu changed the title AzureStackHCIVirtualMachine Bug fix AzureStackHCIVirtualMachine Deletion Bug fix and fixes for issues including linting, codeql, tsa, etc Jan 22, 2024
@PavanNeerudu PavanNeerudu changed the title AzureStackHCIVirtualMachine Deletion Bug fix and fixes for issues including linting, codeql, tsa, etc AzureStackHCIVirtualMachine Deletion Bug fix and fixes for issues including linting, codeql, tsa, etc with tests and mocks Jan 22, 2024
@PavanNeerudu PavanNeerudu changed the title AzureStackHCIVirtualMachine Deletion Bug fix and fixes for issues including linting, codeql, tsa, etc with tests and mocks AzureStackHCIVirtualMachine Deletion Bug fix and fixes for issues including linting, codeql, tsa along with tests and mocks Jan 22, 2024
@PavanNeerudu PavanNeerudu changed the title AzureStackHCIVirtualMachine Deletion Bug fix and fixes for issues including linting, codeql, tsa along with tests and mocks AzureStackHCIVirtualMachine Deletion Bug fix, tests, mocks and fixes for issues including linting, codeql, tsa, etc Jan 22, 2024
@PavanNeerudu PavanNeerudu marked this pull request as ready for review January 22, 2024 18:15
@pradipd
Copy link
Contributor

pradipd commented Jan 23, 2024

This is great, but, can we break this into 2 pr's?

  1. the fix.
  2. the rest of the test framework

If we need to cherry pick the fix for this PR, it would be very challenging with all the test fraweowkr stuff.

@PavanNeerudu
Copy link
Contributor Author

PavanNeerudu commented Jan 23, 2024

This is great, but, can we break this into 2 pr's?

  1. the fix.
  2. the rest of the test framework

If we need to cherry pick the fix for this PR, it would be very challenging with all the test fraweowkr stuff.

Hi @pradipd . The changes made are kind of reliant on each other(other than things like kustomization, crds, tsa, etc). Do you think once the changes in this PR are approved, can we raise a PR to the release branch with the changes that are actually needed for the fix(just the azurestackhcimachine controller changes).

@zawachte
Copy link
Collaborator

just a few small comments

@PavanNeerudu PavanNeerudu merged commit 8034ac1 into master Jan 25, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants