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

🌱 improve deploy_observability for Tilt #6079

Merged
merged 1 commit into from
Feb 15, 2022
Merged

🌱 improve deploy_observability for Tilt #6079

merged 1 commit into from
Feb 15, 2022

Conversation

fabriziopandini
Copy link
Member

What this PR does / why we need it:
This PR makes tilt_setting options more flexible by allowing to specify which observability tool do you want (vs all or nothing)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 8, 2022
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 8, 2022
@fabriziopandini fabriziopandini force-pushed the improve-deploy_observability-for-Tilt branch from eaa1def to a754ca3 Compare February 8, 2022 12:54
@sbueringer
Copy link
Member

Tested it locally, works as expected!

Thx!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 8, 2022
killianmuldoon
killianmuldoon approved these changes Feb 8, 2022
View reviewed changes
Copy link
Contributor

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

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

Working for me locally

/lgtm

ykakarap
ykakarap reviewed Feb 8, 2022
View reviewed changes
Copy link
Contributor

@ykakarap ykakarap left a comment

Choose a reason for hiding this comment

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

A nice change. 🙂

Just nits.

docs/book/src/developer/tilt.md Outdated Show resolved Hide resolved
Tiltfile
@@ -322,11 +322,17 @@ def deploy_provider_crds():
)

def deploy_observability():
k8s_yaml(read_file("./.tiltbuild/yaml/observability.tools.yaml"))
if "promtail" in settings.get("deploy_observability", []):
k8s_yaml(read_file("./.tiltbuild/yaml/promtail.observability.yaml"), allow_duplicates = True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a review but out of curiosity: why are we doing allow_duplicates=true?

Copy link
Member Author

Choose a reason for hiding this comment

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

After installing promtail helm chart I was getting errors due to duplicated objects, this was the suggested fix; quoting the Tilt documentation:

allow_duplicates ( bool ) – If you try to register the same Kubernetes resource twice, this function will assume this is a mistake and emit an error. Set allow_duplicates=True to allow duplicates. There are some Helm charts that have duplicate resources for esoteric reasons.

😄 for esoteric reasons

Copy link
Member

Choose a reason for hiding this comment

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

The Namespace resource is duplicated between all three observability tools, so that could explain the issues.

@fabriziopandini fabriziopandini force-pushed the improve-deploy_observability-for-Tilt branch from a754ca3 to b6efec5 Compare February 8, 2022 21:20
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 8, 2022
@sbueringer
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2022
@sbueringer sbueringer mentioned this pull request Feb 9, 2022
Merged
@ykakarap
Copy link
Contributor

ykakarap commented Feb 9, 2022

/lgtm

@sbueringer
Copy link
Member

@fabriziopandini
WDYT can we merge this PR so that #6117 can build on top?

(cc @apricote)

@apricote
Copy link
Member

There is a slight dependency of grafana on loki, as the latter is configured as a source in the former. This won't break anything, but the source will still be visible (and broken) in grafana if loki is disabled.

The same situation will occur with Prometheus

@sbueringer
Copy link
Member

sbueringer commented Feb 15, 2022 •
edited
Loading

There is a slight dependency of grafana on loki, as the latter is configured as a source in the former. This won't break anything, but the source will still be visible (and broken) in grafana if loki is disabled.

The same situation will occur with Prometheus

Very good point.

I think we try to limit the dependencies in the Tiltfile to the more problematic ones, like promtail=>loki because promtail without loki doesn't make sense at all.

@apricote apricote mentioned this pull request Feb 15, 2022
Merged
@fabriziopandini
Copy link
Member Author

There is a slight dependency of grafana on loki, as the latter is configured as a source in the former. This won't break > > > > anything, but the source will still be visible (and broken) in grafana if loki is disabled.

The same situation will occur with Prometheus

That's true, but what I'm trying with this PR is to move us away from all or nothing, because installing too many tools makes the developer flow slow, requires unnecessary resources etc.
We can eventually iterate later and make dependencies among tools managed in a more developer-friendly way (e.g. erroring if we recognize missing dependency, or automatically adding missing dependencies)

vincepri
vincepri approved these changes Feb 15, 2022
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 15, 2022
@k8s-ci-robot k8s-ci-robot merged commit 8ea899a into kubernetes-sigs:main Feb 15, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.2 milestone Feb 15, 2022
@fabriziopandini fabriziopandini deleted the improve-deploy_observability-for-Tilt branch March 1, 2022 09:49
@prometherion prometherion mentioned this pull request Aug 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ykakarap ykakarap ykakarap left review comments

@apricote apricote apricote left review comments

@killianmuldoon killianmuldoon killianmuldoon approved these changes

@vincepri vincepri vincepri approved these changes

@sbueringer sbueringer Awaiting requested review from sbueringer

@JoelSpeed JoelSpeed Awaiting requested review from JoelSpeed

@stmcginnis stmcginnis Awaiting requested review from stmcginnis

Assignees

@sbueringer sbueringer

@ykakarap ykakarap

@killianmuldoon killianmuldoon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.
7 participants
@fabriziopandini @sbueringer @ykakarap @apricote @k8s-ci-robot @vincepri @killianmuldoon