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

TransportURL: rely on PatchInstance instead of manual Update #217

Merged

Conversation

gibizer
Copy link
Contributor

@gibizer gibizer commented Apr 25, 2024

The transporturl controller did a separate client.Update after
it initialized it status then continued the normal reconciliation. As it
is explicitly updated the status subresource and no finalizer is added
this probably works. However to allow easier reasoning about when the
instance is updated this patch removes the explicit Update call and
instead returns immediately to use the deferred PatchInstance call
as the only place that persists the instance.

@openshift-ci openshift-ci bot requested review from olliewalsh and stuggi April 25, 2024 08:08
@gibizer gibizer marked this pull request as draft April 25, 2024 08:11
@gibizer
Copy link
Contributor Author

gibizer commented Apr 25, 2024

I mixed things up. I need to rethink the reasoning to remove the extra update, but the issue is not as sever as I thought here.

The transporturl  controller did a separate client.Update after
it initialized it status then continued the normal reconciliation. As it
is explicitly updated the status subresource and no finalizer is added
this probably works. However to allow easier reasoning about when the
instance is updated this patch removes the explicit Update call and
instead returns immediately to use the deferred PatchInstance call
as the only place that persists the instance.
@gibizer gibizer force-pushed the remove-double-update branch from 73d2dca to 77fda22 Compare April 25, 2024 08:16
@gibizer gibizer changed the title TransportURL: return imediately when finalizer added TransportURL: rely on PatchInstance instead of manual Update Apr 25, 2024
@gibizer gibizer marked this pull request as ready for review April 25, 2024 08:18
@openshift-ci openshift-ci bot requested a review from dprince April 25, 2024 08:18
Copy link
Contributor

@stuggi stuggi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Copy link
Contributor

openshift-ci bot commented Apr 25, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gibizer, stuggi

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

@gibizer
Copy link
Contributor Author

gibizer commented Apr 26, 2024

recheck
the zuul trigger is missed due to a zuul restart

Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://review.rdoproject.org/zuul/buildset/79fa3a5dbde84ffe9ca0f745cc8c585c

✔️ openstack-k8s-operators-content-provider SUCCESS in 26m 33s
podified-multinode-edpm-deployment-crc RETRY_LIMIT in 3s
cifmw-crc-podified-edpm-baremetal RETRY_LIMIT in 2s

@gibizer
Copy link
Contributor Author

gibizer commented Apr 26, 2024

recheck retry limit

@openshift-merge-bot openshift-merge-bot bot merged commit ba41f87 into openstack-k8s-operators:main Apr 26, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants