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

Bug fix for updater parsing #1581

Merged
merged 1 commit into from
Jan 17, 2025
Merged

Bug fix for updater parsing #1581

merged 1 commit into from
Jan 17, 2025

Conversation

nick-enoent
Copy link
Collaborator

Addresses bug when "updaters" is omitted from "peers" dictionary in top level "aggregators" dictionary.
Fixes bug that occurs when "producers" regular expression is not defined in the "updaters" dictionary.
Updaters now correctly only add the parent dictionary's producers when "produers" is not defined in the updaters dictionary.
Updated producer balancing when multiple aggregators share producers in the YAML configuration.
Updated ldmsd_yaml_parser man page

@emdonat
Copy link
Collaborator

emdonat commented Jan 16, 2025

Is the intention for an updaters stanza of:

aggregators:
- daemons: "admin[1-1]-agg"
  peers:
  - daemons: "compute[1-30]"
    endpoints: "compute[1-30]-ib0"
    reconnect: 20s
    type: active
    updaters:
      - mode: pull
        interval: "1.0s"
        offset: "10ms"
        sets:
          - regex: ".*"
            field: inst

to result in configuration lines like:

updtr_prdcr_add name=updtr_0 regex=compute1-ib0
updtr_prdcr_add name=updtr_0 regex=compute2-ib0
updtr_prdcr_add name=updtr_0 regex=compute3-ib0
...
updtr_prdcr_add name=updtr_0 regex=compute29-ib0
updtr_prdcr_add name=updtr_0 regex=compute30-ib0   

Just want to confirm that this is expected. I am adding no producers: entry under aggregators: peers: updaters:.

@nick-enoent
Copy link
Collaborator Author

Is the intention for an updaters stanza of:

aggregators:
- daemons: "admin[1-1]-agg"
  peers:
  - daemons: "compute[1-30]"
    endpoints: "compute[1-30]-ib0"
    reconnect: 20s
    type: active
    updaters:
      - mode: pull
        interval: "1.0s"
        offset: "10ms"
        sets:
          - regex: ".*"
            field: inst

to result in configuration lines like:

updtr_prdcr_add name=updtr_0 regex=compute1-ib0
updtr_prdcr_add name=updtr_0 regex=compute2-ib0
updtr_prdcr_add name=updtr_0 regex=compute3-ib0
...
updtr_prdcr_add name=updtr_0 regex=compute29-ib0
updtr_prdcr_add name=updtr_0 regex=compute30-ib0   

Just want to confirm that this is expected. I am adding no producers: entry under aggregators: peers: updaters:.

Hey @emdonat, this is the intended behavior. Otherwise we'd have to reverse engineer a regular expression from the list of producer names.

@emdonat
Copy link
Collaborator

emdonat commented Jan 16, 2025

Is the intention for an updaters stanza of:

aggregators:
- daemons: "admin[1-1]-agg"
  peers:
  - daemons: "compute[1-30]"
    endpoints: "compute[1-30]-ib0"
    reconnect: 20s
    type: active
    updaters:
      - mode: pull
        interval: "1.0s"
        offset: "10ms"
        sets:
          - regex: ".*"
            field: inst

to result in configuration lines like:

updtr_prdcr_add name=updtr_0 regex=compute1-ib0
updtr_prdcr_add name=updtr_0 regex=compute2-ib0
updtr_prdcr_add name=updtr_0 regex=compute3-ib0
...
updtr_prdcr_add name=updtr_0 regex=compute29-ib0
updtr_prdcr_add name=updtr_0 regex=compute30-ib0   

Just want to confirm that this is expected. I am adding no producers: entry under aggregators: peers: updaters:.

Hey @emdonat, this is the intended behavior. Otherwise we'd have to reverse engineer a regular expression from the list of producer names.

Just to confirm - each updtr_prdcr_add on the same name=updtr_0 appends rather than overwrites the prior? I just want to make sure that the end result isn't that the only regex is the final one processed e.g. compute30-ib0.

@tom95858
Copy link
Collaborator

@emdonat, match_add appends to a list of match expressions. They. are all evaluated to determine if there is a match. You can view this list in the ldmsd_controller using the updtr_match_list command.

@nick-enoent
Copy link
Collaborator Author

Just to confirm - each updtr_prdcr_add on the same name=updtr_0 appends rather than overwrites the prior? I just want to make sure that the end result isn't that the only regex is the final one processed e.g. compute30-ib0.

Yes, that's correct. Subsequent updtr_prdcr_add calls do not overwrite previous calls

@emdonat
Copy link
Collaborator

emdonat commented Jan 16, 2025

@emdonat, match_add appends to a list of match expressions. They. are all evaluated to determine if there is a match. You can view this list in the ldmsd_controller using the updtr_match_list command.

This is prdcr_add in this case.

@emdonat
Copy link
Collaborator

emdonat commented Jan 16, 2025

Thanks. I think that this actually helps with some functionality that was hoped for when demoing this yesterday. Forging our own limiting regex was going to be a frustrating ordeal and error prone.

Addresses bug when "updaters" is omitted from "peers" dictionary in top level "aggregators" dictionary.
Fixes bug that occurs when "producers" regular expression is not defined in the "updaters" dictionary.
Updaters now correctly only add the parent dictionary's producers when "produers" is not defined in the updaters dictionary.
Updated producer balancing when multiple aggregators share producers in the YAML configuration.
@nick-enoent
Copy link
Collaborator Author

Added a missing newline when using a string for configuration commands in the top level daemons dictionary

@tom95858 tom95858 merged commit 4f105db into ovis-hpc:b4.4 Jan 17, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants