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.

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

[OSPRH-11073] Add mysqld_exporter #551

Merged
merged 3 commits into from
Dec 11, 2024
Merged

[OSPRH-11073] Add mysqld_exporter #551

merged 3 commits into from
Dec 11, 2024

Conversation

vyzigold
Copy link
Contributor

This adds mysqld_exporter to the Ceilometer resource.

What I'd really appreciate some feedback:

  • Where exactly to put the status of mysqld_exporter in the CRD

Whats missing:

  • ScrapeConfig - followup PR
  • API, TLS and openstackversion support in openstack-operator - PR when this is merged

@softwarefactory-project-zuul softwarefactory-project-zuul
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://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/4e70fbea8a6b4e5dacab425480125be5

✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 56m 00s
❌ podified-multinode-edpm-deployment-crc FAILURE in 1h 05m 03s
❌ telemetry-operator-multinode-autoscaling-tempest FAILURE in 1h 36m 57s
❌ telemetry-operator-multinode-default-telemetry FAILURE in 1h 05m 50s
❌ functional-tests-on-osp18 FAILURE in 1h 36m 30s (non-voting)
❌ functional-logging-tests-osp18 FAILURE in 1h 10m 05s (non-voting)
❌ functional-graphing-tests-osp18 FAILURE in 1h 37m 35s (non-voting)
❌ functional-metric-verification-tests-osp18 FAILURE in 1h 39m 04s (non-voting)

@vyzigold vyzigold force-pushed the mysqld_exporter_squashed branch from 139ba92 to 8237b3f Compare November 28, 2024 10:41
@vyzigold
Add mysqld_exporter
8f9673e 
This adds mysqld_exporter to the Ceilometer resource.

What I'd really appreciate some feedback:
 - Where exactly to put the status of mysqld_exporter in the CRD

Whats missing:
 - ScrapeConfig - followup PR
 - API, TLS and openstackversion support in openstack-operator - PR when this is merged
@vyzigold vyzigold force-pushed the mysqld_exporter_squashed branch from 8237b3f to 8f9673e Compare November 28, 2024 16:55
@vyzigold
Copy link
Contributor Author

/retest

1 similar comment
@jlarriba
Copy link
Collaborator

/retest

jlarriba
jlarriba approved these changes Nov 29, 2024
View reviewed changes
Copy link
Collaborator

@jlarriba jlarriba left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Nov 29, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 29, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlarriba, vyzigold

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

@vyzigold
Copy link
Contributor Author

/hold
This adds new api fields. AFAIK this should be backwards compatible, but we have a pre-commit related PR, which will enable automatic checking of CRD backwards compatibility coming soon in: #549 . So lets wait until that PR merges so that this PR can go through that check.

@vyzigold
Copy link
Contributor Author

/unhold

@vyzigold
Copy link
Contributor Author

/retest

@vyzigold vyzigold changed the title Add mysqld_exporter [OSPRH-11073] Add mysqld_exporter Nov 29, 2024
@vyzigold
Copy link
Contributor Author

vyzigold commented Dec 2, 2024

The new crd-schema check is complaining about a few fields. IMO they're not a big deal, they follow established patterns from this and other openstack operators. Unfortunately there isn't a straightforward way to fix some of the fields and comply with the checker at the moment. There is a slack discussion about the check ongoing.

@vyzigold
Copy link
Contributor Author

vyzigold commented Dec 5, 2024

/hold I'll rewrite the status portion of the API after a slack discussion

@openshift-ci openshift-ci bot removed the lgtm label Dec 6, 2024
@vyzigold
Copy link
Contributor Author

vyzigold commented Dec 9, 2024

/unhold

@vyzigold
Copy link
Contributor Author

vyzigold commented Dec 9, 2024

The crd-checker fails with the followin errors:

ERROR: "NoBools": crd/ceilometers.telemetry.openstack.org version/v1beta1 field/^.spec.mysqldExporterEnabled may not be a boolean
ERROR: "NoMaps": crd/ceilometers.telemetry.openstack.org version/v1beta1 field/^.status.mysqldExporterHash may not be a map

Following the crd-checker suggestions would break already established patterns throughout all of the operators, while not providing any additional value. IMO we should just ignore the errors. This should be fixed when (and if) some larger oko effort to change all these values across all of the operators happens.

@jlarriba
Copy link
Collaborator

/override precommit-check

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 10, 2024

@jlarriba: /override requires failed status contexts, check run or a prowjob name to operate on.
The following unknown contexts/checkruns were given:

  • precommit-check

Only the following failed contexts/checkruns were expected:

  • ci/prow/images
  • ci/prow/precommit-check
  • ci/prow/telemetry-operator-build-deploy
  • ci/prow/telemetry-operator-build-deploy-kuttl
  • pull-ci-openstack-k8s-operators-telemetry-operator-18.0-fr1-images
  • pull-ci-openstack-k8s-operators-telemetry-operator-18.0-fr1-precommit-check
  • pull-ci-openstack-k8s-operators-telemetry-operator-18.0-fr1-telemetry-operator-build-deploy
  • pull-ci-openstack-k8s-operators-telemetry-operator-18.0-fr1-telemetry-operator-build-deploy-kuttl
  • rdoproject.org/github-check
  • tide

If you are trying to override a checkrun that has a space in it, you must put a double quote on the context.

In response to this:

/override precommit-check

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.

@jlarriba
Copy link
Collaborator

/override ci/prow/precommit-check

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 10, 2024

@jlarriba: Overrode contexts on behalf of jlarriba: ci/prow/precommit-check

In response to this:

/override ci/prow/precommit-check

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.

@jlarriba
Copy link
Collaborator

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Dec 10, 2024
@vyzigold
Copy link
Contributor Author

/retest
error: the server doesn't have a resource type "openstackversion" Doesn't seem related.

@vyzigold
Copy link
Contributor Author

/retest
I believe this was supposed to fix the issue: openshift/release#59733

@openshift-merge-bot openshift-merge-bot bot merged commit 5fc45e1 into openstack-k8s-operators:main Dec 11, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlarriba jlarriba jlarriba approved these changes

@elfiesmelfie elfiesmelfie Awaiting requested review from elfiesmelfie

@yadneshk yadneshk Awaiting requested review from yadneshk

Assignees

@jlarriba jlarriba

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vyzigold @jlarriba