Skip to content

metrics: Add metrics config test for Hubble. #34325

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
Merged
merged 1 commit into from
Sep 6, 2024
Merged

metrics: Add metrics config test for Hubble. #34325

merged 1 commit into from
Sep 6, 2024

Conversation

rectified95
Copy link
Contributor

@rectified95 rectified95 commented Aug 12, 2024 •
edited
Loading

Starts a standalone Hubble server and passes a metrics config, then fetches from the HTTP /metrics endpoint for validation.
Also splits metrics server creation and starting.

Checklist

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 12, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Aug 12, 2024
chancez
chancez reviewed Aug 12, 2024
View reviewed changes
@rectified95 rectified95 marked this pull request as ready for review August 13, 2024 18:44
@rectified95 rectified95 requested review from a team as code owners August 13, 2024 18:44
@rectified95 rectified95 requested a review from kaworu August 13, 2024 18:44
@rectified95 rectified95 force-pushed the igklemen/metric_test branch from 5edb4cf to 6e2d4c5 Compare August 13, 2024 18:50
@chancez
Copy link
Contributor

chancez commented Aug 13, 2024

/test

2 similar comments
@chancez
Copy link
Contributor

chancez commented Aug 13, 2024

/test

@chancez
Copy link
Contributor

chancez commented Aug 13, 2024

/test

@rectified95 rectified95 force-pushed the igklemen/metric_test branch from 6e2d4c5 to 2511336 Compare August 14, 2024 01:25
@rectified95 rectified95 requested a review from a team as a code owner August 14, 2024 01:25
@rectified95 rectified95 requested a review from marseel August 14, 2024 01:25
@rectified95
Copy link
Contributor Author

Fixed small issue in go.mod that came up in CI

@rolinh rolinh added release-note/ci This PR makes changes to the CI. sig/hubble labels Aug 14, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Aug 14, 2024
rolinh
rolinh reviewed Aug 14, 2024
View reviewed changes
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

More tests, I like it, thanks for your contribution! As Chance already pointed out, I'm not really fond of time.Sleep in a test either as I worry it might flake. I assume the alternative would involve too many changes?

kaworu
kaworu requested changes Aug 14, 2024
View reviewed changes
Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Hi @rectified95 👋 and thanks for the PR!

Overall LGTM, but I'd really like to avoid introducing a new potential flake in testing.

@rectified95 rectified95 marked this pull request as draft August 15, 2024 22:20
@rectified95 rectified95 marked this pull request as ready for review August 15, 2024 22:43
@rectified95 rectified95 requested review from kaworu and chancez August 15, 2024 22:43
@rectified95
Copy link
Contributor Author

What is Cilium's practice for resolving merge conflicts for ease of PR review:

  • locally rebase on top of main and force-push, or
  • use Github's "Resolve conflicts" button, which creates a merge commit, and then a "squash merge" at the end?

@kaworu
Copy link
Member

kaworu commented Aug 19, 2024

What is Cilium's practice for resolving merge conflicts for ease of PR review:

Good question, it's indeed unclear in our documentation to thanks for pointing it out!

  • locally rebase on top of main and force-push, or

That's how I do it. If I'm addressing review comments I try to force-push twice (especially for complicated PRs): first the rebase on top of main, and then a second time with the changes I made. That way GitHub will show two force-push and reviewer can click on the second one to see the changes made since last time they've seen the PR (without the rebase changes which are "noisy" and rarely related to the PR). If the conflict resolution is non trivial and affect reviewed code, I'll then add a comment on GitHub to clarify how the conflict was solved.

  • use Github's "Resolve conflicts" button, which creates a merge commit, and then a "squash merge" at the end?

I've never used this method, I think it could work as long as we're ending with a clean linear commit history. Double check that the GitHub UI doesn't scrap the Developer’s Certificate of Origin (i.e. Signed-off-by) from the commit message.

