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

Add tests for hubble metrics handlers #22518

Merged

Conversation

marqc
Copy link
Contributor

@marqc marqc commented Dec 2, 2022

PortDistributionHandler and TcpHandler.

Signed-off-by: Marek Chodor mchodor@google.com

Please ensure your pull request adheres to the following guidelines:

  • 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.
  • Thanks for contributing!

Add tests for hubble metrics handlers: DropHandler, PortDistributionHandler and TcpHandler.

@marqc marqc requested review from a team as code owners December 2, 2022 13:15
@marqc marqc requested a review from kaworu December 2, 2022 13:15
@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 Dec 2, 2022
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Dec 2, 2022
@rolinh rolinh added area/metrics Impacts statistics / metrics gathering, eg via Prometheus. sig/hubble Impacts hubble server or relay release-note/misc This PR makes changes that have no direct user impact. labels Dec 2, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 2, 2022
@kaworu kaworu requested a review from chancez December 2, 2022 14:14
Copy link
Contributor

@chancez chancez left a comment

Choose a reason for hiding this comment

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

This is great, I'm really happy to see tests for the metrics, we really need these. I added few comments above, but in all the tests, there's no assertions against the actual metric values, so let's add that everywhere we're expecting a non-empty series.

@marqc marqc force-pushed the add_missing_hubble_metric_handler_tests branch 4 times, most recently from e2e353d to bd2a2a6 Compare December 5, 2022 14:12
@marqc marqc requested review from chancez and removed request for kaworu December 5, 2022 14:16
@maintainer-s-little-helper
Copy link

Commit f4a279c9e272ba01ef92bbcbd43607a9d5ce66bf does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Dec 5, 2022
@marqc marqc force-pushed the add_missing_hubble_metric_handler_tests branch from f4a279c to c44028a Compare December 5, 2022 16:58
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Dec 5, 2022
@chancez
Copy link
Contributor

chancez commented Dec 5, 2022

This looks good. Can you fix your commits and make sure everything passes the initial CI checks?

@marqc marqc force-pushed the add_missing_hubble_metric_handler_tests branch 2 times, most recently from 7363f8f to 88503aa Compare December 6, 2022 14:31
Copy link
Contributor

@chancez chancez left a comment

Choose a reason for hiding this comment

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

New tests look good to me.

@marqc marqc force-pushed the add_missing_hubble_metric_handler_tests branch from 88503aa to d44ec07 Compare December 7, 2022 11:28
@chancez
Copy link
Contributor

chancez commented Dec 7, 2022

You'll need to try and shorten the commit message.

Error: ERROR:CUSTOM: Please avoid long commit subjects (max: 75, found: 91)
42

@marqc marqc force-pushed the add_missing_hubble_metric_handler_tests branch from d44ec07 to ff8365e Compare December 9, 2022 09:41
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.

Thank you for the contribution @marqc patch LGTM!

@aanm
Copy link
Member

aanm commented Dec 10, 2022

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sAgentPolicyTest Basic Test TLS policy

Failure Output

FAIL: Cannot connect from "app2-7fd4ddfc6-9gqv5" to 'https://www.lyft.com:443/privacy'

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

@marqc marqc force-pushed the add_missing_hubble_metric_handler_tests branch from ff8365e to a4cc061 Compare December 12, 2022 14:18
@rolinh
Copy link
Member

rolinh commented Dec 12, 2022

