PCP Update #1956

Maxusmusti · 2020-11-05T06:57:20Z

Finally got everything into a good enough state that it's worth putting up a draft.
NOTE: there is still a lot of work left for me to do here, like...

Cleaner integration with tool metadata
Further abstracting and optimizing solution to work within persistent tool system rather than separately
Fix unit test compatibility

I will continue updating this draft PR regularly

ANOTHER NOTE: The first commit in this draft needs credit to be given to Ashwin @not4win as a lot of the code was from his original work, but I'm not sure how to do that...

lgtm-com · 2020-11-05T07:06:18Z

This pull request introduces 9 alerts when merging 95d627d into a86cbb6 - view on LGTM.com

new alerts:

4 for Except block handles 'BaseException'
3 for Unused import
2 for Unused local variable

portante · 2020-11-06T00:31:05Z

agent/tool-scripts/meta.json

 		"node-exporter": {"collector": "prometheus", "port": "9100"},
-		"dcgm": {"collector": "prometheus", "port": "8000"}
+		"dcgm": {"collector": "prometheus", "port": "8000"},
+		"pcptool": {"collector": "pcp", "port": "44321"}


Let's drop the existing pcp tool entirely, and just replace it with this one.

agent/util-scripts/pbench-tool-meister-start

portante · 2020-11-06T00:36:22Z

lib/pbench/agent/tool_meister.py