chancez
chancez reviewed Aug 19, 2024
View reviewed changes
@rectified95
Copy link
Contributor Author

@kaworu Done! Thank you very much.

@gandro gandro removed the request for review from kaworu September 3, 2024 08:32
@gandro
Copy link
Member

gandro commented Sep 3, 2024

/test

@chancez
Copy link
Contributor

chancez commented Sep 3, 2024

/test

@rectified95
Copy link
Contributor Author

One flake left after last rebase - can we re-run and hopefully merge today? 😄
Conformance EKS (ci-eks) https://github.com/cilium/cilium/actions/runs/10689756908/job/29632639804

@chancez
Copy link
Contributor

chancez commented Sep 4, 2024

/test

@marseel
Copy link
Contributor

marseel commented Sep 5, 2024 •
edited
Loading

I've triggered "Conformance Ginkgo (ci-ginkgo)" only as it was the only one failing.
I wonder though why a test by @chancez triggered all tests to be rerun, I thought we changed that to rerun only failed tests 🤔

@chancez
Copy link
Contributor

chancez commented Sep 5, 2024

I've triggered "Conformance Ginkgo (ci-ginkgo)" only as it was the only one failing. I wonder though why a test by @chancez triggered all tests to be rerun, I thought we changed that to rerun only failed tests 🤔

That's what I thought too 🙃

@rectified95
Copy link
Contributor Author

rectified95 commented Sep 5, 2024 •
edited
Loading

I've triggered "Conformance Ginkgo (ci-ginkgo)" only as it was the only one failing. I wonder though why a test by @chancez triggered all tests to be rerun, I thought we changed that to rerun only failed tests 🤔

That's what I thought too 🙃

@chancez Looks like you'd need to run /ci-ginkgo while /test runs the full suite: https://github.com/cilium/cilium/blob/78096f8674e87df509dbd4cd5c839ed067166808/.github/ariane-config.yaml#L5C1-L26C39

@rectified95
Copy link
Contributor Author

Created two issues for these flakes - the existing closed ones happened in the same test but the errors were different:
#34717
#34716

@rectified95
metrics: Add metrics config test for Hubble.
01cb718 
Starts a standalone Hubble server and passes a metrics config,
then fetches from the HTTP /metrics endpoint for validation.

Signed-off-by: Igor Klemenski <igor.klemenski@microsoft.com>
auto-merge was automatically disabled September 6, 2024 17:10

Head branch was pushed to by a user without write access

@rectified95
Copy link
Contributor Author

Rebased on main to include the removal of unstable required workflows.

@marseel
Copy link
Contributor

marseel commented Sep 6, 2024

/test

@rectified95
Copy link
Contributor Author

Hit an existing flake:
Conformance E2E #34558

And one in ci-integration (github.com/cilium/cilium/pkg/identity/cache) without a specific failure indication.

@chancez / @marseel Could we re-trigger just these two?

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 6, 2024
@marseel
Copy link
Contributor

marseel commented Sep 6, 2024

Full success, thanks for patience :)

@joestringer joestringer added this pull request to the merge queue Sep 6, 2024
Merged via the queue into cilium:main with commit 7c4d0ed Sep 6, 2024
65 checks passed
@rectified95
Copy link
Contributor Author

Thanks to everyone who helped move this along to completion!!

@joestringer
Copy link
Member

Thanks for contributing :)

@rectified95 rectified95 mentioned this pull request Sep 13, 2024
Merged
8 tasks
@rectified95 rectified95 mentioned this pull request Oct 2, 2024
Merged
@rectified95 rectified95 mentioned this pull request Oct 10, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@kaworu kaworu kaworu approved these changes

@rolinh rolinh rolinh approved these changes

@chancez chancez chancez approved these changes

@marseel marseel marseel approved these changes

Assignees
No one assigned
Labels
kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@rectified95 @chancez @kaworu @joestringer @gandro @marseel @rolinh