Skip to content

Commit

Permalink
metrics: Prefer valkey over redis
Browse files Browse the repository at this point in the history 
This fork is Fedora's prefered redis API now [1], since redis' license
change. Our Fedora bots images have `valkey` since [2].

However, also still test with the original `redis` package as long as
that exists in Fedora, in the new `testPmProxySettingsRedis()`.

Declare the prefered package in a per-OS "config" manifest field, as
detecting it at runtime is too expensive and brittle.

[1] https://fedoraproject.org/wiki/Changes/Replace_Redis_With_Valkey
[2] cockpit-project/bots#6432
  • Loading branch information
@martinpitt
martinpitt committed May 29, 2024
1 parent d5b41af commit fe9a48e
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 16 deletions.
2 changes: 1 addition & 1 deletion doc/guide/feature-pcp.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ $ <command>pmstat</command>

<para>These metrics can also be exposed to other machines on a TCP port with
<ulink url="https://linux.die.net/man/1/pmproxy">pmproxy</ulink>
and <ulink url="https://redis.io/">Redis</ulink>:</para>
and <ulink url="https://redis.io/">Redis</ulink> or <ulink url="https://valkey.io/">Valkey</ulink>:</para>

<programlisting>
systemctl enable --now redis pmproxy
Expand Down
6 changes: 6 additions & 0 deletions pkg/metrics/manifest.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,11 @@
"url": "https://pcp.readthedocs.io/en/latest/"
}
]
},

"config": {
"redis_package": {
"fedora": "valkey"
}
}
}
36 changes: 26 additions & 10 deletions pkg/metrics/metrics.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ import * as service from "service";
import * as timeformat from "timeformat";
import { superuser } from "superuser";
import { journal } from "journal";
import { read_os_release } from "os-release";
import { get_manifest_config_matchlist } from "utils";
import { useObject, useEvent, useInit } from "hooks.js";
import { WithDialogs, useDialogs } from "dialogs.jsx";

