You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/12/08 05:49:18 UTC
[GitHub] [pulsar] mattisonchao opened a new pull request, #18812: [fix][broker] Revert #14829 Filter the virtual NIC with relative path
mattisonchao opened a new pull request, #18812:
URL: https://github.com/apache/pulsar/pull/18812
### Motivation
As https://github.com/apache/pulsar/issues/14820#issuecomment-1340677223 mentioned, we need to revert this change to fix regression.
For the original issue, the workaround fix is he can divide the target by two. for the fixing, I would like to start a discussion, and I suggest introducing the new config likes `loadBalancerOverrideBrokerNicSpeedGbps`, but not summarised by NIC numbers.
### Modifications
- Revert 14829
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Documentation
I'm not sure if we need to notice this problem.
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #18812: Revert #14829 Filter the virtual NIC with relative path
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #18812:
URL: https://github.com/apache/pulsar/pull/18812#issuecomment-1341947933
@mattisonchao Please add the following content to your PR description and select a checkbox:
```
- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #18812: [fix][broker] Revert #14829 Filter the virtual NIC with relative path
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #18812:
URL: https://github.com/apache/pulsar/pull/18812#issuecomment-1342141411
@grssam
Oh, sorry, I missed the RX and TX.
> Please let me know if this sounds good?
Yes, I like this approach. and It would be better if we could set different NIC speeds.
By the way, I revert this PR just want to ensure the load balancer can work like before. Because the current behaviour of this component looks like isn't working in k8s env. (I do not very confirm this problem, please correct me if I missing something)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #18812: [fix][broker] Revert #14829 Filter the virtual NIC with relative path
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #18812:
URL: https://github.com/apache/pulsar/pull/18812#issuecomment-1342321595
wait for #18818
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- merged pull request #18812: [fix][broker] Revert #14829 Filter the virtual NIC with relative path
Posted by GitBox <gi...@apache.org>.
Technoboy- merged PR #18812:
URL: https://github.com/apache/pulsar/pull/18812
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao closed pull request #18812: [fix][broker] Revert #14829 Filter the virtual NIC with relative path
Posted by GitBox <gi...@apache.org>.
mattisonchao closed pull request #18812: [fix][broker] Revert #14829 Filter the virtual NIC with relative path
URL: https://github.com/apache/pulsar/pull/18812
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- closed pull request #18812: [fix][broker] Revert #14829 Filter the virtual NIC with relative path
Posted by GitBox <gi...@apache.org>.
Technoboy- closed pull request #18812: [fix][broker] Revert #14829 Filter the virtual NIC with relative path
URL: https://github.com/apache/pulsar/pull/18812
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] grssam commented on pull request #18812: [fix][broker] Revert #14829 Filter the virtual NIC with relative path
Posted by GitBox <gi...@apache.org>.
grssam commented on PR #18812:
URL: https://github.com/apache/pulsar/pull/18812#issuecomment-1342108446
@mattisonchao Hello. As I understand the core issue --
> the property `loadBalancerOverrideBrokerNicSpeedGbps` was introduced because certain VM (and also true for k8s world) the max nic speed reported in the `/sys/class/net/%s/speed` was incorrect, so the property helped define the max speed limit. Then it was later found out that using all of the nics present inside the `/sys/class/net/%s` do not correctly represent the actual rx and tx of the broker and thus the `/virtual/` ones were removed.
These assumptions are actually correct most of the times. For instance, with the change introduced in this PR, now both of the `eth0` and `lo` would be counted towards current rx and tx of the broker host.. which is also incorrect, as the `lo` represents internal local rx/tx between processes and 127.0.0.1 and thus, only `eth0`'s rx and tx should be counted towards network io.
My suggestion to fix this would be following:
* Keep relying on `loadBalancerOverrideBrokerNicSpeedGbps` config for max rx/tx of the broker.
* Keep filtering out `/virtual/` nics.
* Introduce another config `loadBalancerOverrideBrokerWhitelistedNics` as a comma separated list of nics to be considered for calculating actual rx/tx.
that way, in our example :
```
pulsar@pulsar-broker-6:~$ ls -larth /sys/class/net/
total 0
drwxr-xr-x 29 root root 0 Oct 31 18:08 ..
drwxr-xr-x 2 root root 0 Oct 31 18:08 .
lrwxrwxrwx 1 root root 0 Oct 31 18:08 lo -> ../../devices/virtual/net/lo
lrwxrwxrwx 1 root root 0 Oct 31 18:08 eth0 -> ../../devices/virtual/net/eth0
```
we will set the config `loadBalancerOverrideBrokerWhitelistedNics` to `eth0` (even this name is not fixed across debian versions for us) and the config `loadBalancerOverrideBrokerNicSpeedGbps` to the correct software (k8s) imposed network limits.
Please let me know if this sounds good?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] grssam commented on pull request #18812: [fix][broker] Revert #14829 Filter the virtual NIC with relative path
Posted by GitBox <gi...@apache.org>.
grssam commented on PR #18812:
URL: https://github.com/apache/pulsar/pull/18812#issuecomment-1342173546
> Thanks a lot for your professional suggestion. I will point out this approach in ML after this PR gets merged.
@mattisonchao Sorry for the confusion, but what is ML? :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] grssam commented on pull request #18812: [fix][broker] Revert #14829 Filter the virtual NIC with relative path
Posted by GitBox <gi...@apache.org>.
grssam commented on PR #18812:
URL: https://github.com/apache/pulsar/pull/18812#issuecomment-1345918275
@mattisonchao I see, yes that one seem to be 2.11+ only, but this one should be cherry-picked to 2.9 and 2.10
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #18812: [fix][broker] Revert #14829 Filter the virtual NIC with relative path
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #18812:
URL: https://github.com/apache/pulsar/pull/18812#issuecomment-1345772899
@grssam No, #18818 just affect 2.11+
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] grssam commented on pull request #18812: [fix][broker] Revert #14829 Filter the virtual NIC with relative path
Posted by GitBox <gi...@apache.org>.
grssam commented on PR #18812:
URL: https://github.com/apache/pulsar/pull/18812#issuecomment-1345483919
Should this and related change #18818 not go into 2.9 and 2.10 branches as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #18812: Revert #14829 Filter the virtual NIC with relative path
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #18812:
URL: https://github.com/apache/pulsar/pull/18812#issuecomment-1341948897
@codelipenghui @eolivelli @lhotari @Jason918 @Technoboy- @zengguan Please take a look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] grssam commented on pull request #18812: [fix][broker] Revert #14829 Filter the virtual NIC with relative path
Posted by GitBox <gi...@apache.org>.
grssam commented on PR #18812:
URL: https://github.com/apache/pulsar/pull/18812#issuecomment-1342155954
> Yes, I like this approach. and It would be better if we could set different NIC speeds.
Yes, would be good to have different NIC speed override. Since the config `loadBalancerOverrideBrokerNicSpeedGbps` gets multiplied by number of nics, which is also incorrect in k8s world. the `lo` is just a loopback interface as the packets are addressed to localhost and they are generally not limited by any max limit.
> By the way, I revert this PR just want to ensure the load balancer can work like before. Because the current behaviour of this component looks like isn't working in k8s env. (I do not very confirm this problem, please correct me if I missing something)
Sure, please go ahead with the revert, and your understanding is correct that currently, in k8s pods, the network speeds are not monitored.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #18812: [fix][broker] Revert #14829 Filter the virtual NIC with relative path
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #18812:
URL: https://github.com/apache/pulsar/pull/18812#issuecomment-1342163238
@grssam
Thanks a lot for your professional suggestion. I will point out this approach in ML after this PR gets merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #18812: [fix][broker] Revert #14829 Filter the virtual NIC with relative path
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #18812:
URL: https://github.com/apache/pulsar/pull/18812#issuecomment-1342185313
@grssam
> Sorry for the confusion, but what is ML
Apache pulsar mail list, https://lists.apache.org/list.html?dev@pulsar.apache.org
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codecov-commenter commented on pull request #18812: [fix][broker] Revert #14829 Filter the virtual NIC with relative path
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #18812:
URL: https://github.com/apache/pulsar/pull/18812#issuecomment-1342103791
# [Codecov](https://codecov.io/gh/apache/pulsar/pull/18812?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#18812](https://codecov.io/gh/apache/pulsar/pull/18812?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6593fde) into [master](https://codecov.io/gh/apache/pulsar/commit/68ca60cea7665e6a4bd9f07518b038c17893fe02?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (68ca60c) will **decrease** coverage by `12.94%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/pulsar/pull/18812?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18812 +/- ##
=============================================
- Coverage 50.05% 37.10% -12.95%
+ Complexity 11024 1970 -9054
=============================================
Files 703 209 -494
Lines 68814 14425 -54389
Branches 7378 1574 -5804
=============================================
- Hits 34446 5353 -29093
+ Misses 30621 8487 -22134
+ Partials 3747 585 -3162
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `37.10% <ø> (-12.95%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pulsar/pull/18812?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../org/apache/pulsar/client/impl/ConnectionPool.java](https://codecov.io/gh/apache/pulsar/pull/18812/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL0Nvbm5lY3Rpb25Qb29sLmphdmE=) | `37.43% <0.00%> (-1.03%)` | :arrow_down: |
| [...ache/pulsar/broker/loadbalance/LinuxInfoUtils.java](https://codecov.io/gh/apache/pulsar/pull/18812/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9MaW51eEluZm9VdGlscy5qYXZh) | | |
| [...ersistentStickyKeyDispatcherMultipleConsumers.java](https://codecov.io/gh/apache/pulsar/pull/18812/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL3BlcnNpc3RlbnQvUGVyc2lzdGVudFN0aWNreUtleURpc3BhdGNoZXJNdWx0aXBsZUNvbnN1bWVycy5qYXZh) | | |
| [...r/stats/prometheus/PrometheusMetricsGenerator.java](https://codecov.io/gh/apache/pulsar/pull/18812/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zdGF0cy9wcm9tZXRoZXVzL1Byb21ldGhldXNNZXRyaWNzR2VuZXJhdG9yLmphdmE=) | | |
| [...apache/pulsar/broker/admin/impl/FunctionsBase.java](https://codecov.io/gh/apache/pulsar/pull/18812/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi9pbXBsL0Z1bmN0aW9uc0Jhc2UuamF2YQ==) | | |
| [...er/impl/SingleSnapshotAbortedTxnProcessorImpl.java](https://codecov.io/gh/apache/pulsar/pull/18812/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci90cmFuc2FjdGlvbi9idWZmZXIvaW1wbC9TaW5nbGVTbmFwc2hvdEFib3J0ZWRUeG5Qcm9jZXNzb3JJbXBsLmphdmE=) | | |
| [...rg/apache/bookkeeper/mledger/impl/OpReadEntry.java](https://codecov.io/gh/apache/pulsar/pull/18812/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9pbXBsL09wUmVhZEVudHJ5LmphdmE=) | | |
| [...che/pulsar/broker/BookKeeperClientFactoryImpl.java](https://codecov.io/gh/apache/pulsar/pull/18812/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9Cb29rS2VlcGVyQ2xpZW50RmFjdG9yeUltcGwuamF2YQ==) | | |
| [...che/bookkeeper/mledger/offload/OffloaderUtils.java](https://codecov.io/gh/apache/pulsar/pull/18812/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9vZmZsb2FkL09mZmxvYWRlclV0aWxzLmphdmE=) | | |
| [...balance/impl/SimpleResourceAllocationPolicies.java](https://codecov.io/gh/apache/pulsar/pull/18812/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9pbXBsL1NpbXBsZVJlc291cmNlQWxsb2NhdGlvblBvbGljaWVzLmphdmE=) | | |
| ... and [486 more](https://codecov.io/gh/apache/pulsar/pull/18812/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org