OCPBUGS-86770: Fix OpenStack identityRef conversion and add e2e test#575
OCPBUGS-86770: Fix OpenStack identityRef conversion and add e2e test#575simkam wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds an ordered e2e test that verifies MAPI-to-CAPI machine synchronization and infra UID stability, and updates the MAPO->CAPO OpenStack conversion to set OpenStackIdentityReference.Type to "Secret". ChangesMachine Synchronization E2E Test
OpenStack Identity Reference Enhancement
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| g.Expect(err).NotTo(HaveOccurred()) | ||
| g.Expect(freshInfraMachine.GetUID()).To(Equal(infraMachineUID), | ||
| "infrastructure machine UID changed — was deleted and recreated") | ||
| }, "30s", "5s").Should(Succeed()) |
There was a problem hiding this comment.
We can't have a 30s wait here. These add up so fast in a test suite.
Let me think about this...
There was a problem hiding this comment.
Locally, without the fix, it actually fails on the first check of the synchronized condition on line 50. It might be enough to detect a problem, and we might drop the rest.
There was a problem hiding this comment.
I think it'll flap, though, so it's non-deterministic. I didn't come up with a better idea, so lets do this.
simkam
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@simkam: This pull request references Jira Issue OCPBUGS-86770, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@simkam: This pull request references Jira Issue OCPBUGS-86770, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mdbooth The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
https://redhat.atlassian.net/browse/OCPBUGS-86770 The MAPI-to-CAPI conversion for OpenStack was not setting the Type field on OpenStackIdentityReference. While the CRD has a kubebuilder default of "Secret", this only applies at the API server level. Programmatic struct construction yields a zero-value empty string, which differs from the stored value. This caused the differ to detect a spec change on every reconciliation, triggering an infinite delete/recreate loop of the OpenStackMachine (infra machines are immutable, so spec changes require delete+recreate). The symptom was the Synchronized condition oscillating between True and False. The fix explicitly sets Type: "Secret" in the conversion function. Add a platform-agnostic e2e test (machine_sync_test.go) that verifies the Synchronized condition reaches True and stays stable via Consistently, catching the oscillation without needing platform-specific assertions. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
simkam
force-pushed
the
openstack-sync-issue
branch
from
1bfa838 to
277ee44
Compare
|
New changes are detected. LGTM label has been removed. |
|
Rebased and the new test skipped on platforms where MachineAPIMigration isn't supported. |
|
/pipeline auto |
|
Pipeline controller notification The |
|
/pipeline required |
|
Scheduling tests matching the |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/retest-required |
|
@simkam: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/retest-required |
3 participants
https://redhat.atlassian.net/browse/OCPBUGS-86770
The MAPI-to-CAPI conversion for OpenStack was not setting the Type field on OpenStackIdentityReference. While the CRD has a kubebuilder default of "Secret", this only applies at the API server level. Programmatic struct construction yields a zero-value empty string, which differs from the stored value. This caused the differ to detect a spec change on every reconciliation, triggering an infinite delete/recreate loop of the OpenStackMachine (infra machines are immutable, so spec changes require delete+recreate). The symptom was the Synchronized condition oscillating between True and False.
The fix explicitly sets Type: "Secret" in the conversion function.
Add a platform-agnostic e2e test (machine_sync_test.go) that verifies the Synchronized condition reaches True and stays stable via Consistently, catching the oscillation without needing platform-specific assertions.
Summary by CodeRabbit
Tests
Bug Fixes