Expand Down Expand Up @@ -1363,10 +1365,11 @@ const wait_cond = (cond, objects) => {
const PCPConfigDialog = ({
firewalldRequest,
needsLogout, setNeedsLogout,
s_pmlogger, s_pmproxy, s_redis, s_redis_server
s_pmlogger, s_pmproxy, s_redis, s_redis_server, s_valkey,
}) => {
const Dialogs = useDialogs();
const dialogInitialProxyValue = runningService(s_pmproxy) && (runningService(s_redis) || runningService(s_redis_server));
const dialogInitialProxyValue = runningService(s_pmproxy) && (
runningService(s_redis) || runningService(s_redis_server) || runningService(s_valkey));
const [dialogError, setDialogError] = useState(null);
const [dialogLoggerValue, setDialogLoggerValue] = useState(runningService(s_pmlogger));
const [dialogProxyValue, setDialogProxyValue] = useState(dialogInitialProxyValue);
Expand All @@ -1380,9 +1383,13 @@ const PCPConfigDialog = ({
const missing = [];
if (dialogLoggerValue && !s_pmlogger.exists)
missing.push("cockpit-pcp");
const redisExists = () => s_redis.exists || s_redis_server.exists;
if (dialogProxyValue && !redisExists())
missing.push("redis");
const redisExists = () => s_redis.exists || s_redis_server.exists || s_valkey.exists;
if (dialogProxyValue && !redisExists()) {
const os_release = await read_os_release();
missing.push(get_manifest_config_matchlist("metrics", "redis_package", "redis",
[os_release.PLATFORM_ID, os_release.ID]));
}

if (missing.length > 0) {
debug("PCPConfig: missing packages", JSON.stringify(missing), ", offering install");
Dialogs.close();
Expand All @@ -1392,7 +1399,7 @@ const PCPConfigDialog = ({
setNeedsLogout(true);
await wait_cond(() => (s_pmlogger.exists &&
(!dialogProxyValue || (s_pmproxy.exists && redisExists()))),
[s_pmlogger, s_pmproxy, s_redis, s_redis_server]);
[s_pmlogger, s_pmproxy, s_redis, s_redis_server, s_valkey]);
}
};

Expand All @@ -1405,7 +1412,10 @@ const PCPConfigDialog = ({

let real_redis;
let redis_name;
if (s_redis_server.exists) {
if (s_valkey.exists && s_valkey.unit?.UnitFileState !== 'masked') {
real_redis = s_valkey;
redis_name = "valkey.service";
} else if (s_redis_server.exists && s_redis_server.unit?.UnitFileState !== 'masked') {
real_redis = s_redis_server;
redis_name = "redis-server.service";
} else {
Expand Down Expand Up @@ -1527,15 +1537,20 @@ const PCPConfig = ({ buttonVariant, firewalldRequest, needsLogout, setNeedsLogou
// redis.service on Fedora/RHEL, redis-server.service on Debian/Ubuntu with an Alias=redis
const s_redis = useObject(() => service.proxy("redis.service"), null, []);
const s_redis_server = useObject(() => service.proxy("redis-server.service"), null, []);
const s_valkey = useObject(() => service.proxy("valkey.service"), null, []);

useEvent(superuser, "changed");
useEvent(s_pmlogger, "changed");
useEvent(s_pmproxy, "changed");
useEvent(s_redis, "changed");
useEvent(s_redis_server, "changed");
useEvent(s_valkey, "changed");

debug("PCPConfig s_pmlogger.state", s_pmlogger.state, "needs logout", needsLogout);
debug("PCPConfig s_pmproxy state", s_pmproxy.state, "redis exists", s_redis.exists, "state", s_redis.state, "redis-server exists", s_redis_server.exists, "state", s_redis_server.state);
debug("PCPConfig s_pmproxy state", s_pmproxy.state,
"redis exists", s_redis.exists, "state", s_redis.state,
"redis-server exists", s_redis_server.exists, "state", s_redis_server.state,
"valkey exists", s_valkey.exists, "state", s_valkey.state);

if (!superuser.allowed)
return null;
Expand All @@ -1545,12 +1560,13 @@ const PCPConfig = ({ buttonVariant, firewalldRequest, needsLogout, setNeedsLogou
needsLogout={needsLogout} setNeedsLogout={setNeedsLogout}
s_pmlogger={s_pmlogger}
s_pmproxy={s_pmproxy}
s_redis={s_redis} s_redis_server={s_redis_server} />);
s_redis={s_redis} s_redis_server={s_redis_server} s_valkey={s_valkey} />);
}

return (
<Button variant={buttonVariant} icon={<CogIcon />}
isDisabled={ invalidService(s_pmlogger) || invalidService(s_pmproxy) || invalidService(s_redis) || invalidService(s_redis_server) }
isDisabled={ invalidService(s_pmlogger) || invalidService(s_pmproxy) ||
invalidService(s_redis) || invalidService(s_redis_server) || invalidService(s_valkey) }
onClick={show_dialog}>
{ _("Metrics settings") }
</Button>);
Expand Down
24 changes: 19 additions & 5 deletions test/verify/check-metrics
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ def prepareArchive(machine, name, time, hostname="localhost.localdomain"):
def redisService(image):
if image.startswith(("debian", "ubuntu")):
return "redis-server"
if image.startswith("fedora"):
return "valkey"
return "redis"


Expand Down Expand Up @@ -546,7 +548,7 @@ class TestHistoryMetrics(testlib.MachineCase):

@testlib.nondestructive
@testlib.skipOstree("no PCP support")
def testPmProxySettings(self):
def testPmProxySettings(self, customRedisService=None):
b = self.browser
m = self.machine

Expand All @@ -557,10 +559,10 @@ class TestHistoryMetrics(testlib.MachineCase):
m.execute("firewall-cmd --zone=public --change-interface eth0 --permanent")
m.execute("firewall-cmd --reload")

redis = redisService(m.image)
redis = customRedisService or redisService(m.image)
hostname = m.execute("hostname").strip()

self.addCleanup(m.execute, f"systemctl stop {redis}")
self.addCleanup(m.execute, f"systemctl disable --now {redis}")

def checkEnable(firewalld_alert):
b.click("#metrics-header-section button.pf-m-secondary")
Expand Down Expand Up @@ -715,6 +717,14 @@ class TestHistoryMetrics(testlib.MachineCase):
m.execute("systemctl start pmproxy")
checkEnabled(expected=True)

@testlib.nondestructive
@testlib.skipOstree("no PCP support")
@testlib.onlyImage("valkey is the default only in Fedora", "fedora*")
def testPmProxySettingsRedis(self):
self.machine.execute("systemctl mask valkey")
self.addCleanup(self.machine.execute, "systemctl unmask valkey")
self.testPmProxySettings(customRedisService="redis")


@testlib.nondestructive
class TestCurrentMetrics(testlib.MachineCase):
Expand Down Expand Up @@ -1255,6 +1265,7 @@ class TestMetricsPackages(packagelib.PackageCase):
m = self.machine

redis_service = redisService(m.image)
redis_package = "valkey" if redis_service == "valkey" else "redis"

if m.ostree_image:
self.login_and_go("/metrics")
Expand All @@ -1274,7 +1285,10 @@ class TestMetricsPackages(packagelib.PackageCase):
# HACK: pcp does not clean up correctly on Debian https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=986074
m.execute("rm -f /etc/systemd/system/pmlogger.service.requires/pmlogger_farm.service")
else:
m.execute(f"rpm --erase --verbose cockpit-pcp pcp {redis_service}")
m.execute(f"rpm --erase --verbose cockpit-pcp pcp {redis_package}")
if m.image.startswith("fedora"):
# we removed valkey above, but also remove redis to trigger install dialog
m.execute("rpm --erase --verbose redis")

dummy_service = "[Service]\nExecStart=/bin/sleep infinity\n[Install]\nWantedBy=multi-user.target\n"

Expand All @@ -1293,7 +1307,7 @@ class TestMetricsPackages(packagelib.PackageCase):
self.createPackage("cockpit-pcp", "999", "1", content=cpcp_content, depends="pcp",
postinst="chmod +x /usr/libexec/cockpit-pcp")
self.createPackage("pcp", "999", "1", content=pcp_content, postinst="systemctl daemon-reload")
self.createPackage("redis", "999", "1", content=redis_content, postinst="systemctl daemon-reload")
self.createPackage(redis_package, "999", "1", content=redis_content, postinst="systemctl daemon-reload")
self.enableRepo()
m.execute("pkcon refresh")

Expand Down

0 comments on commit fe9a48e

Please sign in to comment.