Update models and interface tests for tracing and tempo_cluster to match current state
#174
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tempo_config is now worker_config Set optional fields in JujuTopology Add worker relation to state Make config optional default: None Make juju topology optional Proper test cases for provider provider: Only tests that do validation on existing state assuming statelessness Add required s3 to the test context Provider: convert databags to json json.dumps( on each field, not on whole databag Use assert_relation_data_empty instead of exceptions comment out other provider tests Add required unit data pack dict to string Experiment: using DatabagModel from o11y HA cluster objects Don't json.dumps JujuTopology temp make jujutopology optional and don't include it in test json.dumps on address too try doing json.dumps on each of fields in JujuTopology tracing: drop tempo_ protocols from requested list Add tempo-coordinator and grafana-agent-k8s as tracing v2 providers focus on fixing tempo first Use tempo from branch with tempo-ready service interface-fix branch on all tracing versions tempo-k8s is no longer provider for tracing/v0 and tracing/v1 Uncomment grafana-agent-k8s for tracing/v2 Use branch of grafana-agent-k8s with interface tests support tempo-k8s already has the fix merged just the coordinator for now dumps entire topology Is it just unit and charm_name that was missing? Remove spare s3 relation ignored anyway Custom tester override location too Add Json to schema Ignore exact contents of loki_endpoints I forgot a bracket Enable other tests from tempo_cluster provider interface Enable requirer tests Add config section to relation databags empty json as worker config One field in json Have a basic yaml just the worker to focus debugging json.dumps on pseudo-yaml One test is enough, for clarity Try adding local data add role to app databag juju topology as string Leave local data out - it's the charm that should fill them local data are back remove dumps on role Remove dumps on topology again So we need dumps do we need to specify jujutopology? Dumps juju_topology disable local databags for now is it wrong endpoint naming? this isn't it. add local databags again try mocking topology in charm Add remote_write_endpoints to schema Wrong order of properties focus on requirer again change to cluster-created Pack everything into Json add model config drop model and model_uuid and rename JujuTopology to _Topology does whole tempo_cluster work now? Add cluster-changed test and check non-happy flow juju topology will always be filled Point at right fixture in tempo-coordinator for tracing tests Requested protocols are part of app data remove tests on action that doesn't work point at squashed branches to test they still work Move to main branches of tempo HA repos
|
@benhoyt @tonyandrewmeyer @dimaqq @IronCore864: could one of you have a look at this one? I preferably don't want to approve my own team's PRs. |
|
Main question: what does "to match current state" (PR heading) mean? |
dimaqq
reviewed
Sep 10, 2024
@dimaqq: I have added an explanation in the description of the PR. Hope it explains the changes well enough but please feel free to ask to clarify anything that still seems off. |
This was referenced Sep 20, 2024
|
For the failing tests, I think @canonical/charm-tech need to do some additional work in getting both this repo and pytest-interface-tester to be fully Scenario 7 compatible. @IronCore864 and/or I will work on that today. |
tonyandrewmeyer
approved these changes
Sep 26, 2024
IronCore864
approved these changes
Sep 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates
tracingandtempo_clusterinterface definitions and their respective interface tests. It also:tracing v0interface that is no longer supported by any of the tempo charmstracing v2In order for the tests to pass, this PR requires canonical/tempo-worker-k8s-operator#27 and canonical/tempo-coordinator-k8s-operator#32 to be merged first.
Context
tracinginterface has been under development for the last 2 cycles. As the interface has already stabilised onv2and we've migrated all the charms and documentation to usev2, we dropped support for oldertracingversions in tempo charm: Clean up tracing databag in v2 tempo-k8s-operator#108 ; we also made sure that any user of the oldertracinginterfaces has migrated to the new one.tempo_clusteris a new interface that we're implementing in the in-development Tempo HA charms: https://github.com/canonical/tempo-coordinator-k8s-operator and https://github.com/canonical/tempo-worker-k8s-operator . This interface is responsible for passing data between coordinator and worker charms. An early version of this interface was pushed out intempo_clusterinterface #158, however since that push we have aligned the three sets of charms following the same coordinator-worker pattern (Tempo HA, Loki HA and Mimir HA) to use common code objects (see https://github.com/canonical/observability/blob/main/decision-records/2024-06-14--coordinated-multirole-workers.md ) for the decision spec. Part of the change was switching to a shared databag model, usingworker_configinstead of the dedicated config names for each of the solutions.