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

[Patterns] Notification of Dynamic Registration Failure #273

Open
leecalcote opened this issue Sep 9, 2021 · 6 comments
Open

[Patterns] Notification of Dynamic Registration Failure #273

leecalcote opened this issue Sep 9, 2021 · 6 comments
Labels
area/ux help wanted Extra attention is needed issue/willfix This issue will be fixed kind/enhancement Improvement in current feature language/go Golang related

Comments

@leecalcote
Copy link
Member

leecalcote commented Sep 9, 2021

Enhancement Request:
Add info log to verify registration of pattern components (or failure to register pattern components).

Context:
Going forward PatternOps (pattern-based operations) will be used for all of the adapter operations. If an adapter fails to register is capabilities (register its operations), we should denote this in the UI eventually, so the user can see this in the Notification Center.

Immediately, providing an INFO log will be an improvement.

Originally posted by @leecalcote in #269 (comment)

@leecalcote leecalcote added enhancement kind/enhancement Improvement in current feature area/ux language/go Golang related and removed enhancement labels Sep 9, 2021
@tangledbytes
Copy link
Contributor

tangledbytes commented Sep 9, 2021

@leecalcote the issue with sending any notification on failing registration is that "events" bidirectional connection is opened by Meshery Server (which acts like a client to meshery adapter) when it initiates a request on the adapter while the registration process is initiated by the meshery adapter (in this case adapter is the client). Implementing this would be a bit more trickier than sending a WARN event to meshery because there is no channel to send the message.

Here's what we can do though:
Right now, every operation that Meshery Server performs on Adapters is bidirectional but that does not guarantee that channels associated with operations will actually be used for streaming the events infact In case of concurrent operations, channels gets replaced (due to its global nature). This is not ideal but this is what happens.
We can create a new gRPC bidirectional RPC only for the purpose of streaming events. In this case, meshery server will execute this operation during bootup and adapter will store a global events channel which will be used to transmit all sort of events, basically building on the principle "decouple production of events from consumption of events".

Having said that, the my above suggestion seems bad. Can't think of another better solution to the problem at the moment.

// @Revolyssup

@leecalcote
Copy link
Member Author

For now, let's change this to minimally providing an INFO log in stdout. Example of successfully registered Istio adapter:

➜  meshery-istio git:(master) make run
DEBUG=true GOPROXY=direct GOSUMDB=off go run main.go
INFO[2021-09-29T17:06:48-05:00] Adaptor Listening at port: 10000              app=istio-adaptor
INFO[2021-09-29T17:07:46-05:00] Creating instance                             app=istio-adaptor
INFO[2021-09-29T17:07:46-05:00] Listing Operations                            app=istio-adaptor
INFO[2021-09-29T17:07:47-05:00] Creating instance                             app=istio-adaptor
INFO[2021-09-29T17:07:49-05:00] Creating instance                             app=istio-adaptor

No indication of success or failure.

@leecalcote leecalcote added the help wanted Extra attention is needed label Sep 29, 2021
@stale
Copy link

stale bot commented Nov 4, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the issue/stale Issue has not had any activity for an extended period of time label Nov 4, 2021
@leecalcote
Copy link
Member Author

Something to be addressed in v0.7.0

@stale stale bot removed the issue/stale Issue has not had any activity for an extended period of time label Nov 4, 2021
@stale
Copy link

stale bot commented Dec 9, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the issue/stale Issue has not had any activity for an extended period of time label Dec 9, 2021
@leecalcote leecalcote added the issue/willfix This issue will be fixed label Dec 9, 2021
@stale stale bot removed the issue/stale Issue has not had any activity for an extended period of time label Dec 9, 2021
@Revolyssup
Copy link
Contributor

@leecalcote Given the new EventStreamer, we can do this now. The only caviat is since we are not currently persisting or maintaining history, if the client connects after the registration has been done then the client won't be notified. Although if the client is already connected, they would get the notification of components being registered. Should I add that feature and close this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ux help wanted Extra attention is needed issue/willfix This issue will be fixed kind/enhancement Improvement in current feature language/go Golang related
Projects
None yet
Development

No branches or pull requests

3 participants