Repository: open-telemetry/opamp-go
Pull request #246 (2024-01-19 16:20:09)

Changes to set the ServerOfferedVersion and ServerOfferedHash always
This PR will fix the bug https://github.com/open-telemetry/opamp-go/issues/245, the ServerOfferedVersion and ServerOfferedHash are only set when the package is sent from the server the very first time. 

The changes in this PR will make sure that the info is set all the time. 

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/open-telemetry/opamp-go/pull/246?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`fcfa095`)](https://app.codecov.io/gh/open-telemetry/opamp-go/commit/fcfa0954c545e1b842a3d64adada28d0c032a518?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) 72.50% compared to head [(`1038015`)](https://app.codecov.io/gh/open-telemetry/opamp-go/pull/246?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) 72.68%.


<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #246      +/-   ##
==========================================
+ Coverage   72.50%   72.68%   +0.18%     
==========================================
  Files          25       25              
  Lines        2022     2021       -1     
==========================================
+ Hits         1466     1469       +3     
+ Misses        446      443       -3     
+ Partials      110      109       -1     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/open-telemetry/opamp-go/pull/246?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry).

srikanthccv: Please add a test case. Let try to include cases one where the installation of new version fails and another that succeeds. 
phanidevavarapu: > Please add a test case. Let's try to include cases one where the installation of the new version fails and another that succeeds.

@srikanthccv the unit test you suggested will cut across all many functions with in this file itself, it will be huge undertaking compared to the code change that I made. 
srikanthccv: Regardless of the size of the change, we should add tests. That's how we prove the change actually fixes it and having a test ensures the issue will not appear again in future. 
tigrannajaryan: > Regardless of the size of the change, we should add tests. That's how we prove the change actually fixes it and having a test ensures the issue will not appear again in future.

+1.  For all bug fixes we want tests that demonstrate that there is a bug (tests fail before the fix) and that the PR fixes the bug (tests pass after the fix).

I understand this adds more work, but this is important for maintaining the quality of the code and being confident about bug fixes.
srikanthccv: @phanidevavarapu I had opened a PR to add tests against your fork here https://github.com/phanidevavarapu/opamp-go/pull/1. Did you get a chance to look at it?
srikanthccv: Can you fix the conflicts and update the packagessyncer.go because I commented out the lines to show tests fail.
phanidevavarapu: @srikanthccv updated the PR, There is a test failing, but I don't see any failures in local. 
tigrannajaryan: TestOfferUpdatedVersion/ws test got stuck, likely an indication of a real problem.
phanidevavarapu: @tigrannajaryan I am not sure how to approach this now, tests seems to failing and passing at random. Any help to have everything green is really appreciated. 
srikanthccv: While the individual test case `go test -timeout 30s -run ^TestOfferUpdatedVersion$ github.com/open-telemetry/opamp-go/client` passes successfully, running the entire test suite fails with a timeout failure consistently. We need to check why the collective test execution fails and fix it.
phanidevavarapu: Can we approve and merge this ticket?
srikanthccv: We need to fix the failing test first. Were you able to find out why the test times out?
srikanthccv: The changes look good to me. I tried running the workflow multiple times to see if shows flaky behaviour but it passed all runs. I still suspect the test is flaky since it passes now without any changes to the test case.
tigrannajaryan: @phanidevavarapu this looks good to me, thank you. Can you please resolve the conflicts so that we can merge?
phanidevavarapu: @tigrannajaryan the merge conflicts are resolved, please go ahead and merge.

