Skip to content
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.

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

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

Merged
merged 3 commits into from
Apr 26, 2024

Conversation

tthvo
Copy link
Member

@tthvo tthvo commented Apr 25, 2024

Welcome to Cryostat3! 👋

Before contributing, make sure you have:

  • Read the contributing guidelines
  • Linked a relevant issue which this PR resolves
  • Linked any other relevant issues, PR's, or documentation, if any
  • Resolved all conflicts, if any
  • Rebased your branch PR on top of the latest upstream main branch
  • Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
  • Signed all commits using a GPG signature

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


Fixes: #412

Description of the change:

  • Fetched (pod) discovery node from database if any when handling FOUND event.
  • Fixed deletion fails when deleting pods (i.e. podman), thus causing new containers to be considered duplicate.
    • Need a label to keep track of the container ID so that we just delete by container id instead of fetching for container information. In the case of LOST event, the query will return null -> NullPointerException.
    • With this fix, deleting a pod now works fine.

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
Copy link
Member Author

tthvo commented Apr 25, 2024

/build_test

@tthvo tthvo requested a review from a team April 25, 2024 23:43
@tthvo tthvo marked this pull request as ready for review April 25, 2024 23:43
Copy link

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

Copy link

No OpenAPI schema changes detected.

Copy link

No GraphQL schema changes detected.

Copy link

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

@tthvo
Copy link
Member Author

tthvo commented Apr 25, 2024

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
Copy link
Member

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
Copy link
Member

/build_test

Copy link

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

Copy link

No OpenAPI schema changes detected.

Copy link

No GraphQL schema changes detected.

Copy link

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

@andrewazores andrewazores merged commit c634cdd into cryostatio:main Apr 26, 2024
8 checks passed
@tthvo tthvo deleted the dup-discoverynode branch April 26, 2024 18:01
@tthvo
Copy link
Member Author

tthvo commented Apr 26, 2024

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Bug] Duplicate discovery node for pod in container discovery
2 participants