Skip to content

🐛 [release-0.11] fix: fix panic on server deletion #2685

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
Merged
merged 3 commits into from
Sep 12, 2025
Merged

🐛 [release-0.11] fix: fix panic on server deletion #2685

merged 3 commits into from
Sep 12, 2025

Conversation

@okozachenko1203
Copy link
Contributor

@okozachenko1203 okozachenko1203 commented Sep 4, 2025 •
edited
Loading

What this PR does / why we need it:
This PR is just a backport of these.
#2475
#2478

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

  1. Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

  • squashed commits
  • if necessary:
    • includes documentation
    • adds unit tests

/hold

@mdbooth @okozachenko1203
Fix panic when OpenStack server is deleted by an external agent
9feb382 
The panic happened because we did not correctly handle that
GetInstanceStatus returns a nil server if the server does not exist,
rather than a 404 error.

We had the same oversight in the OpenStackServer controller. It looks
like this would have resulted in recreating the server.
@mdbooth @okozachenko1203
Don't set OSMachine Ready until all config is complete
38bdee3 
With this change, an OpenStackMachine will not be marked Ready until it
has been completely initialised, including setting its Addresses and
adding it to the APIServer loadbalancer if required.

Also fix a missing early return when the instance has been unexpectedly
deleted.
@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 4, 2025
@netlify
Copy link

netlify bot commented Sep 4, 2025 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name Link
🔨 Latest commit 46194f2
🔍 Latest deploy log https://app.netlify.com/projects/kubernetes-sigs-cluster-api-openstack/deploys/68c21662f1ecb200088b7ef4
😎 Deploy Preview https://deploy-preview-2685--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 4, 2025
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Sep 4, 2025

Hi @okozachenko1203. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 4, 2025
@mnaser
Copy link
Contributor

mnaser commented Sep 8, 2025

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 8, 2025
@lentzi90
Copy link
Contributor

lentzi90 commented Sep 10, 2025

Could you add some explanation to the description of this pull request?
Why is it targeting release-0.11 instead of main? Is there some issue that describes what this fixes?

@okozachenko1203
Copy link
Contributor Author

okozachenko1203 commented Sep 10, 2025 •
edited
Loading

Could you add some explanation to the description of this pull request? Why is it targeting release-0.11 instead of main? Is there some issue that describes what this fixes?
This PR is just a backport of these.
#2475
#2478

@okozachenko1203 okozachenko1203 force-pushed the release-0.11-fix-panic-on-server-deletion branch from 5cba124 to 8d94856 Compare September 11, 2025 00:09
@okozachenko1203
changes for 0.11 release
46194f2 
Signed-off-by: okozachenko1203 <[email protected]>
@okozachenko1203 okozachenko1203 force-pushed the release-0.11-fix-panic-on-server-deletion branch from 8d94856 to 46194f2 Compare September 11, 2025 00:22
lentzi90
lentzi90 reviewed Sep 11, 2025
View reviewed changes
Copy link
Contributor

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

Thanks for the explanation! Now it makes more sense to me.
/approve
/cc @mdbooth
/retitle 🐛 [release-0.11] fix: fix panic on server deletion

@k8s-ci-robot k8s-ci-robot changed the title [release-0.11] fix: fix panic on server deletion 🐛 [release-0.11] fix: fix panic on server deletion Sep 11, 2025
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Sep 11, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lentzi90

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 11, 2025
@mnaser
Copy link
Contributor

mnaser commented Sep 11, 2025

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 11, 2025
@lentzi90
Copy link
Contributor

lentzi90 commented Sep 12, 2025

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 12, 2025
@k8s-ci-robot k8s-ci-robot merged commit fc4946c into kubernetes-sigs:release-0.11 Sep 12, 2025
9 checks passed
@github-project-automation github-project-automation bot moved this from Inbox to Done in CAPO Roadmap Sep 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lentzi90 lentzi90 lentzi90 left review comments

@mdbooth mdbooth Awaiting requested review from mdbooth

Assignees

@mnaser mnaser

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

CAPO Roadmap
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@okozachenko1203 @k8s-ci-robot @mnaser @lentzi90 @mdbooth