Skip to content
This repository has been archived by the owner on Aug 24, 2022. It is now read-only.

PMM-9993 Add support to set web.listen-address #381

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cezmunsta
Copy link

PMM-9993

Build: SUBMODULES-0

  • Added --exporter-listen-address option
  • Added config.ExporterAddress, stored in the config under
    exporter-listen-address
  • Added supervisor.Supervisor.exporterAddress to refer to
    config.ExporterAddress
  • Dynamically rewrite any web.listen-address that do not have a
    specified address (=:)

* Added `--exporter-listen-address` option
* Added `config.ExporterAddress`, stored in the config under
  `exporter-listen-address`
* Added `supervisor.Supervisor.exporterAddress` to refer to
  `config.ExporterAddress`
* Dynamically rewrite any `web.listen-address` that do not have a
  specified address (`=:`)
@cezmunsta cezmunsta added the enhancement New feature or request label May 10, 2022
@cezmunsta cezmunsta self-assigned this May 10, 2022
@cezmunsta cezmunsta requested a review from BupycHuk as a code owner May 10, 2022 17:12
@cezmunsta cezmunsta marked this pull request as draft May 10, 2022 17:12
@it-percona-cla
Copy link

it-percona-cla commented May 10, 2022

CLA assistant check
All committers have signed the CLA.

config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
agents/supervisor/supervisor_test.go Outdated Show resolved Hide resolved
changes chan *agentpb.StateChangedRequest
qanRequests chan *agentpb.QANCollectRequest
l *logrus.Entry
ctx context.Context
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci-lint] reported by reviewdog 🐶
found a struct that contains a context.Context field (containedctx)

Copy link
Author

Choose a reason for hiding this comment

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

Already exists

@@ -143,6 +153,8 @@ type Config struct {
ListenAddress string `yaml:"listen-address"`
ListenPort uint16 `yaml:"listen-port"`

ExporterListenAddress ExporterAddress `yaml:"exporter-listen-address"`
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci-lint] reported by reviewdog 🐶
yaml(camel): got 'exporter-listen-address' want 'exporterListenAddress' (tagliatelle)

Copy link
Author

Choose a reason for hiding this comment

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

Following the pattern of the struct

Copy link
Member

@BupycHuk BupycHuk left a comment

Choose a reason for hiding this comment

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

Hi @cezmunsta, it's better to implement it on pmm-managed side since it controls what flags are passed and knows metrics mode of exporter. we also should take care of users who might want to collect metrics from exporters directly by external prometheus.

@cezmunsta
Copy link
Author

cezmunsta commented May 10, 2022

Hi @cezmunsta, it's better to implement it on pmm-managed side since it controls what flags are passed and knows metrics mode of exporter.

@BupycHuk I think that you will need to clarify those points, as:

  • This is under user control, unless they set the option then behaviour doesn't change
  • I don't see the relevance of the mode. You may still want to set the address when pulling, e.g. multiple interfaces

we also should take care of users who might want to collect metrics from exporters directly by external prometheus.

Well, they will struggle to do that as the port can change across service restarts :)

$ cat ~/.config/pmm/pmm-agent-flags.txt
--metrics-mode=push
--server-address=127.0.0.1:28443
--server-username=admin
--server-password=admin
--server-insecure-tls
--ports-min=41000
--ports-max=41100
10.0.20.1
generic
test

$ pmm-admin inventory list nodes --json | jq '.nodes[] | select(.node_name == "test")'
{
  "node_type": "GENERIC_NODE",
  "node_name": "test",
  "address": "10.0.20.1",
  "node_id": "/node_id/06bf1d1a-5113-46fd-bf82-070cd062fb6a"
}

$ pmm-admin list --json | jq '.agent[] | select(.agent_type == "NODE_EXPORTER")' 
{
  "agent_type": "NODE_EXPORTER",
  "agent_id": "/agent_id/86f7ea27-65d5-4f92-a94e-2e71cbaaccbc",
  "service_id": "",
  "status": "RUNNING",
  "disabled": false,
  "push_metrics_enabled": "push"
}

$ podman exec --user postgres pmm-server psql -d pmm-managed -c "select listen_port from agents where agent_id = '/agent_id/86f7ea27-65d5-4f92-a94e-2e71cbaaccbc'"
 listen_port 
-------------
       41001
(1 row)

$ pkill -TERM -x pmm-agent.prod

$ ( nc -lk 41001 | tee ) &

$ /usr/local/percona/pmm2/bin/pmm-agent.prod run --config-file /usr/local/percona/pmm2/config/pmm-agent.yaml &

$ podman exec --user postgres pmm-server psql -d pmm-managed -c "select listen_port from agents where agent_id = '/agent_id/86f7ea27-65d5-4f92-a94e-2e71cbaaccbc'"
 listen_port 
-------------
       41002
(1 row)

@BupycHuk
Copy link
Member

If we let users set a listen-address on pmm-agent side it won't support both metrics modes at the same time. Because part of them will be pulled by victoria metrics on PMM Server and part of them by vm-agent on the same machine. So we should make it on pmm-managed side. Based on metrics mode we can set node address or localhost as listen-address. To control it we can store new value like restrict-listen-address in DB.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants