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

[improve][broker] Support cgroup v2 by using jdk.internal.platform.Metrics in Pulsar Loadbalancer #16832

Merged
merged 9 commits into from Apr 28, 2023

Conversation

coderzc
Copy link
Member

@coderzc coderzc commented Jul 28, 2022

Master Issue: #16601

Motivation

The Pulsar load balancer detects CPU limits using cgroup v1 API, and the jdk.internal.platform.Metrics already support cgroup (V1, v2) so we should use jdk.internal.platform.Metrics to get the cgroup metrics.

Reference: https://code.yawk.at/java/17/java.base/jdk/internal/platform/

Modifications

Use jdk.internal.platform.Metrics to get the cgroup metrics in the LinuxInfoUtils.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as testCGroupMetrics.

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes / no)
  • Anything that affects deployment: (yes / no / don't know)

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@coderzc coderzc changed the title [feature][broker] Support cgroup v2 for by using jdk.internal.platform.Metrics in Pulsar Loadbalancer [feature][broker] Support cgroup v2 by using jdk.internal.platform.Metrics in Pulsar Loadbalancer Jul 28, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jul 28, 2022
@coderzc coderzc marked this pull request as draft July 28, 2022 05:01
@coderzc coderzc force-pushed the cgroup_metrics branch 2 times, most recently from 8fcc1c6 to 92b41b6 Compare July 28, 2022 08:13
@coderzc coderzc marked this pull request as ready for review July 28, 2022 10:29
@coderzc coderzc force-pushed the cgroup_metrics branch 3 times, most recently from 9bc04e1 to 3e6d1b7 Compare July 28, 2022 12:57
@coderzc
Copy link
Member Author

coderzc commented Jul 29, 2022

/pulsarbot run-failure-checks

@coderzc coderzc requested a review from nodece August 2, 2022 16:07
Copy link
Member

@nodece nodece left a comment

Choose a reason for hiding this comment

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

LGTM

@coderzc
Copy link
Member Author

coderzc commented Aug 4, 2022

/pulsarbot run-failure-checks

@@ -97,4 +101,25 @@ public void testNoNICSpeed() throws Exception {
}


@Test
public void testCGroupMetrics() throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should check if the test really uses the jdk.internal.platform.Metrics not the old way?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, we can assert metrics != null.

@codelipenghui codelipenghui added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/broker labels Aug 8, 2022
@coderzc
Copy link
Member Author

coderzc commented Aug 11, 2022

/pulsarbot run-failure-checks

@asafm
Copy link
Contributor

asafm commented Aug 17, 2022

However, LB also requires other signals such as dict_memory_usage_in_percent_cgroup, network_usage_in_percent_cgroup, which require separate limits for their percentage computation.

Can you please expand on that? I don't any method providing usage and limit of direct memory in Pulsar. I do see usage and limit for physical memory (in the container) which as we said we can take from the mxBean which provides total and usage of memory.

Regarding network usage - yes, that I was surprised to see that the operating system didn't include, and we resort to reading operating system specific files to obtain that. I posted a question in the OpenJDK general discussion mailing to get the motivation for that ( I did search their JIRA issue database and also googled the mailing list site but found nothing).

Maybe we can tweak the code to ignore the limit and use the signal as-is if they are already in the *_usage_in_percent_cgroup form.

I guess that requires a separate PR. Maybe we can push that PR in but add an issue to refactor (delete it out) the dependency on limit and only use percentage (which should be enough) or refactor to avoid using limit and be more specific like ResourceUsage.CpuUsage, MemoryUsage, NetworkUsage, each providing it's own method, where the CPU would only provide percentage.

@heesung-sn
Copy link
Contributor

Can you please expand on that? I don't any method providing usage and limit of direct memory in Pulsar. I do see usage and limit for physical memory (in the container) which as we said we can take from the mxBean which provides total and usage of memory.

I see this method is used to collect direct memory usage.

systemResourceUsage.setDirectMemory(new ResourceUsage((double) (getJvmDirectMemoryUsed() / MIBI),
(double) (DirectMemoryUtils.jvmMaxDirectMemory() / MIBI)));

I guess that requires a separate PR. Maybe we can push that PR in but add an issue to refactor (delete it out) the dependency on limit and only use percentage (which should be enough) or refactor to avoid using limit and be more specific like ResourceUsage.CpuUsage, MemoryUsage, NetworkUsage, each providing it's own method, where the CPU would only provide percentage.

Yes, a separate PR makes sense to me.

@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Sep 17, 2022
@poorbarcode poorbarcode modified the milestones: 3.0.0, 3.1.0 Apr 10, 2023
@nodece
Copy link
Member

nodece commented Apr 26, 2023

Any updates? I think this PR makes sense to use the cgroup v2.

try {
if (metrics != null && getCpuUsageMethod != null) {
return (long) getCpuUsageMethod.invoke(metrics);
Copy link
Contributor

Choose a reason for hiding this comment

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

For backward compatibility, don't we need to multiply the limit in calculateBrokerHostUsage?

public void calculateBrokerHostUsage() {
... 
       double totalCpuLimit = getTotalCpuLimit(isCGroupsEnabled);
        if (isCGroupsEnabled && metrics != null && getCpuUsageMethod != null) {
            // cgroup cpuUsage is already scaled, [0.0, 1.0]
            usage.setCpu(new ResourceUsage(cpuUsage * totalCpuLimit, totalCpuLimit));
        } else {
            usage.setCpu(new ResourceUsage(cpuUsage, totalCpuLimit));
        }
        

Copy link
Member Author

@coderzc coderzc Apr 28, 2023

Choose a reason for hiding this comment

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

The Metrics.getCpuUsage will return the aggregate time, so I think we don't need to multiply the limit.

    /**
     * Returns the aggregate time, in nanoseconds, consumed by all
     * tasks in the Isolation Group.
     *
     * @return Time in nanoseconds, -1 if unknown or
     *         -2 if the metric is not supported.
     *
     */
    public long getCpuUsage();

Copy link
Contributor

Choose a reason for hiding this comment

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

@coderzc
Copy link
Member Author

coderzc commented Apr 27, 2023

I will update this PR as soon.

# Conflicts:
#	bin/pulsar
#	pom.xml
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/LinuxInfoUtils.java
add test
@coderzc
Copy link
Member Author

coderzc commented Apr 28, 2023

I guess that requires a separate PR. Maybe we can push that PR in but add an issue to refactor (delete it out) the dependency on limit and only use percentage (which should be enough) or refactor to avoid using limit and be more specific like ResourceUsage.CpuUsage, MemoryUsage, NetworkUsage, each providing it's own method, where the CPU would only provide percentage.

Yes, we need a separate PR to refactor it.

@coderzc coderzc requested a review from heesung-sn April 28, 2023 13:42
@coderzc coderzc changed the title [feature][broker] Support cgroup v2 by using jdk.internal.platform.Metrics in Pulsar Loadbalancer [improve][broker] Support cgroup v2 by using jdk.internal.platform.Metrics in Pulsar Loadbalancer Apr 28, 2023
@nodece nodece merged commit b8543ad into apache:master Apr 28, 2023
41 of 43 checks passed
@lhotari lhotari added release/blocker Indicate the PR or issue that should block the release until it gets resolved release/2.11.2 release/3.0.2 labels Jun 27, 2023
@lhotari
Copy link
Member

lhotari commented Jun 27, 2023

We need this to be included in branch-3.0 and branch-2.11 asap. I backported the changes to branch-2.10 with all dependencies in #20659.

This is becoming urgent.

AKS Kubernetes 1.25+ switches to use cgroup v2:
https://github.com/Azure/AKS/releases/tag/2023-03-05

AKS Kubernetes 1.24 goes End-of-life on July 30, 2023.

GKE contains to have a way to select between cgroup v1 & cgroup v2:
https://cloud.google.com/kubernetes-engine/docs/how-to/node-system-config#cgroup-mode-options
GKE will default to cgroup v2 in new Kubernetes 1.26 clusters or node pools.
AWS EKS v1.26 nodes will default to cgroup v2.

lhotari pushed a commit that referenced this pull request Jun 27, 2023
…etrics` in Pulsar Loadbalancer (#16832)

(cherry picked from commit b8543ad)
lhotari added a commit that referenced this pull request Jun 27, 2023
…etrics` in Pulsar Loadbalancer (#16832)

(cherry picked from commit b8543ad)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker cherry-picked/branch-2.11 cherry-picked/branch-3.0 doc-not-needed Your PR changes do not impact docs release/blocker Indicate the PR or issue that should block the release until it gets resolved release/2.11.2 release/3.0.1 Stale type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants