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

DPL: disable cpu_usage_fraction for proxies #12976

Merged
merged 1 commit into from
Apr 24, 2024
Merged

Conversation

ktf
Copy link
Member

@ktf ktf commented Apr 3, 2024

DPL: disable cpu_usage_fraction for proxies

@ktf ktf requested a review from a team as a code owner April 3, 2024 00:42
Copy link
Contributor

github-actions bot commented Apr 3, 2024

REQUEST FOR PRODUCTION RELEASES:
To request your PR to be included in production software, please add the corresponding labels called "async-" to your PR. Add the labels directly (if you have the permissions) or add a comment of the form (note that labels are separated by a ",")

+async-label <label1>, <label2>, !<label3> ...

This will add <label1> and <label2> and removes <label3>.

The following labels are available
async-2023-pbpb-apass
async-2023-pp-apass1
async-2022-pp-apass6
async-2022-pp-apass4
async-mc
async-data

@alibuild
Copy link
Collaborator

alibuild commented Apr 3, 2024

Error while checking build/O2/fullCI for c1c8ce9 at 2024-04-03 06:45:

## sw/BUILD/O2-latest/log
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
c++: error: unrecognized command-line option '--rtlib=compiler-rt'

Full log here.

// because of the rate limiting which biases the measurement.
auto& spec = services.get<DeviceSpec const>();
bool enableCPUUsageFraction = true;
if (spec.name.find("proxy") == std::string::npos) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this will find all process with proxy in the name, I'd like to change it to affect only input proxies for now.

@davidrohr
Copy link
Collaborator

@ktf : I had a closer look, and I really don't like using the name for such things. Particularly, since the name is user-defined for the proxies. This will:

  • not work if the user uses capital letter in the "Proxy" name.
  • not work if the user creates a proxy that is not called proxy.
  • accidentally affect devices that have "proxy" in their name.

I think we should use some kind of flag / label mechanism for such purposes. I know that DPL already supports labels somehow. Could we make it such, that the ExternalFairMQDevice proxy sets a label "input-proxy", and we check for that label?

Alternatively, as a simple solution we could add a bitmask to DeviceSpec, ExternalFairMQDeviceProxy could set a bit when the spec is created, and then this can be checked instead of DeviceSpec.name.

Whatever you prefer, but I'd prefer to go directly with a proper solution, not with such a hack.

That said, in DPL there are many places where we do string comparison with the devicespec name, which I don't like. Perhaps we should generally transition that to a flag/label mechanism. I'll open a JIRA for this.

@ktf
Copy link
Member Author

ktf commented Apr 18, 2024

We actually have labels which we could use, which we can require to fully match. I am happy to go that way, instead, but we need to agree on what the label is ("input-proxy"?).

@davidrohr
Copy link
Collaborator

For me it is fine to use "input-proxy". Although, I have to say, I would actually prefer to have a bitmask for dpl-internal properties / labels, like "dpl-internal", "input-proxy", "output-proxy", "dummy-sink", "ccdb-backend", to avoid the string compares needed for matching the labels. Then we can switch also the existing string compares we have for these cases to just checking the bitmask.
I would use labels for user-defined stuff. But this is just my opinion...

@ktf
Copy link
Member Author

ktf commented Apr 19, 2024

yes, we can use the bitmask/ enum at the DeviceSpec level, however at the dataprocessorspec level i would prefer to use the labels in order to avoid some extra json handling for the enums (dataprocessorspecs need to be json serialised).

@ktf
Copy link
Member Author

ktf commented Apr 19, 2024

On a second look, on this specific case, we do not need the enum. The check is done on init and what it does is to enable / disable the metric, so there is no string comparison while in Running. I will expose a bitmask with the proxy / whatever information in the DataProcessingContext, but it does not need to be done in this particular PR.

@alibuild
Copy link
Collaborator

Error while checking build/O2/fullCI for 56aa6f5 at 2024-04-24 13:59:

## sw/BUILD/O2-latest/log
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
c++: error: unrecognized command-line option '--rtlib=compiler-rt'

Full log here.

@ktf ktf merged commit e268535 into AliceO2Group:dev Apr 24, 2024
15 of 19 checks passed
@ktf ktf deleted the pr12976 branch April 24, 2024 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants