cryostatio / cryostat

Secure JDK Flight Recorder management for containerized JVMs
https://cryostat.io
Other
8 stars 8 forks source link

fix(discovery): fix duplicate pod nodes in container discovery #420

Closed tthvo closed 2 months ago

tthvo commented 2 months ago

Welcome to Cryostat3! 👋

Before contributing, make sure you have:

To recreate commits with GPG signature git fetch upstream && git rebase --force --gpg-sign upstream/main


Fixes: #412

Description of the change:

image

Motivation for the change:

See #412

How to manually test:

podman pod create --replace --name cryostat-pod

podman run \
        --name jmxquarkus \
        --pod cryostat-pod \
        --label io.cryostat.discovery="true" \
        --label io.cryostat.jmxPort="51423" \
        --env QUARKUS_HTTP_PORT=10012 \
        --rm -d quay.io/roberttoyonaga/jmx:jmxquarkus@sha256:b067f29faa91312d20d43c55d194a2e076de7d0d094da3d43ee7d2b2b5a6f100

podman run \
        --name vertx-fib-demo-0 \
        --env HTTP_PORT=8079 \
        --env JMX_PORT=9089 \
        --env START_DELAY=60 \
        --pod cryostat-pod \
        --label io.cryostat.discovery="true" \
        --label io.cryostat.jmxHost="vertx-fib-demo-0" \
        --label io.cryostat.jmxPort="9089" \
        --rm -d quay.io/andrewazores/vertx-fib-demo:0.13.1
tthvo commented 2 months ago

/build_test

github-actions[bot] commented 2 months ago

Workflow started at 4/25/2024, 7:43:38 PM. View Actions Run.

github-actions[bot] commented 2 months ago

No OpenAPI schema changes detected.

github-actions[bot] commented 2 months ago

No GraphQL schema changes detected.

github-actions[bot] commented 2 months ago

CI build and push: All tests pass ✅ (JDK17) https://github.com/cryostatio/cryostat3/actions/runs/8840828545

tthvo commented 2 months ago

https://github.com/cryostatio/cryostat3/blob/2027869c0c4f033cf3175d0a687d181940a59ae4/src/main/java/io/cryostat/discovery/ContainerDiscovery.java#L197

I think the diff operation here should be done against persisted targets instead? Otherwise, if cryostat is restarted, it will cause all previously observed containers to be pruned. I will save that for another PR tho to keep this one minimal...

andrewazores commented 2 months ago

https://github.com/cryostatio/cryostat3/blob/2027869c0c4f033cf3175d0a687d181940a59ae4/src/main/java/io/cryostat/discovery/ContainerDiscovery.java#L197

I think the diff operation here should be done against persisted targets instead? Otherwise, if cryostat is restarted, it will cause all previously observed containers to be pruned. I will save that for another PR tho to keep this one minimal...

I don't think it's that big of a problem, since the correct data will end up getting restored into the database again when Cryostat does come back and query the container platform again. They'll just end up with new database IDs, which seems like a minor annoyance. If you'd like to fix that in another PR I'd be happy to review it though.

andrewazores commented 2 months ago

/build_test

github-actions[bot] commented 2 months ago

Workflow started at 4/26/2024, 11:46:03 AM. View Actions Run.

github-actions[bot] commented 2 months ago

No OpenAPI schema changes detected.

github-actions[bot] commented 2 months ago

No GraphQL schema changes detected.

github-actions[bot] commented 2 months ago

CI build and push: All tests pass ✅ (JDK17) https://github.com/cryostatio/cryostat3/actions/runs/8850852121

tthvo commented 2 months ago

https://github.com/cryostatio/cryostat3/blob/2027869c0c4f033cf3175d0a687d181940a59ae4/src/main/java/io/cryostat/discovery/ContainerDiscovery.java#L197

I think the diff operation here should be done against persisted targets instead? Otherwise, if cryostat is restarted, it will cause all previously observed containers to be pruned. I will save that for another PR tho to keep this one minimal...

I don't think it's that big of a problem, since the correct data will end up getting restored into the database again when Cryostat does come back and query the container platform again. They'll just end up with new database IDs, which seems like a minor annoyance. If you'd like to fix that in another PR I'd be happy to review it though.

Sure! And actually, from another look, I think it would instead leave stale (removed containers) targets intact and cause duplicate key violation when cryostat comes back up again. I will work a PR for that...