[cmd/opampsupervisor] Handle OpAMP connection settings #30237

srikanthccv · 2023-12-30T21:48:10Z

Link to tracking Issue:

Part of #21043; based on top of #29848 to add test

Testing:

Added integration test

srikanthccv · 2024-01-17T15:21:11Z

While debugging the test failure I found that wsclient Stop gets indefinitely blocked by ReceiverLoop unless some error occurs (or conn close). I attempted to fix that here open-telemetry/opamp-go#240. @evan-bradley @tigrannajaryan take a look when you get some time.

haoqixu · 2024-01-18T04:26:36Z

While debugging the test failure I found that wsclient Stop gets indefinitely blocked by ReceiverLoop unless some error occurs (or conn close). I attempted to fix that here open-telemetry/opamp-go#240. @evan-bradley @tigrannajaryan take a look when you get some time.

I think open-telemetry/opamp-go#213 will fix this. 🤔

srikanthccv · 2024-01-18T05:04:39Z

Both fixes should be there because ReceiverLoop should stop on the context cancellation as well.

haoqixu · 2024-01-18T06:49:16Z

Both fixes should be there because ReceiverLoop should stop on the context cancellation as well.

It seems that gorilla/websocket doesn't provide a method to unblock the bloking reading of connection. And we need to close the connection to unblock the reading.

If we consider ReceiverLoop as an internal implementation detail of the wsclient and we could gracefully shut down the wsclient, I think it's not a problem.

srikanthccv · 2024-01-19T01:52:55Z

I see these two fixes as complementary. Even as an internal implementation detail, I believe the context cancellation needs to be respected and closing the connection shouldn't be the only way to break out the receiver loop.

cmd/opampsupervisor/go.mod

cmd/opampsupervisor/supervisor/supervisor.go

github-actions · 2024-02-14T05:19:46Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

evan-bradley

Looks good to me, thanks for adding this.

tigrannajaryan · 2024-03-01T15:31:27Z

cmd/opampsupervisor/supervisor/supervisor.go

