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

Move to VerifySecret when checking the ctlplane secret, and other code consolidation #437

Merged

Conversation

ASBishop
Copy link
Contributor

@ASBishop ASBishop commented Sep 4, 2024

This PR consists of 3 commits:

  1. Steals a commit from the not-yet-merged PR Improve logs #365, which defines variables for durations and requeue. The other PR needs to be rebased, and that commit can be dropped if this patch merges first.
  2. A new verifyServiceSecret() function ensures cinder services don't get restarted when something unrelated in the ctlplane secret changes.
  3. A new verifyConfigSecrets() function consolidates code that records the hash of the TransportURL secret and various service configuration secrets.

See also openstack-k8s-operators/manila-operator#324

Akrog and others added 3 commits September 3, 2024 11:38
We have a number of places where we are using 5 seconds durations, 10
seconds duration, and requeue after 10 seconds.

These are all "magic numbers" spread all over the operator code.

This patch creates 2 constants and 1 variable (because golang doesn't
allow it to be defined as a constant) for these so that they can be used
throughout the code having defined the "magic numbers" in a single
location.
Instead of recording a hash of the entire ctlplane secret, a new
function is introduced that uses lib-common's VerifySecret() to
verify and return the hash of only the cinder service's portion
of the secret. This avoids restarting all cinder services when
an unrelated portion of the ctlplane secret changes.

This change follows a similar manila PR [1].

[1] openstack-k8s-operators/manila-operator#324

Jira: OSPRH-8192
Replace multiple calls to collect hashes of the Transport URL
secret and other service configuration secrets with a new
verifyConfigSecrets() function that can be shared by all
cinder services. This also eliminates the need for each service
to provide its own getSecret function.

This change continues to mimic code in manila's PR [1].

[1] openstack-k8s-operators/manila-operator#324
@ASBishop
Copy link
Contributor Author

ASBishop commented Sep 5, 2024

/retest

@abays
Copy link
Contributor

abays commented Sep 6, 2024

/test cinder-operator-build-deploy-kuttl

@abays
Copy link
Contributor

abays commented Sep 6, 2024

@ASBishop: 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/cinder-operator-build-deploy-kuttl 8509ddc link true /test cinder-operator-build-deploy-kuttl

Full PR test history. Your PR dashboard.

error: build error: Failed to push image: trying to reuse blob sha256:c0ca389759b542396e6b30afcb100a3823d3b343bc847eee7f60d609a106f674 at destination: pinging container registry quay.rdoproject.org: Get "https://quay.rdoproject.org/v2/": dial tcp 38.129.56.158:443: i/o timeout

/retest

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 Sep 9, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ASBishop, 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

@openshift-merge-bot openshift-merge-bot bot merged commit 5d48463 into openstack-k8s-operators:main Sep 9, 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.

4 participants