+ANSIBLE_CONF = "/etc/ansible/ansible.cfg"
+ANSIBLE_ROLES_PATH = "/etc/ansible/roles"
+PLAYBOOK_PATH = ""
+TOOLS_PMDA_MAPPING = {


Can we consider making this a class variable of PCPTools?

dbutenhof · 2020-11-10T20:36:31Z

agent/util-scripts/pcp-mapping.json

+        "probe": "bpftrace"
+    },
+    "cpuacct": {
+        "ident": "cpu accouting  Information",


Extra space between "accouting" (which is misspelled) and "Information" (which shouldn't be capitalized).

dbutenhof · 2020-11-10T20:41:20Z

agent/util-scripts/pcp-mapping.json

+        "probe": "dmcache.cache.used values"
+    },
+    "docker": {
+        "ident": "Docker Information",


The "voice" of these "ident" strings are all over the place. It'd be nice to get them all consistent. For example, this one is basically a title, with both words capitalized, and no real indication of what it means; whereas dm-cache was a paragraph introducing the metric. Most people will probably know what Docker is and produce some good guesses about what "Docker Information" might imply, but "docker usage statistics" might be more useful. Or even "docker controller daemon statistics" since the metrics below suggest we're really looking at those rather than wider statistics on container usage within the system.

dbutenhof · 2020-11-10T20:45:09Z

agent/util-scripts/pcp-mapping.json

+        "probe": "kernel.percpu.cpu"
+    },
+    "kvmstat": {
+        "ident": "metrics used by the pcp-kvm(1) command        ",


Why is there whitespace at the end of this? Again, these "doc strings" are all over the place, and that's distracting at best.

dbutenhof · 2020-11-10T20:47:56Z

lib/pbench/agent/tool_data_sink.py

+        """
+        input: takes list of hosts and tools
+        what init function does:
+        1. checks whether pmcd is active amongst the hosts mentions in the list.


"hosts mentioned"? Or maybe just "hosts in the list". I'd probably nuke "amongst" as well and just go with "active in one of the listed hosts".

dbutenhof · 2020-11-10T20:48:55Z

lib/pbench/agent/tool_data_sink.py

+        input: takes list of hosts and tools
+        what init function does:
+        1. checks whether pmcd is active amongst the hosts mentions in the list.
+        2. reads the pcp-mappig file.


Maybe be a bit more specific about "pcp-mapping.json" (ignoring the missing "n")

dbutenhof · 2020-11-10T21:22:45Z

lib/pbench/agent/tool_data_sink.py

+                if (
+                    stdout.decode().strip()
+                    != f'Creating config file "{host}.config" using default settings ...\n.'
+                ):
+                    # pmlogger failed to stop. Log it
+                    self.logger.error("not able to build pmlogconf file")


We can't check the exit status? Judging success by matching output strings is always risky.

(Not to mention that this module has now adding template string formatting to %s formatting and .format formatting, making the full sweep of Python string formatting styles and making the whole thing look just a bit more messy. I'm assuming this is an inherited issue, but it'd be nice to clean it up.)

dbutenhof · 2020-11-10T21:23:57Z

lib/pbench/agent/tool_data_sink.py

+                    stdout.decode().strip()
+                    != f'Creating config file "{host}.config" using default settings ...\n.'
+                ):
+                    # pmlogger failed to stop. Log it


"Failed to stop"? Were we trying to stop it???

dbutenhof · 2020-11-10T21:24:58Z

lib/pbench/agent/tool_data_sink.py

+
+                self.pmlog_config_files.append("{}.config".format(host))
+            except Exception as e:
+                # log exception as we are not able to stop logger


Again we failed to stop it when trying to write a config file. I don't really know how pmlogconf works, although the man page I found didn't suggest it does any more than writing a formatted config file.

dbutenhof · 2020-11-10T21:30:41Z

lib/pbench/agent/tool_data_sink.py

+            except Exception as e:
+                # log exception as we are not able to stop logger
+                self.logger.error(
+                    "not able to create a logger conf file\n", e, exc_info=True


exc_info=True causes the logger to add exception information, so I think the e argument is unused since there's no substitution placeholder in the message string.

This pattern is repeated several times below.

dbutenhof · 2020-11-10T21:35:30Z

lib/pbench/agent/tool_data_sink.py

+    def check_connection(self, hosts):
+        """ Check if pmcd is running in given list of hosts"""
+        self.logger.info("checking connection")
+
+        for host in hosts:
+            res = ""
+            try:
+                self.logger.debug("checking connection on host:%s", host)
+                res = pmapi.pmContext(api.PM_CONTEXT_HOST, host + ":44321")
+            except Exception:
+                res = None
+
+            if res is not None:
+                self.logger.info("Port 44321 is open in:{}".format(host))
+                self.hosts.append(
+                    host
+                )  # logging only on those hosts with port 44321 active
+            else:
+                self.logger.error("Port 44321 is not open in:{}".format(host))
+                self.logger.error("Ignoring host:%s", host)


This shouldn't be repeated twice. Why not promote it to BaseCollector (or a new intermediary PCP class) so they can share?

lgtm-com · 2020-11-12T08:46:14Z

This pull request introduces 1 alert when merging 36b79f8 into ad3e4d3 - view on LGTM.com

new alerts:

1 for Syntax error

dbutenhof

Stacking tool meister on top of Ansible just seems... "off". This sort of cross-node orchestration of tool start/stop seems to be exactly what tool meister was designed to do in the first place. Unfortunate if we need to stack like this to manage PCP.

dbutenhof · 2020-11-12T12:44:52Z

lib/pbench/agent/tool_meister.py

+from ansible.parsing.dataloader import DataLoader
+from ansible.playbook.play import Play
+
+# from ansible.plugins.callback import CallbackBase


Deliberately left commented-out, or sad and forgotten? ;-)

dbutenhof · 2020-11-12T13:01:44Z

lib/pbench/agent/tool_meister.py

+            loader=self.loader, inventory=self.inventory
+        )
+
+    def find_roles_path(self):


Ansible doesn't support APIs to manage roles cleanly without assuming and poking into internals? This really doesn't make me feel good.

dbutenhof · 2020-11-12T13:09:29Z

lib/pbench/agent/tool_meister.py

+        command = "ls -lA " + self.roles_path
+        out = subprocess.Popen(
+            [command], stdout=subprocess.PIPE, stderr=subprocess.STDOUT, shell=True
+        )
+        stdout, stderr = out.communicate()
+        # decode the output
+        output = stdout.decode()
+        self.logger.debug(
+            "output from command:%s is:%s\n and stderr:%s",
+            command,
+            stdout.decode(),
+            stderr,
+        )


Seems like a very cumbersome way to just list files in a directory.

dbutenhof · 2020-11-12T13:11:10Z

lib/pbench/agent/tool_meister.py

    pass


+class PCPTools:


It'd be great to pull this class out of "tool meister" along with the mapping table, and just import it here.

dbutenhof · 2020-11-12T16:03:49Z

lib/pbench/agent/tool_meister.py

+        if output_err is not None:
+            self.logger.error("not able to download role. error:%s", output_err)
+
+    def start(self):


There seems to be a lot of duplication between start and stop that could probably be factored out into a common method to simplify maintenance.

portante · 2020-11-13T03:18:42Z

lib/pbench/agent/tool_data_sink.py

+        # code to start logging
+        self.logger.info("starting logger")
+
+        command = PCP.pmGetConfig("PCP_BINADM_DIR") + "/pmlogger_check -c {}".format(


As discussed verbally today, let's work on containerizing the pmlogger much like the way we handle Prometheus.

portante · 2020-11-13T03:21:42Z

lib/pbench/agent/tool_meister.py

+                return
+
+        # given role does not exist. We have to download it from ansible galaxy
+        command = "ansible-galaxy collection install performancecopilot.metrics"


As we talked about today verbally, let's drop the use of Ansible to install and start PCP on local/remote systems. Philosophically, the pbench-agent should not be in the business of install software on hosts, or changing the host configuration to suite its needs. Instead let's build a container for pmcd which encapsulates what we need, and do the same for node-exporter as well.

portante · 2020-11-18T04:00:55Z

Replaced by #1986.

portante assigned Maxusmusti Nov 6, 2020

portante added Agent enhancement tools Of and related to the operation and behavior of various tools (iostat, sar, etc.) labels Nov 6, 2020

portante added this to the v0.71 milestone Nov 6, 2020

portante reviewed Nov 6, 2020

View reviewed changes

Maxusmusti force-pushed the localrpmtest branch from 95d627d to f046eca Compare November 6, 2020 21:50

Maxusmusti added 17 commits November 10, 2020 11:07

PCP update 1

23b7691

Completed original (will remove htd)

c59fd33

Bug fixes round 1

53c9084

Bugfixes part 1.5

75b305b

Bugfixes 2.0

11edd53

Bugfixes 2.5

c8c623c

Debugging part 3-ish

d146a73

Should now work when not registered

87c4bcb

typo fix

5a39a06

WAIT it's actually kinda working

56a6120

Cleanup misleading error logs and some unnecessary vars

633559b

Flake8 and Black changes

63c5b0e

TM error removal

2082362

TDS remove errors

a580a03

Remove extra redis upload part 1

93c62ba

typo fixes

174f692

Remove redis upload pt2

1a727aa

dbutenhof reviewed Nov 10, 2020

View reviewed changes

Using new performancecopilot.metrics collection on galaxy

a2a838d

Maxusmusti force-pushed the localrpmtest branch from 06f3e84 to a2a838d Compare November 11, 2020 01:46

Fixed broken ansible run

36b79f8

dbutenhof reviewed Nov 12, 2020

View reviewed changes

portante requested changes Nov 13, 2020

View reviewed changes

portante mentioned this pull request Nov 13, 2020

pcp-pbench: [WIP] added PCPTools(tm), PCPPmlogger(tds), PCPPmie(tds) classes and pcp-mapping.json files #1759

Closed

portante changed the base branch from master to pcp-tool-meister November 13, 2020 21:46

portante closed this Nov 18, 2020

PCP Update #1956

PCP Update #1956

Uh oh!

Conversation

Maxusmusti commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgtm-com bot commented Nov 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Nov 12, 2020

Uh oh!

dbutenhof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

portante Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

portante commented Nov 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Maxusmusti commented Nov 5, 2020 •

edited

Loading

portante Nov 13, 2020 •

edited

Loading