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

Correctly handle status compression on the server side #105

Merged
merged 1 commit into from
Jul 28, 2022
Merged

Correctly handle status compression on the server side #105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions client/internal/clientcommon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,12 +206,7 @@ func (c *ClientCommon) PrepareFirstMessage(ctx context.Context) error {
msg.EffectiveConfig = cfg
msg.RemoteConfigStatus = c.ClientSyncedState.RemoteConfigStatus()
msg.PackageStatuses = c.ClientSyncedState.PackageStatuses()

if c.PackagesStateProvider != nil {
// We have a state provider, so package related capabilities can work.
msg.Capabilities |= protobufs.AgentCapabilities_AcceptsPackages
msg.Capabilities |= protobufs.AgentCapabilities_ReportsPackageStatuses
}
msg.Capabilities = c.Capabilities
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ensures that the agent correctly reports the capabilities. I can extract this fix into a separate PR, but probably not worth it since it only became visible in this PR.

},
)
return nil
Expand Down
30 changes: 24 additions & 6 deletions internal/examples/server/data/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,17 +194,35 @@ func (agent *Agent) updateEffectiveConfig(
}
}

func (agent *Agent) hasCapability(capability protobufs.AgentCapabilities) bool {
return agent.Status.Capabilities&capability != 0
}

func (agent *Agent) processStatusUpdate(
newStatus *protobufs.AgentToServer,
response *protobufs.ServerToAgent,
) {
if agent.Status != nil && agent.Status.SequenceNum+1 != newStatus.SequenceNum {
// We lost the previous status update. Request full status update from the agent.
response.Flags |= protobufs.ServerToAgent_ReportFullState
}
// We don't have any status for this Agent, or we lost the previous status update from the Agent, so our
// current status is not up-to-date.
lostPreviousUpdate := (agent.Status == nil) || (agent.Status != nil && agent.Status.SequenceNum+1 != newStatus.SequenceNum)

agentDescrChanged := agent.updateStatusField(newStatus)

// Check if any fields were omitted in the status report.
effectiveConfigOmitted := agent.hasCapability(protobufs.AgentCapabilities_ReportsEffectiveConfig) && newStatus.EffectiveConfig == nil
packageStatusesOmitted := agent.hasCapability(protobufs.AgentCapabilities_ReportsPackageStatuses) && newStatus.PackageStatuses == nil
remoteConfigStatusOmitted := agent.hasCapability(protobufs.AgentCapabilities_AcceptsRemoteConfig) && newStatus.RemoteConfigStatus == nil
healthOmitted := agent.hasCapability(protobufs.AgentCapabilities_ReportsHealth) && newStatus.Health == nil

// True if the status was not fully reported.
statusIsCompressed := effectiveConfigOmitted || packageStatusesOmitted || remoteConfigStatusOmitted || healthOmitted

if statusIsCompressed && lostPreviousUpdate {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be OR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, "AND" is correct here. We only ned to ask for the full state if both of these conditions are true:

  • The full state was not reported in the message we have just received. If this is not true, it means we have already just received the full state, there is no need to ask for it again.
  • We detected that we lost a previous update. If this is not true, it means we already have the correct state on our end, there is no need to ask the client to send it again.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks!

// The status message is not fully set in the message that we received, but we lost the previous
// status update. Request full status update from the agent.
response.Flags |= protobufs.ServerToAgent_ReportFullState
}

configChanged := false
if agentDescrChanged {
// Agent description is changed.
Expand All @@ -219,8 +237,8 @@ func (agent *Agent) processStatusUpdate(
// If remote config is changed and different from what the Agent has then
// send the new remote config to the Agent.
if configChanged ||
(newStatus.RemoteConfigStatus != nil &&
bytes.Compare(newStatus.RemoteConfigStatus.LastRemoteConfigHash, agent.remoteConfig.ConfigHash) != 0) {
(agent.Status.RemoteConfigStatus != nil &&
bytes.Compare(agent.Status.RemoteConfigStatus.LastRemoteConfigHash, agent.remoteConfig.ConfigHash) != 0) {
// The new status resulted in a change in the config of the Agent or the Agent
// does not have this config (hash is different). Send the new config the Agent.
response.RemoteConfig = agent.remoteConfig
Expand Down