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

adr: juju-doctor #260

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

adr: juju-doctor #260

wants to merge 3 commits into from

Conversation

MichaelThamm
Copy link
Contributor

Please review

Copy link
Contributor

@sed-i sed-i left a comment

Choose a reason for hiding this comment

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

Good start @MichaelThamm!

  • I think at this point we should reduce the scope of the ADR to "Charm probes"
  • I think the requirements listed are actually the decision itself.

@lucabello lucabello changed the title Juju-doctor ADR adr: juju-doctor Jan 23, 2025
Copy link
Contributor

@lucabello lucabello left a comment

Choose a reason for hiding this comment

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

Thanks for the draft :) Leaving a few comments

I agree with both points Leon made!


Other than running juju-doctor --probe ... --probe ... --model ... etc, we might allow defining a simple yaml config file that defines the CLI arguments; the one-liner could otherwise become too long, and hard to repeat consistently if executed on different machines.

We could even allow said config files to live somewhere remotely (e.g. on a repository) and fetch it, e.g. with juju-doctor --config=https://...)

Just a few ideas :)

`juju-doctor` could work in a similar way by generating/receiving a standardized dump (status.yaml, bundle.yaml, show-unit.yaml) and validating this deployment. The deployments variations (theoretically infinite) that `juju-doctor` needs to validate cannot be maintained solely by the Observability team. This introduces the decentralized probes (a unit-like test) architecture, where the tool can pull probes from remotes which are maintained by the stakeholders of the charm/deployment/probe

## Requirements
1. Probes are runnable without `juju-doctor` (piping from stdin)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not clear on the value of this. What advantage does that provide vs running juju-doctor and pointing to one probe?

Minor thing: note that this is not enforceable on probes written by others, it'd be a convention that others might not follow.

It also prevents us from "sandboxing" the execution in any way, and the responsibility would fall on each individual probe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Edge cases:

  • Multiple juju-status.yaml (e.g. CMRs) as input to probe/doctor
  • CMRs to k8s models need to go through an ingress
    • With juju status --format json we can find CMR info in application-endpoints` key:
  "application-endpoints": {
    "graf": {
	...
    },

Comment on lines +70 to +72
The user of `juju-doctor` is responsible for defining the remote probes required for the current validation and could define:
1. Multiple probe URLs (`--probe probe-1.py --probe probe-2.py`)
2. A directory of probes (run all inside)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add (3), multiple charms for which to fetch probes (--charm grafana-k8s --charm loki-k8s), where juju-doctor could do:

  • from charm name, get the repo
  • from repo, get the probes (under /probes)

Copy link
Contributor

Choose a reason for hiding this comment

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

Until we get to option 5 there's no hope to agree on any of this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants