Skip to content

Pcp transient main #2196

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 12, 2021
Merged

Pcp transient main #2196

merged 1 commit into from
Apr 12, 2021

Conversation

@Maxusmusti
Copy link
Member

@Maxusmusti Maxusmusti commented Apr 1, 2021

Original: portante#14

  • Moved to be on top of main branch now that PR Containerization of TM and TDS #2076 has been merged
  • Adds a transient pcp-tool that creates local pmloggers and pmcds on remote nodes, and starts/stops/sends alongside transient tools, rather than permanently running in background.
  • Tool is currently fully functional according to both traditional and containerized tests.

@Maxusmusti Maxusmusti requested a review from portante April 1, 2021 10:19
@Maxusmusti Maxusmusti mentioned this pull request Apr 1, 2021
Closed
@portante
Copy link
Member

portante commented Apr 1, 2021

@Maxusmusti, let's squash these commits down to one now, and perhaps drop the repeated comment from PR #2076 in favor of just one commit message describing this work.

@portante portante added this to the v0.71 milestone Apr 1, 2021
@portante portante added enhancement tools Of and related to the operation and behavior of various tools (iostat, sar, etc.) labels Apr 1, 2021
@Maxusmusti Maxusmusti force-pushed the pcp-transient-main branch from ba21701 to 55f948e Compare April 1, 2021 10:45
portante
portante requested changes Apr 1, 2021
View reviewed changes
Copy link
Member

@portante portante left a comment

Choose a reason for hiding this comment

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

Good change, not sure about how to solve the duplication of PCP commands from the persistent tools, we may to use a "mix-in" class (see https://www.residentmar.io/2019/07/07/python-mixins.html) to address that, and I am also not sure about the -transient suffix everywhere, seems like that could be in the tool metadata somehow, but would have to think about it. Just one change to consider inline ...

@Maxusmusti Maxusmusti force-pushed the pcp-transient-main branch from 55f948e to 60d9966 Compare April 1, 2021 20:54
@Maxusmusti Maxusmusti requested a review from portante April 1, 2021 21:03
dbutenhof
dbutenhof reviewed Apr 1, 2021
View reviewed changes
@Maxusmusti Maxusmusti requested a review from dbutenhof April 5, 2021 06:34
@Maxusmusti Maxusmusti force-pushed the pcp-transient-main branch 2 times, most recently from 63c82f2 to 3d52e17 Compare April 5, 2021 06:55
dbutenhof
dbutenhof reviewed Apr 5, 2021
View reviewed changes
@Maxusmusti Maxusmusti force-pushed the pcp-transient-main branch from 3d52e17 to 1777418 Compare April 6, 2021 21:11
@Maxusmusti Maxusmusti requested a review from dbutenhof April 6, 2021 21:11
webbnh
webbnh requested changes Apr 7, 2021
View reviewed changes
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

Looks generally good. I found a few things for you to consider addressing.

The biggest one is, what is the deal with agent/tool-scripts/pcp-transient? It claims to be deprecated but looks like new code? Should it be omitted from this PR, or is it supposed to be a placeholder for a future tool (in which case, "deprecated" is the wrong word!)?

@Maxusmusti Maxusmusti mentioned this pull request Apr 8, 2021
Open
@Maxusmusti Maxusmusti requested a review from webbnh April 8, 2021 06:10
@Maxusmusti Maxusmusti force-pushed the pcp-transient-main branch from c4f349c to a304b9e Compare April 8, 2021 06:15
webbnh
webbnh previously approved these changes Apr 8, 2021
View reviewed changes
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

Looks good. I say "Ship it!".

I do have one question, but don't feel like you have to gate the merge on fixing it.

@webbnh
Copy link
Member

webbnh commented Apr 8, 2021

...but, you need a rebase....

@Maxusmusti Maxusmusti force-pushed the pcp-transient-main branch from a304b9e to 30eb5d7 Compare April 8, 2021 18:43
@Maxusmusti Maxusmusti requested a review from webbnh April 8, 2021 18:52
webbnh
webbnh previously approved these changes Apr 8, 2021
View reviewed changes
dbutenhof
dbutenhof previously approved these changes Apr 8, 2021
View reviewed changes
dbutenhof
dbutenhof previously approved these changes Apr 9, 2021
View reviewed changes
@dbutenhof
Copy link
Member

dbutenhof commented Apr 9, 2021

Of course you actually need to rebase since I just merged #2206 (sorry ;-) )

portante
portante previously approved these changes Apr 9, 2021
View reviewed changes
Copy link
Member

@portante portante left a comment

Choose a reason for hiding this comment

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

This code is pushing us to the edge of the usefulness of pbench-register-tool in bash.

@Maxusmusti Maxusmusti dismissed stale reviews from portante and dbutenhof via c545a1b April 9, 2021 13:32
@Maxusmusti Maxusmusti force-pushed the pcp-transient-main branch from 6dfb201 to c545a1b Compare April 9, 2021 13:32
@Maxusmusti Maxusmusti requested review from dbutenhof and portante April 9, 2021 13:35
dbutenhof
dbutenhof previously approved these changes Apr 9, 2021
View reviewed changes
webbnh
webbnh previously approved these changes Apr 9, 2021
View reviewed changes
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

Looks good -- I like the headers.

One comment and suggestion, but it doesn't need to gate the merge.

portante
portante requested changes Apr 10, 2021
View reviewed changes
Copy link
Member

@portante portante left a comment

Choose a reason for hiding this comment

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

Needs a rebase and a squash ... looks good otherwise (though addressing @webbnh's last comment might be worth it.

@Maxusmusti Maxusmusti dismissed stale reviews from webbnh and dbutenhof via e3bddaf April 10, 2021 21:45
@Maxusmusti
Copy link
Member Author

Maxusmusti commented Apr 10, 2021

Rebased, squashed, and added suggested change. Also, the makefile needed to have "validate-hostname" and "validate-ipaddress" from the latest commit on main.

dbutenhof
dbutenhof previously approved these changes Apr 12, 2021
View reviewed changes
@portante
Copy link
Member

portante commented Apr 12, 2021

Rebased, squashed, and added suggested change. Also, the makefile needed to have "validate-hostname" and "validate-ipaddress" from the latest commit on main.

Had filed #2212.

@portante @Maxusmusti
PCP Transient Tool Update
86e2434 
 - Introduces a transient option for the pcp tool
 - Also adds new tool register options and naming conventions
    - `--transient` and `--persistent` upon registration
@Maxusmusti
Copy link
Member Author

Maxusmusti commented Apr 12, 2021

Just rebased on the validate-* fix

@Maxusmusti Maxusmusti requested a review from dbutenhof April 12, 2021 12:47
@portante portante merged commit 1ec7308 into distributed-system-analysis:main Apr 12, 2021
@portante portante linked an issue Apr 12, 2021 that may be closed by this pull request
Add ability to run PCP as a transient tool as well as a persistent tool #2122
Closed
@portante portante mentioned this pull request Apr 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@portante portante portante approved these changes

@dbutenhof dbutenhof dbutenhof approved these changes

@webbnh webbnh Awaiting requested review from webbnh

Assignees

@Maxusmusti Maxusmusti

Labels

Agent enhancement tools Of and related to the operation and behavior of various tools (iostat, sar, etc.)

Projects

None yet

Milestone

v0.71

Development

Successfully merging this pull request may close these issues.

Add ability to run PCP as a transient tool as well as a persistent tool

4 participants

@Maxusmusti @portante @webbnh @dbutenhof