Patch:
diff --git a/client/internal/packagessyncer.go b/client/internal/packagessyncer.go
index bdb52fd..074fc83 100644
--- a/client/internal/packagessyncer.go
+++ b/client/internal/packagessyncer.go
@@ -150,12 +150,13 @@ func (s *packagesSyncer) syncPackage(
 	if status == nil {
 		// This package has no status. Create one.
 		status = &protobufs.PackageStatus{
-			Name:                 pkgName,
-			ServerOfferedVersion: pkgAvail.Version,
-			ServerOfferedHash:    pkgAvail.Hash,
+			Name: pkgName,
 		}
 		s.statuses.Packages[pkgName] = status
 	}
+	// Update the newly offered package Version and Hash
+	status.ServerOfferedVersion = pkgAvail.Version
+	status.ServerOfferedHash = pkgAvail.Hash
 
 	pkgLocal, err := s.localState.PackageState(pkgName)
 	if err != nil {
@@ -233,7 +234,9 @@ func (s *packagesSyncer) syncPackageFile(
 }
 
 // shouldDownloadFile returns true if the file should be downloaded.
-func (s *packagesSyncer) shouldDownloadFile(ctx context.Context, packageName string, file *protobufs.DownloadableFile) (bool, error) {
+func (s *packagesSyncer) shouldDownloadFile(ctx context.Context,
+	packageName string,
+	file *protobufs.DownloadableFile) (bool, error) {
 	fileContentHash, err := s.localState.FileContentHash(packageName)
 
 	if err != nil {


Test patch:
diff --git a/client/clientimpl_test.go b/client/clientimpl_test.go
index 288c754..ba308cb 100644
--- a/client/clientimpl_test.go
+++ b/client/clientimpl_test.go
@@ -533,7 +533,7 @@ func TestSetEffectiveConfig(t *testing.T) {
 			t,
 			func() bool {
 				return rcvConfig.Load() != nil &&
-					proto.Equal(sendConfig, rcvConfig.Load().(*protobufs.EffectiveConfig))
+						proto.Equal(sendConfig, rcvConfig.Load().(*protobufs.EffectiveConfig))
 			},
 		)
 
@@ -546,7 +546,7 @@ func TestSetEffectiveConfig(t *testing.T) {
 			t,
 			func() bool {
 				return rcvConfig.Load() != nil &&
-					proto.Equal(sendConfig, rcvConfig.Load().(*protobufs.EffectiveConfig))
+						proto.Equal(sendConfig, rcvConfig.Load().(*protobufs.EffectiveConfig))
 			},
 		)
 
@@ -765,7 +765,7 @@ func TestServerOfferConnectionSettings(t *testing.T) {
 				},
 
 				OnOpampConnectionSettingsFunc: func(
-					ctx context.Context, settings *protobufs.OpAMPConnectionSettings,
+						ctx context.Context, settings *protobufs.OpAMPConnectionSettings,
 				) error {
 					assert.True(t, proto.Equal(opampSettings, settings))
 					atomic.AddInt64(&gotOpampSettings, 1)
@@ -773,10 +773,10 @@ func TestServerOfferConnectionSettings(t *testing.T) {
 				},
 			},
 			Capabilities: protobufs.AgentCapabilities_AgentCapabilities_ReportsOwnTraces |
-				protobufs.AgentCapabilities_AgentCapabilities_ReportsOwnMetrics |
-				protobufs.AgentCapabilities_AgentCapabilities_ReportsOwnLogs |
-				protobufs.AgentCapabilities_AgentCapabilities_AcceptsOtherConnectionSettings |
-				protobufs.AgentCapabilities_AgentCapabilities_AcceptsOpAMPConnectionSettings,
+					protobufs.AgentCapabilities_AgentCapabilities_ReportsOwnMetrics |
+					protobufs.AgentCapabilities_AgentCapabilities_ReportsOwnLogs |
+					protobufs.AgentCapabilities_AgentCapabilities_AcceptsOtherConnectionSettings |
+					protobufs.AgentCapabilities_AgentCapabilities_AcceptsOpAMPConnectionSettings,
 		}
 		settings.OpAMPServerURL = "ws://" + srv.Endpoint
 		prepareClient(t, &settings, client)
@@ -823,7 +823,7 @@ func TestClientRequestConnectionSettings(t *testing.T) {
 			settings := types.StartSettings{
 				Callbacks: types.CallbacksStruct{
 					OnOpampConnectionSettingsFunc: func(
-						ctx context.Context, settings *protobufs.OpAMPConnectionSettings,
+							ctx context.Context, settings *protobufs.OpAMPConnectionSettings,
 					) error {
 						assert.True(t, proto.Equal(opampSettings, settings))
 						atomic.AddInt64(&clientGotOpampSettings, 1)
@@ -929,7 +929,7 @@ func TestReportAgentHealth(t *testing.T) {
 		settings := types.StartSettings{
 			OpAMPServerURL: "ws://" + srv.Endpoint,
 			Capabilities: protobufs.AgentCapabilities_AgentCapabilities_ReportsEffectiveConfig |
-				protobufs.AgentCapabilities_AgentCapabilities_ReportsHealth,
+					protobufs.AgentCapabilities_AgentCapabilities_ReportsHealth,
 		}
 		prepareClient(t, &settings, client)
 
@@ -1090,7 +1090,7 @@ func verifyRemoteConfigUpdate(t *testing.T, successCase bool, expectStatus *prot
 				},
 			},
 			Capabilities: protobufs.AgentCapabilities_AgentCapabilities_AcceptsRemoteConfig |
-				protobufs.AgentCapabilities_AgentCapabilities_ReportsRemoteConfig,
+					protobufs.AgentCapabilities_AgentCapabilities_ReportsRemoteConfig,
 		}
 		prepareClient(t, &settings, client)
 
@@ -1207,6 +1207,50 @@ type packageTestCase struct {
 
 const packageUpdateErrorMsg = "cannot update packages"
 
+func assertPackageStatus(t *testing.T,
+		testCase packageTestCase,
+		msg *protobufs.AgentToServer) (*protobufs.ServerToAgent, bool) {
+	expectedStatusReceived := false
+
+	status := msg.PackageStatuses
+	require.NotNil(t, status)
+	assert.EqualValues(t, testCase.expectedStatus.ServerProvidedAllPackagesHash, status.ServerProvidedAllPackagesHash)
+
+	if testCase.expectedError != "" {
+		assert.EqualValues(t, testCase.expectedError, status.ErrorMessage)
+		return &protobufs.ServerToAgent{InstanceUid: msg.InstanceUid}, true
+	}
+
+	// Verify individual package statuses.
+	for name, pkgExpected := range testCase.expectedStatus.Packages {
+		pkgStatus := status.Packages[name]
+		if pkgStatus == nil {
+			// Package status not yet included in the report.
+			continue
+		}
+		switch pkgStatus.Status {
+		case protobufs.PackageStatusEnum_PackageStatusEnum_InstallFailed:
+			assert.Contains(t, pkgStatus.ErrorMessage, pkgExpected.ErrorMessage)
+
+		case protobufs.PackageStatusEnum_PackageStatusEnum_Installed:
+			assert.EqualValues(t, pkgExpected.AgentHasHash, pkgStatus.AgentHasHash)
+			assert.EqualValues(t, pkgExpected.AgentHasVersion, pkgStatus.AgentHasVersion)
+			assert.Empty(t, pkgStatus.ErrorMessage)
+		default:
+			assert.Empty(t, pkgStatus.ErrorMessage)
+		}
+		assert.EqualValues(t, pkgExpected.ServerOfferedHash, pkgStatus.ServerOfferedHash)
+		assert.EqualValues(t, pkgExpected.ServerOfferedVersion, pkgStatus.ServerOfferedVersion)
+
+		if pkgStatus.Status == pkgExpected.Status {
+			expectedStatusReceived = true
+			assert.Len(t, status.Packages, len(testCase.available.Packages))
+		}
+	}
+
+	return &protobufs.ServerToAgent{InstanceUid: msg.InstanceUid}, expectedStatusReceived
+}
+
 func verifyUpdatePackages(t *testing.T, testCase packageTestCase) {
 	testClients(t, func(t *testing.T, client OpAMPClient) {
 
@@ -1242,7 +1286,7 @@ func verifyUpdatePackages(t *testing.T, testCase packageTestCase) {
 			},
 			PackagesStateProvider: localPackageState,
 			Capabilities: protobufs.AgentCapabilities_AgentCapabilities_AcceptsPackages |
-				protobufs.AgentCapabilities_AgentCapabilities_ReportsPackageStatuses,
+					protobufs.AgentCapabilities_AgentCapabilities_ReportsPackageStatuses,
 		}
 		prepareClient(t, &settings, client)
 
@@ -1264,48 +1308,10 @@ func verifyUpdatePackages(t *testing.T, testCase packageTestCase) {
 
 		// ---> Server
 		// Wait for the expected package statuses to be received.
-		srv.EventuallyExpect("full PackageStatuses",
-			func(msg *protobufs.AgentToServer) (*protobufs.ServerToAgent, bool) {
-				expectedStatusReceived := false
-
-				status := msg.PackageStatuses
-				require.NotNil(t, status)
-				assert.EqualValues(t, testCase.expectedStatus.ServerProvidedAllPackagesHash, status.ServerProvidedAllPackagesHash)
-
-				if testCase.expectedError != "" {
-					assert.EqualValues(t, testCase.expectedError, status.ErrorMessage)
-					return &protobufs.ServerToAgent{InstanceUid: msg.InstanceUid}, true
-				}
-
-				// Verify individual package statuses.
-				for name, pkgExpected := range testCase.expectedStatus.Packages {
-					pkgStatus := status.Packages[name]
-					if pkgStatus == nil {
-						// Package status not yet included in the report.
-						continue
-					}
-					switch pkgStatus.Status {
-					case protobufs.PackageStatusEnum_PackageStatusEnum_InstallFailed:
-						assert.Contains(t, pkgStatus.ErrorMessage, pkgExpected.ErrorMessage)
-
-					case protobufs.PackageStatusEnum_PackageStatusEnum_Installed:
-						assert.EqualValues(t, pkgExpected.AgentHasHash, pkgStatus.AgentHasHash)
-						assert.EqualValues(t, pkgExpected.AgentHasVersion, pkgStatus.AgentHasVersion)
-						assert.Empty(t, pkgStatus.ErrorMessage)
-					default:
-						assert.Empty(t, pkgStatus.ErrorMessage)
-					}
-					assert.EqualValues(t, pkgExpected.ServerOfferedHash, pkgStatus.ServerOfferedHash)
-					assert.EqualValues(t, pkgExpected.ServerOfferedVersion, pkgStatus.ServerOfferedVersion)
-
-					if pkgStatus.Status == pkgExpected.Status {
-						expectedStatusReceived = true
-						assert.Len(t, status.Packages, len(testCase.available.Packages))
-					}
-				}
-
-				return &protobufs.ServerToAgent{InstanceUid: msg.InstanceUid}, expectedStatusReceived
-			})
+		srv.EventuallyExpect("full PackageStatuses", func(msg *protobufs.AgentToServer) (*protobufs.ServerToAgent,
+				bool) {
+			return assertPackageStatus(t, testCase, msg)
+		})
 
 		if syncerDoneCh != nil {
 			// Wait until all syncing is done.
@@ -1464,7 +1470,7 @@ func TestMissingCapabilities(t *testing.T) {
 					assert.Nil(t, msg.PackagesAvailable)
 				},
 				OnOpampConnectionSettingsFunc: func(
-					ctx context.Context, settings *protobufs.OpAMPConnectionSettings,
+						ctx context.Context, settings *protobufs.OpAMPConnectionSettings,
 				) error {
 					assert.Fail(t, "should not be called since capability is not set to accept it")
 					return nil
@@ -1526,7 +1532,7 @@ func TestMissingPackagesStateProvider(t *testing.T) {
 		settings := types.StartSettings{
 			Callbacks: types.CallbacksStruct{},
 			Capabilities: protobufs.AgentCapabilities_AgentCapabilities_AcceptsPackages |
-				protobufs.AgentCapabilities_AgentCapabilities_ReportsPackageStatuses,
+					protobufs.AgentCapabilities_AgentCapabilities_ReportsPackageStatuses,
 		}
 		prepareClient(t, &settings, client)
 
@@ -1555,6 +1561,97 @@ func TestMissingPackagesStateProvider(t *testing.T) {
 	})
 }
 
+func TestOfferUpdatedVersion(t *testing.T) {
+
+	downloadSrv := createDownloadSrv(t)
+	defer downloadSrv.Close()
+
+	testCase := createPackageTestCase("offer new version", downloadSrv)
+
+	testClients(t, func(t *testing.T, client OpAMPClient) {
+
+		localPackageState := internal.NewInMemPackagesStore()
+		srv := internal.StartMockServer(t)
+		srv.EnableExpectMode()
+
+		onMessageFunc := func(ctx context.Context, msg *types.MessageData) {
+			if msg.PackageSyncer != nil {
+				msg.PackageSyncer.Done()
+				err := msg.PackageSyncer.Sync(ctx)
+				require.NoError(t, err)
+			}
+		}
+
+		// Start a client.
+		settings := types.StartSettings{
+			OpAMPServerURL: "ws://" + srv.Endpoint,
+			Callbacks: types.CallbacksStruct{
+				OnMessageFunc: onMessageFunc,
+			},
+			PackagesStateProvider: localPackageState,
+			Capabilities: protobufs.AgentCapabilities_AgentCapabilities_AcceptsPackages |
+					protobufs.AgentCapabilities_AgentCapabilities_ReportsPackageStatuses,
+		}
+		prepareClient(t, &settings, client)
+
+		// Client --->
+		assert.NoError(t, client.Start(context.Background(), settings))
+
+		// ---> Server
+		srv.Expect(func(msg *protobufs.AgentToServer) *protobufs.ServerToAgent {
+			assert.EqualValues(t, 0, msg.SequenceNum)
+			// Send the packages to the Agent.
+			return &protobufs.ServerToAgent{
+				InstanceUid:       msg.InstanceUid,
+				PackagesAvailable: testCase.available,
+			}
+		})
+
+		// The Agent will try to install the packages and will send the status
+		// report about it back to the Server.
+		// ---> Server
+		// Wait for the expected package statuses to be received.
+		srv.EventuallyExpect("full PackageStatuses", func(msg *protobufs.AgentToServer) (*protobufs.ServerToAgent,
+				bool) {
+			return assertPackageStatus(t, testCase, msg)
+		})
+
+		newByte := []byte{45}
+		testCase.available.Packages["package1"].Version = "1.0.1"
+		testCase.available.AllPackagesHash = append(testCase.available.AllPackagesHash, newByte...)
+		testCase.expectedStatus.Packages["package1"].AgentHasVersion = "1.0.1"
+		testCase.expectedStatus.Packages["package1"].ServerOfferedVersion = "1.0.1"
+		testCase.expectedStatus.ServerProvidedAllPackagesHash = append(testCase.expectedStatus.ServerProvidedAllPackagesHash, newByte...)
+
+		_ = client.SetHealth(&protobufs.ComponentHealth{})
+
+		// ---> Server
+		srv.Expect(func(msg *protobufs.AgentToServer) *protobufs.ServerToAgent {
+			// Send the packages to the Agent.
+			return &protobufs.ServerToAgent{
+				InstanceUid:       msg.InstanceUid,
+				PackagesAvailable: testCase.available,
+			}
+		})
+		// The Agent will try to install the packages and will send the status
+		// report about it back to the Server.
+
+		// ---> Server
+		// Wait for the expected package statuses to be received.
+		srv.EventuallyExpect("full PackageStatuses updated version", func(msg *protobufs.AgentToServer) (*protobufs.ServerToAgent,
+				bool) {
+			return assertPackageStatus(t, testCase, msg)
+		})
+
+		// Shutdown the Server.
+		srv.Close()
+
+		// Shutdown the client.
+		err := client.Stop(context.Background())
+		assert.NoError(t, err)
+	})
+}
+
 func TestReportCustomCapabilities(t *testing.T) {
 	testClients(t, func(t *testing.T, client OpAMPClient) {