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

Use len(Allocator.poolIPsInUse) for addresses_in_use metric #627

Closed
wants to merge 4 commits into from

Conversation

uablrek
Copy link
Contributor

@uablrek uablrek commented Jun 11, 2020

The Allocator.poolServices counter was removed and instead len(Allocator.poolIPsInUse) is used for the "addresses_in_use" metric. It handles shared IP's and seem to be intended for this purpose from start, see comment;

if a.poolIPsInUse[al.pool][al.ip.String()] == 0 {
// Explicitly delete unused IPs from the pool, so that len()
// is an accurate count of IPs in use.
delete(a.poolIPsInUse[al.pool], al.ip.String())
}

Since this would conflict with PR #624 that fix is included in this PR and #624 will be closed.

Fixes #625
Fixed #622
Fixes #626

Signed-off-by: Lars Ekman lars.g.ekman@est.tech

@russellb
Copy link
Collaborator

russellb commented Jun 11, 2020

Thanks! This change looks good to me, though I haven't tested it myself.

Bonus points if you add test coverage, though :)

(I'm not a maintainer, so I can't merge this myself)

@andersmarkendahl
Copy link

andersmarkendahl commented Jun 15, 2020

Hi!

I can confirm that #625 and #622 are both solved by this PR (as expected since now covered by unittest as well).

I built commit 722078a and tested the following:

kubectl apply -f service1.yaml
kubectl apply -f service2.yaml
test-1       LoadBalancer   12.0.245.5   10.0.0.0      5001:32235/TCP   9s
test-2       LoadBalancer   12.0.1.31    10.0.0.1      5001:30399/TCP   4s
wget -q -O- http://$controllerpod:7472/metrics | grep metallb_allocator_addresses_in
_use_total | grep -v 'HELP\|TYPE'
metallb_allocator_addresses_in_use_total{pool="default"} 2

kubectl delete -f service1.yaml
kubectl delete -f service2.yaml
wget -q -O- http://$controllerpod:7472/metrics | grep metallb_allocator_addresses_in
_use_total | grep -v 'HELP\|TYPE'
metallb_allocator_addresses_in_use_total{pool="default"} 0

kubectl apply -f services-shared.yaml
test-1       LoadBalancer   12.0.62.191   10.0.0.0      5001:32469/TCP   4s
test-2       LoadBalancer   12.0.44.248   10.0.0.0      5002:30582/TCP   4s
wget -q -O- http://$controllerpod:7472/metrics | grep metallb_allocator_addresses_in
_use_total | grep -v 'HELP\|TYPE'
metallb_allocator_addresses_in_use_total{pool="default"} 1

Both the empty system and the sharedvip system now shows correct metrics.

@rata Is it possible to get a review from a maintainer on this PR?

Thanks in advance
Anders

@russellb
Copy link
Collaborator

russellb commented Jun 15, 2020

Thanks for adding a test!

@rata
Copy link
Member

rata commented Jun 15, 2020

Thanks! Had a quick look and looks good. Will review later this week in case I'm missing anything in the rush :)

@rata rata self-requested a review Jun 15, 2020
@russellb
Copy link
Collaborator

russellb commented Jun 16, 2020

We just enabled CircleCI to run tests on open PRs, but that didn't make it run against PRs that were already open. I'm leaving this comment to see if a new comment triggers it to run.

Copy link
Member

@rata rata left a comment

@uablrek Hi! Thanks a lot for the PR. The actual fix looks good to me :)

I had a look at the test cases, and it seems prometheus has a testutil package to use for tests. Using that, we can actually test that is what we want and not use the protocol transform + strings.Contains(). I tested it locally and works fine.

Added some suggestions to give you a clear idea of what I just tested, but feel free to modify it if you think it's better with some other changes.

Can you please update it to use the prometheus testutil package for testing?