Marking for v1.13 backport. This PR only adds unit tests to Hubble metrics and better test coverage is better overall in case of backport (assuming tests aren't flaky). As suggested by @gandro, I ran the test 1000 times and they always succeeded so I take it they're not flaky.

$ go test -count 1000 -race ./...
ok      github.com/cilium/cilium/pkg/hubble/metrics     0.081s
ok      github.com/cilium/cilium/pkg/hubble/metrics/api 1.291s
?       github.com/cilium/cilium/pkg/hubble/metrics/dns [no test files]
ok      github.com/cilium/cilium/pkg/hubble/metrics/drop        0.514s
ok      github.com/cilium/cilium/pkg/hubble/metrics/flow        0.665s
ok      github.com/cilium/cilium/pkg/hubble/metrics/flows-to-world      2.506s
ok      github.com/cilium/cilium/pkg/hubble/metrics/http        3.391s
?       github.com/cilium/cilium/pkg/hubble/metrics/icmp        [no test files]
?       github.com/cilium/cilium/pkg/hubble/metrics/kafka       [no test files]
ok      github.com/cilium/cilium/pkg/hubble/metrics/policy      0.443s
ok      github.com/cilium/cilium/pkg/hubble/metrics/port-distribution   0.710s
ok      github.com/cilium/cilium/pkg/hubble/metrics/tcp 1.989s

@rolinh rolinh added the needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch label Dec 12, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.13.0-rc4 Dec 12, 2022
@rolinh
Copy link
Member

rolinh commented Dec 13, 2022

/test

Job 'Cilium-PR-K8s-1.24-kernel-5.4' failed:

Click to show.

Test Name

K8sDatapathConfig Host firewall With native routing and endpoint routes

Failure Output

FAIL: Found 1 k8s-app=cilium logs matching list of errors that must be investigated:

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.24-kernel-5.4 so I can create one.

@rolinh
Copy link
Member

rolinh commented Dec 14, 2022

Not sure why Travis doesn't get triggered, it's basically the only truly relevant CI test. Let's try closing the PR and re-opening it.

@rolinh rolinh closed this Dec 14, 2022
@rolinh rolinh reopened this Dec 14, 2022
@marqc marqc force-pushed the add_missing_hubble_metric_handler_tests branch from a4cc061 to 1afaa67 Compare December 15, 2022 10:55
@marqc
Copy link
Contributor Author

marqc commented Dec 15, 2022

Looks like the testing environment is not stable. I rebased to master branch HEAD and re-pushed changes to trigger tests again.

@marqc
Copy link
Contributor Author

marqc commented Dec 15, 2022

Looks like the testing environment is not stable. I rebased to master branch HEAD and re-pushed changes to trigger tests again.

Unfortunately still failing on some k8s setup checks https://github.com/cilium/cilium/actions/runs/3703427000/jobs/6274901939
The reported problem is a missing coredns pod which seems unrelated to cilium code.

Test failed on this check https://github.com/cilium/cilium/blob/master/.github/workflows/conformance-gateway-api.yaml#L124 with info that coredns pod does not exist. Later in post test cluster dump there is another coredns pod visible healthy and ready. I tried performing these test steps manually on my local machine and I haven't seen any problems. Coredns does not transition into ready state until cilium gets installed, but right after that it becomes healthy.

/test

@marqc marqc force-pushed the add_missing_hubble_metric_handler_tests branch from 1afaa67 to e89c688 Compare December 15, 2022 11:56
@rolinh
Copy link
Member

rolinh commented Dec 15, 2022

@marqc Some tests are flaky indeed, let's ignore the test failures as long as the Travis test is ✔️

Signed-off-by: Marek Chodor <mchodor@google.com>
@marqc marqc force-pushed the add_missing_hubble_metric_handler_tests branch from e89c688 to c465899 Compare December 16, 2022 09:13
@rolinh
Copy link
Member

rolinh commented Dec 16, 2022

I'm not sure what's wrong with Travis and why it doesn't trigger. This PR only adds unit tests to Hubble metrics. I have run them 1000 times without any failures, thus marking this PR as ready to merge.

@rolinh rolinh added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 16, 2022
@joamaki joamaki merged commit 00eb46e into cilium:master Dec 20, 2022
@joestringer joestringer added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Dec 21, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport done to v1.10 in 1.13.0-rc4 Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/metrics Impacts statistics / metrics gathering, eg via Prometheus. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. 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/misc This PR makes changes that have no direct user impact. sig/hubble Impacts hubble server or relay
Projects
No open projects
1.13.0-rc4
Backport done to v1.13
Development

Successfully merging this pull request may close these issues.

None yet

7 participants