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

Fix VerifySecret so it properly requeues when necessary #602

Merged

Conversation

abays
Copy link
Contributor

@abays abays commented Aug 8, 2024

We've come to realize three things recently:

  1. Returning an error to the reconciler along with a RequeueAfter will cause the reconciler to abandon the explicit requeue and only consider the error [1].
  2. The VerifySecret function in lib-common was returning an error along with a requeue, thus contributing to the problem listed in item 1. [2]
  3. Also, the error VerifySecret was returning was not a "not found" error, but a generic k8s error -- so it wouldn't be properly handled by callers looking for a "not found" error. [3]

Now VerifySecret only returns a non-empty ctrl.Result{} if the Secret is missing, so let's handle that case in a separate if-clause. [4]

[1] openstack-k8s-operators/keystone-operator#459
[2] https://github.com/openstack-k8s-operators/lib-common/blob/ad3edb6e67ab738cfdbf4e2e78ac403784a08ab7/modules/common/secret/secret.go#L423-L425
[3] https://github.com/openstack-k8s-operators/lib-common/blob/ad3edb6e67ab738cfdbf4e2e78ac403784a08ab7/modules/common/secret/secret.go#L425
[4] https://github.com/openstack-k8s-operators/lib-common/blob/579da98fa7a62b70b43cdba92e82d6497afe8835/modules/common/secret/secret.go#L425-L427

@abays abays requested review from fmount and stuggi August 8, 2024 18:05
@openshift-ci openshift-ci bot added the approved label Aug 8, 2024
@abays
Copy link
Contributor Author

abays commented Aug 8, 2024

@abays: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:
Test name Commit Details Required Rerun command
ci/prow/glance-operator-build-deploy-kuttl 6e4343d link true /test glance-operator-build-deploy-kuttl

Full PR test history. Your PR dashboard.


Removing debug pod ...
error: unable to upgrade connection: container container-00 not found in pod oko-19-629pf-master-2-debug-wcjf7_openstack
make: *** [Makefile:551: crc_storage] Error 1
+ n=3
+ ((  n >= retries  ))
+ echo 'Failed to run '\''make crc_storage'\'' target. Aborting'
Failed to run 'make crc_storage' target. Aborting

/test glance-operator-build-deploy-kuttl

Copy link
Contributor

@fmount fmount 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 Aug 8, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abays, fmount

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

@fmount
Copy link
Contributor

fmount commented Aug 8, 2024

thank you @abays for explaining the problem!

@openshift-merge-bot openshift-merge-bot bot merged commit 6fd33f8 into openstack-k8s-operators:main Aug 8, 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