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.

Sign up for GitHub

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

Jump to bottom

fix(discovery): should send notifications on credential store update #1327

Merged
merged 8 commits into from
Jan 16, 2023
Merged

fix(discovery): should send notifications on credential store update #1327

merged 8 commits into from
Jan 16, 2023
+166 −71

Conversation

tthvo
Copy link
Member

@tthvo tthvo commented Jan 11, 2023 •
edited
Loading

Welcome to Cryostat! 👋

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 the last commit: git commit --amend --signoff

Fixes: cryostatio/cryostat-web#805
Depends on cryostatio/cryostat-core#172

Description of the change:

  • Fixed Discovery Storage to always send WS notifications when new Jvmids are successfully computed.
  • Updated tests to check if notifications are sent (also apply fixes to check nested nested Environement node).

Motivation for the change:

Front-end should be able to display newly computed jvmIds.

How to manually test:

  1. Run CRYOSTAT_IMAGE=quay.io... sh smoketest.sh...
  2. Choose any target with authentication (and no backend-saved credentials). See jvmId is null.
  3. Add credentials.
  4. See the updated jvmIds.

@tthvo tthvo added the fix label Jan 11, 2023
@mergify mergify bot added the safe-to-test label Jan 11, 2023
@tthvo tthvo mentioned this pull request Jan 11, 2023
Merged
7 tasks
@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1327-43d74246dc7d89736b055a44aa8f37d805a6f327 sh smoketest.sh

@tthvo tthvo force-pushed the target-update-noti branch 2 times, most recently from ea22969 to 026a948 Compare January 11, 2023 23:00
@tthvo tthvo marked this pull request as ready for review January 11, 2023 23:01
@tthvo
Copy link
Member Author

tthvo commented Jan 11, 2023 •
edited
Loading

DiscoveryStorage previously skip notifications so I enable it now. Just one issue is that because we are emitting LOST event so web-console will de-select the target (front-end probably can use some hacks to cache and wait for the later FOUND event). Maybe in the future, we can have an UPDATE event instead after #1258?

@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1327-026a9487b3af88aeac419b77777d0d4f73f1adf6 sh smoketest.sh

@tthvo
Copy link
Member Author

tthvo commented Jan 12, 2023 •
edited
Loading

front-end probably can use some hacks to cache and wait for the later FOUND event

Though, this is likely gonna cause trouble since other notifications might come in between. Not a good idea then.

If that succeeds, then a WebSocket notification should be emitted with the updated targets' state. The frontend can figure out how to merge that updated state into its current model, or it can simply drop its model and query to replace it entirely.

cryostatio/cryostat-web#806 (comment)

Since we have only LOST and ADD type for now, any guide on this tho? Maybe a new event UPDATE like above?

@andrewazores
Copy link
Member

Yes, I think there needs to be an UPDATED/MODIFIED even kind added.

@tthvo tthvo marked this pull request as draft January 12, 2023 22:36
@tthvo tthvo mentioned this pull request Jan 13, 2023
Merged
@tthvo tthvo mentioned this pull request Jan 13, 2023
Merged
7 tasks
@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Jan 13, 2023
@github-actions github-actions bot removed the needs-triage Needs thorough attention from code reviewers label Jan 13, 2023
@tthvo tthvo marked this pull request as ready for review January 13, 2023 05:33
@tthvo
Copy link
Member Author

tthvo commented Jan 13, 2023 •
edited
Loading

Ready to review again :D

Need to build with:

This should allow target list to update with the new MODIFIED notification and aovid deselecting it abruptly.

There are some added methods to manual compare ServiceRef that only take into account serviceUrl to find updated node. Probably, any other better ideas to refactor that?

@andrewazores
Copy link
Member

cryostatio/cryostat-core#172 has been merged and should be available momentarily in v2.18.0.

@github-actions
Copy link
Contributor

This PR/issue depends on:

@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Jan 13, 2023
@andrewazores andrewazores removed the needs-triage Needs thorough attention from code reviewers label Jan 13, 2023
@tthvo tthvo removed the needs-triage Needs thorough attention from code reviewers label Jan 13, 2023
@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1327-6c9d18112486844f79f612aac24ed7b84fb02148 sh smoketest.sh

@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Jan 13, 2023
@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1327-4e0ef47945a912cb96e7b0ccda24bf560845c8b5 sh smoketest.sh

@andrewazores andrewazores removed the needs-triage Needs thorough attention from code reviewers label Jan 13, 2023
@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1327-4e0ef47945a912cb96e7b0ccda24bf560845c8b5 sh smoketest.sh

@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Jan 16, 2023
@tthvo tthvo removed the needs-triage Needs thorough attention from code reviewers label Jan 16, 2023
@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Jan 16, 2023
@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1327-7c14e1efd0e490c86979d1d6fb55ba0e1f955d80 sh smoketest.sh

@andrewazores andrewazores removed the needs-triage Needs thorough attention from code reviewers label Jan 16, 2023
@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1327-7c14e1efd0e490c86979d1d6fb55ba0e1f955d80 sh smoketest.sh

@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Jan 16, 2023
@tthvo tthvo removed the needs-triage Needs thorough attention from code reviewers label Jan 16, 2023
@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1327-22bbdb4fd5ffab66ae3236ef8967b6585ad931a3 sh smoketest.sh

@andrewazores andrewazores merged commit 5a1f5af into cryostatio:main Jan 16, 2023
@tthvo tthvo deleted the target-update-noti branch January 16, 2023 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxcao13 maxcao13 maxcao13 left review comments

@andrewazores andrewazores andrewazores approved these changes

@ebaron ebaron Awaiting requested review from ebaron

Assignees
No one assigned
Labels
fix safe-to-test
Projects
No open projects
2.3.0 release
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] Target selection cache with disabled auto-refresh causes outdated jvmIds
3 participants
@tthvo @andrewazores @maxcao13