+	s.config.Server = newServerConfig
+
+	if err := s.startOpAMP(); err != nil {
+		s.logger.Error("Cannot connect to the OpAMP server using the new settings", zap.Error(err))


startOpAMP() initiates but does not wait for successful connection. I think we need to wait for it before we consider the new opamp settings successful. It can be done in a future PR, but would be good to add a TODO here.

tigrannajaryan · 2024-03-01T15:40:03Z

cmd/opampsupervisor/supervisor/supervisor.go

+	return headers
+}
+
+func (s *Supervisor) onOpampConnectionSettings(_ context.Context, settings *protobufs.OpAMPConnectionSettings) error {


Should we try to perform this operation in a separate goroutine so that we don't block here in this callback for the entire duration of switching the connections? Let the callback return quickly and initiate the re-establishing of the connection in a separate goroutine.

This would ensure that if the currently received message contains more data and not just connection settings all that data will be processed. With the current approach I am not sure what exactly will happen.

I think callbacks generally should avoid doing long-lasting blocking operations since they than block other callbacks and the entire opamp operation.

I also think the comment here is completely misleading. We should either implement what the comment says (i.e. the caller should do the reconnection) or fix the comment to sat the callback implementation should do the reconnection. @andykellr @evan-bradley Any thoughts on what you would prefer?

Our agent does not currently support AcceptsOpAMPConnectionSettings so I have not spent much time with this part of the implementation. Looking at the code, the following things are confusing to me:

There are several references to OnRemoteConfig (e.g. "See OnRemoteConfig for the behavior."). This was removed a while ago.

This sentence doesn't make sense:

// If OnOpampConnectionSettings returns nil and then the caller will
// attempt to reconnect to the OpAMP Server using the new settings.

We should clearly state who is responsible for establishing a new connection. I could imagine this being fully implemented in the client library and only have OnOpampConnectionSettings responsible for validating the fields (e.g. endpoint matching a list of acceptable servers or certificate signed by acceptable authority). If this returns nil, the library would attempt to start a new client with the new settings, send an initial status message, and stop and transition to the new connection on success.

If we expect the implementer of the callback to do this, we should describe the process we expect to take place and implement that in tests and the example agent.

If we do this in a separate goroutine like the example agent in the opamp-go library, we have no way of knowing if it will be successful and should be accepted. Returning nil from the callback will cause the settings to be accepted. If we create a separate goroutine we will have to wait for it before returning here.

https://github.com/open-telemetry/opamp-go/blob/7e92da0f17ef9f2fd0a387dd6b62b451c80f4207/client/internal/receivedprocessor.go#L198-L202

It is not possible to confirm success until the client is started and an initial status message is sent.

OnOpampConnectionSettingsAccepted references OnOpampConnectionSettingsOffer which was renamed OnOpampConnectionSettings

This would ensure that if the currently received message contains more data and not just connection settings all that data will be processed. With the current approach I am not sure what exactly will happen.

onOpampConnectionSettings is the last part of the message processing in client implementation.

If we do this in a separate goroutine like the example agent in the opamp-go library, we have no way of knowing if it will be successful and should be accepted.

Right, the returned value from this callback indicates the reject/accept status of the connection settings. I don't see how we can make this async without changing how the client is expected to handle OnOpampConnectionSettings. Also, we don't report the error as described in OnOpampConnectionSettings open-telemetry/opamp-spec#164.

We should clearly state who is responsible for establishing a new connection. I could imagine this being fully implemented in the client library and only have OnOpampConnectionSettings responsible for validating the fields (e.g. endpoint matching a list of acceptable servers or certificate signed by acceptable authority). If this returns nil, the library would attempt to start a new client with the new settings, send an initial status message, and stop and transition to the new connection on success.

I think this was the original intent. However, I am not sure this is the best approach. It is also not how the example implementation works and I don't see good arguments against how the example is implemented today.

To summarize this is what the example is supposed to do:

OnOpampConnectionSettings callback called when new settings are offered by the Server.

The callback implementation first pre-verifies the settings (e.g. check certificates, etc).

If checks in (2) pass the callback implementation logs that it is starting to reconnect, creates a reconnection goroutine and returns from the callback.

Reconnection goroutine stops the Client, creates a new Client with new connection settings and wait for successful OnConnect() callback.

If OnConnect() is not called within a predefined period of time reconnection goroutine assumes the new connection settings are bad, reverts to the old connection settings and re-creates the Client again.

We get rid of OnOpampConnectionSettingsAccepted(), it is not needed.

The example implementation of steps 4 and 5 is not complete today (e.g not waiting for OnConnect), but can be modified to match what I described.

Thoughts?

I created an issue for this open-telemetry/opamp-go#261

Let's also discuss today in our call.

FYI, open-telemetry/opamp-go#266 removes OnOpampConnectionSettingsAccepted callback.

Updated the implementation to run in a separate goroutine and use OnConnect to determine the connection status.

MovieStoreGuy

I don't have much to add here, I don't see any potential issues here and @tigrannajaryan has more context here.

MovieStoreGuy · 2024-03-04T01:45:42Z

cmd/opampsupervisor/supervisor/config/config.go

-	ReportsOwnMetrics      *bool `mapstructure:"reports_own_metrics"`
-	ReportsHealth          *bool `mapstructure:"reports_health"`
-	ReportsRemoteConfig    *bool `mapstructure:"reports_remote_config"`
+	AcceptsRemoteConfig            *bool `mapstructure:"accepts_remote_config"`


Sorry I had missed this in earlier, and isn't strictly relevant for the PR but I am curious, what does a pointer value add here?

Since the zero-value for bool is false but the default value may be true, using a pointer instead allows us to use a nil pointer value as a way to represent when the user didn't pass a value in and a default should be used.

MovieStoreGuy · 2024-03-04T01:49:54Z

cmd/opampsupervisor/supervisor/supervisor.go

+
+	newServerConfig := &config.OpAMPServer{}
+
+	if settings.DestinationEndpoint != "" {


I wonder if there any value in using some of the confmap work to merge these values together instead of needing to update each one explicitly?

I am not very familiar with confmap work. I will take a look at it and If it helps here I can send a separate PR for it.

github-actions · 2024-03-20T05:19:59Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

**Description:** <Describe what has changed.> - Linting Errors: [https://productionresultssa5.blob.core.windows.net/actions-results/44e4093a-f4c1-4e35-af4f-d630ea9b8d68/workflow-job-run-071cf28a-9760-544e-287f-c0f9ae5a03a4/logs/job/job-logs.txt?rsct=text%2Fplain&se=2024-03-25T12%3A45%3A21Z&sig=fRzhKwEpzDQDQZyUYwvEY%2BhkcFWOD3IY0EeCOE47AeU%3D&sp=r&spr=https&sr=b&st=2024-03-25T12%3A35%3A16Z&sv=2021-12-02](url) - some linting errors will be fixed by #30237 **Link to tracking Issue:** <Issue number if applicable> - #31240 **Testing:** <Describe what testing was performed and which tests were added.> **Documentation:** <Describe the documentation added.>

MovieStoreGuy · 2024-04-09T07:48:50Z

Could I ask you to clean up the mod file?

evan-bradley and others added 7 commits December 14, 2023 12:59

Implement Collector bootstrapping in the Supervisor

826f613

Address PR feedback

99a1bda

Add issue todo

3add507

Add a changelog

68a633f

Fix lint errors

45d1533

Handle OpAMP connection settings

5e06efa

Part of open-telemetry#21043

Add capability to supervisor and e2e test

1f7a776

github-actions bot added the cmd/opampsupervisor label Dec 30, 2023

github-actions bot requested review from atoulme, evan-bradley and tigrannajaryan December 30, 2023 21:48

fix lint and pass headers

0a8c0bb

srikanthccv mentioned this pull request Dec 31, 2023

support http (security) headers in opamp supervisor #30137

Closed

srikanthccv added 3 commits January 13, 2024 13:27

Merge branch 'main' into issue-21043

e64019c

Fix conflicts

e910234

Fix checks

c9570df

srikanthccv added 4 commits January 20, 2024 23:14

resolve conflicts

2c025cf

mod tidy

a51fbe6

Resolve conflicts

6a10718

Update to latest main and temporarily skip DeadlineExceeded

c40d622

srikanthccv commented Jan 25, 2024

View reviewed changes

cmd/opampsupervisor/go.mod Outdated Show resolved Hide resolved

srikanthccv commented Jan 25, 2024

View reviewed changes

cmd/opampsupervisor/supervisor/supervisor.go Show resolved Hide resolved

srikanthccv marked this pull request as ready for review January 25, 2024 19:08

srikanthccv requested a review from a team January 25, 2024 19:08

github-actions bot assigned MovieStoreGuy Jan 25, 2024

github-actions bot added the Stale label Feb 14, 2024

evan-bradley removed the Stale label Feb 14, 2024

srikanthccv added 3 commits February 21, 2024 21:40

Resolve conflicts

a9a1e85

Add review comments

eeba0e3

Merge branch 'main' into issue-21043

7429102

srikanthccv requested a review from evan-bradley February 21, 2024 17:02

evan-bradley approved these changes Feb 27, 2024

View reviewed changes

Merge branch 'main' into issue-21043

27d3e0b

tigrannajaryan reviewed Mar 1, 2024

View reviewed changes

MovieStoreGuy reviewed Mar 4, 2024

View reviewed changes

tigrannajaryan mentioned this pull request Mar 5, 2024

OnOpampConnectionSettings/Accepted callbacks need re-thinking open-telemetry/opamp-go#261

Open

github-actions bot added the Stale label Mar 20, 2024

led0nk mentioned this pull request Mar 25, 2024

[chore] lint fixes in cmd/telemetrygen & cmd/opampsupervisor #31932

Merged

songy23 mentioned this pull request Mar 25, 2024

[chore] Fix new lint issues with golangci-lint v1.56.1 #31240

Closed

github-actions bot removed the Stale label Mar 29, 2024

srikanthccv added 3 commits April 3, 2024 22:27

Resolve conflicts

a0b4ef1

Rely on OnConnectFunc for conn status

aab7432

Fix lint

36a16cd

srikanthccv added 2 commits April 9, 2024 14:21

Resolve conflicts

3ba9be9

Merge branch 'main' into issue-21043

7d8e3e2

MovieStoreGuy merged commit 4edca20 into open-telemetry:main Apr 9, 2024
169 of 170 checks passed

github-actions bot added this to the next release milestone Apr 9, 2024

evan-bradley mentioned this pull request Apr 16, 2024

[cmd/opampsupervisor] Handle OpAMP connection settings messages #21043

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cmd/opampsupervisor] Handle OpAMP connection settings #30237

[cmd/opampsupervisor] Handle OpAMP connection settings #30237

srikanthccv commented Dec 30, 2023 •

edited

Loading

srikanthccv commented Jan 17, 2024 •

edited

Loading

haoqixu commented Jan 18, 2024

srikanthccv commented Jan 18, 2024

haoqixu commented Jan 18, 2024

srikanthccv commented Jan 19, 2024

github-actions bot commented Feb 14, 2024

evan-bradley left a comment

tigrannajaryan Mar 1, 2024

tigrannajaryan Mar 1, 2024 •

edited

Loading

andykellr Mar 1, 2024

srikanthccv Mar 3, 2024

tigrannajaryan Mar 5, 2024

tigrannajaryan Mar 5, 2024

tigrannajaryan Mar 28, 2024

srikanthccv Apr 3, 2024 •

edited

Loading

MovieStoreGuy left a comment

MovieStoreGuy Mar 4, 2024

evan-bradley Mar 5, 2024

MovieStoreGuy Mar 4, 2024

srikanthccv Apr 3, 2024

github-actions bot commented Mar 20, 2024

MovieStoreGuy commented Apr 9, 2024


		newServerConfig := &config.OpAMPServer{}

		if settings.DestinationEndpoint != "" {

[cmd/opampsupervisor] Handle OpAMP connection settings #30237

[cmd/opampsupervisor] Handle OpAMP connection settings #30237

Conversation

srikanthccv commented Dec 30, 2023 • edited Loading

srikanthccv commented Jan 17, 2024 • edited Loading

haoqixu commented Jan 18, 2024

srikanthccv commented Jan 18, 2024

haoqixu commented Jan 18, 2024

srikanthccv commented Jan 19, 2024

github-actions bot commented Feb 14, 2024

evan-bradley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigrannajaryan Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikanthccv Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

MovieStoreGuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 20, 2024

MovieStoreGuy commented Apr 9, 2024

srikanthccv commented Dec 30, 2023 •

edited

Loading

srikanthccv commented Jan 17, 2024 •

edited

Loading

tigrannajaryan Mar 1, 2024 •

edited

Loading

srikanthccv Apr 3, 2024 •

edited

Loading