internal/allocator/allocator_test.go Outdated Show resolved Hide resolved
internal/allocator/allocator_test.go Show resolved Hide resolved
internal/allocator/allocator_test.go Outdated Show resolved Hide resolved
internal/allocator/allocator_test.go Outdated Show resolved Hide resolved
@rata
Copy link
Member

rata commented Jun 20, 2020

Hi!

I can confirm that #625 and #622 are both solved by this PR (as expected since now covered by unittest as well).

I built commit 722078a and tested the following:

Thanks a lot for testing, is really useful! :-)

@rata Is it possible to get a review from a maintainer on this PR?

You got it! :)

@uablrek
Copy link
Contributor Author

uablrek commented Jun 22, 2020

I have added a commit that fixes #626 also and moved the poolCapacity check to before the test-loop in the stats test.

@uablrek uablrek requested a review from rata Jun 22, 2020
@andersmarkendahl
Copy link

andersmarkendahl commented Jun 22, 2020

Hi!

I tested again, building the newest commit 9111d10 and I can confirm that #626 is as well solved by this PR.

With a configured, but empty (no LB services yet), system the metrics are working as I would expect.
Allocator metrics shows from start with proper values.

## Address pool with ipv4 /28 subnet only
kubectl apply -f metallb.yaml 

wget -q -O- http://$controllerpod:7472/metrics | grep metallb_allocator
# HELP metallb_allocator_addresses_in_use_total Number of IP addresses in use, per pool
# TYPE metallb_allocator_addresses_in_use_total gauge
metallb_allocator_addresses_in_use_total{pool="default"} 0
# HELP metallb_allocator_addresses_total Number of usable IP addresses, per pool
# TYPE metallb_allocator_addresses_total gauge
metallb_allocator_addresses_total{pool="default"} 16

I also retested #625 and #622 to make sure they still work as expected.

Thank you very much!
Anders

Copy link
Member

@rata rata left a comment

@uablrek thanks, LGTM! Added one simple comment to avoid type conversions and, while you are there, can you please squash Updated tests after review commit with Added unit-test for Pool metrics?

If not, no problem, I can do it locally and then push :)

@Aoana thanks a lot for the tests, it really helps! :)

ports []Port
sharingKey string
backendKey string
ipsInUse int
Copy link
Member

@rata rata Jun 27, 2020

Choose a reason for hiding this comment

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

Can you use float64 here, so no type conversion is needed later?

@rata rata self-requested a review Jun 27, 2020
uablrek added 3 commits Jun 29, 2020
Fixes metallb#625
Fixed metallb#622

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>
Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>
Fixes metallb#626

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>
@uablrek
Copy link
Contributor Author

uablrek commented Jun 29, 2020

@rata Updated, please re-review

rata
rata approved these changes Jun 29, 2020
Copy link
Member

@rata rata left a comment

Thanks again, LGTM! :)

If you don't mind, I might fixup two commits locally before pushing (the "corrected type" and the comit that added the "incorrect type") and maybe switch the log test prints statements to use %v if you don't mind (seems simpler, unless you used %f for some reason :))

@uablrek
Copy link
Contributor Author

uablrek commented Jun 30, 2020

@rata Please adjust the commits as you wich. And no, I just used %f because of my C-heritage 😃

@rata
Copy link
Member

rata commented Jun 30, 2020

Please adjust the commits as you wich

Cool, will do in a sec! :)

And no, I just used %f because of my C-heritage

haha, it happens to me too 😂 . I think I miss programming in C from time to time, until I actually program in C :-P

@rata
Copy link
Member

rata commented Jun 30, 2020

Thanks a lot for the PR @uablrek and thanks a lot for the tests @Aoana ! I just pushed these, with a simple ammend on this commit.

Closing as it is already merged :)

@rata rata closed this Jun 30, 2020
@andersmarkendahl
Copy link

andersmarkendahl commented Jun 30, 2020

This is really great progress!
Big thanks to @uablrek and (@rata for reviewing). :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment