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

feat: Add a minimal prometheus server manifest #4687

Merged
merged 5 commits into from Dec 16, 2020

Conversation

simster7
Copy link
Member

@simster7 simster7 commented Dec 9, 2020

Signed-off-by: Simon Behar simbeh7@gmail.com

Checklist:

@simster7 simster7 marked this pull request as ready for review December 10, 2020 19:42
Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

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

  • Could you attach some screenshots? E.g.
    • argo_workflows_count changing over time
    • argo_workflows_queue_latency_bucket showing latency for workflows?
    • otherL
  • Could you do a hands-on with me and Bala?

manifests/prometheus.yaml Outdated Show resolved Hide resolved
manifests/prometheus.yaml Outdated Show resolved Hide resolved
manifests/prometheus.yaml Outdated Show resolved Hide resolved
@simster7 simster7 requested a review from alexec December 11, 2020 18:17
@simster7
Copy link
Member Author

Could you do a hands-on with me and Bala?

Would be happy to. Today at standup?

@alexec
Copy link
Contributor

alexec commented Dec 11, 2020

Stand-up demo - yes please. What would be neat? make start RUN_MODE=kubernetes PROFILE=prometheus and it just works.

@simster7
Copy link
Member Author

What would be neat? make start RUN_MODE=kubernetes PROFILE=prometheus and it just works.

Will have this ready by standup time

Signed-off-by: Simon Behar <simbeh7@gmail.com>
Makefile Show resolved Hide resolved
Signed-off-by: Simon Behar <simbeh7@gmail.com>
Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

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

can we see a demo at stand-up please?

manifests/quick-start/base/prometheus/base/prometheus.yaml Outdated Show resolved Hide resolved
hack/procfiles/Procfile-prometheus Outdated Show resolved Hide resolved
Signed-off-by: Simon Behar <simbeh7@gmail.com>
Makefile Outdated
ifeq ($(PROFILE),prometheus)
kubectl -n $(KUBE_NAMESPACE) scale deploy/prometheus --replicas 1
endif
kubectl -n $(KUBE_NAMESPACE) wait --for=condition=Ready pod --all -l app --timeout 1m || 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.

In my experience kubectl wait hangs/gets stuck even after all pods are ready (which happens after around 30 seconds). Confirmed this by running the same command in another terminal window while it is stuck, and in that case the command completes successfully.

We still want to have a wait before we do port-forwarding, but we don't want this flakey command to kill start up, so I lowered the timeout and added || true to ensure this flakey command doesn't stop the build. If the pods actually aren't ready, the next command will fail anyway (which in my experience does not happen).

Copy link
Contributor

Choose a reason for hiding this comment

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

I've noted this is flakey too. Could we make the wait specific? E.g.

kubectl -n $(KUBE_NAMESPACE) wait --for=condition=Ready pod -l app=prometheus

Copy link
Contributor

Choose a reason for hiding this comment

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

might need a short sleep before hand

Copy link
Member Author

Choose a reason for hiding this comment

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

Added sleep

@@ -3,12 +3,14 @@ kind: Kustomization

resources:
- ../../../../manifests/quick-start/mysql
- ../../../../manifests/quick-start/base/prometheus
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this will work, because RUN!=kubernetes typically , I think you only need to add to PROFILE=prometheus

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing

Makefile Show resolved Hide resolved
Signed-off-by: Simon Behar <simbeh7@gmail.com>
Makefile Outdated Show resolved Hide resolved
test/e2e/manifests/mixins/prometheus-deployment.yaml Outdated Show resolved Hide resolved
Signed-off-by: Simon Behar <simbeh7@gmail.com>
@simster7
Copy link
Member Author

@alexec Ready again

@simster7 simster7 merged commit 9c4d735 into argoproj:master Dec 16, 2020
@simster7 simster7 mentioned this pull request Dec 17, 2020
9 tasks
@simster7 simster7 mentioned this pull request Jan 4, 2021
21 tasks
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.

None yet

2 participants