search

OctopusDeploy / Issues

| Public | Bug reports and known issues for Octopus Deploy and all related tools
https://octopus.com
162 stars 20 forks source link

Machine Event AutoDeploy considers too many targets when working out missed targets due to running deployment #5443

Closed droyad closed 4 years ago

droyad commented 5 years ago

When a machine comes online but the release that should be deployed to it is currently being deployed (or queued) to the same environment, that machine is skipped (so that machines are batched together).

The code that works out which machines are missed casts a too wide of a net. It usually picks up all machines that were changed since the first deployment of that release to the environment, not just the deployment(s) that ran during the missed period.

The error lies in the EventSourceDeployments where is joins the finished event back to the start events based on RelatedDocumentId. As shown in the below screenshot the RelatedDocumentIds are not guaranteed to be in the same order. This causes the join to only find the original DeploymentQueued event (Red) instead of the one related to the DeploymentFailed/Succeded event (Blue)

image

Additionally, the code at https://github.com/OctopusDeploy/OctopusDeploy/blob/b6960a3ad9114b273a61787c55619acade27ffa3/source/Octopus.Server/Schedules/AutoDeployments/AutoDeployMachineFinder.cs#L83 loops over all the missed deployments gathering machines, creating duplicate entries and many slow queries. This could most likely be improved my getting the min deployment.StartAutoId and max deployment.EndAutoId and doing the GetDeploymentTargetsThatHaveTriggeredBetweenAutoIds call once.

The same approach could be used to incorporate this call https://github.com/OctopusDeploy/OctopusDeploy/blob/002df1007071e5eec44166046592f6ba5f85ba90/source/Octopus.Server/Schedules/AutoDeployments/AutoDeployTriggerProcessor.cs#L48 so that we only query for machine events table once.

The duplication issue has been worked around by this change https://github.com/OctopusDeploy/OctopusDeploy/pull/3637/files#diff-b150f59995fb77baf21986e4ed9e557fR57

droyad commented 5 years ago

Possibly related to #5372

droyad commented 5 years ago

Machine Event AutoDeploy considers too many targets when working out missed targets due to running deployment #5443

droyad commented 5 years ago

Work around to this is to create a new release and deploy that once.

droyad commented 5 years ago

We should fix this on LTS

droyad commented 5 years ago

Duplicate of #4563?

octoreleasebot commented 5 years ago

Release Note: Fix for auto-deploy machines becoming available during a deployment

MarkSiedle commented 5 years ago

Just reading into this further, while it's related to #5372, I think there is a different issue going on here with tenant ordering, and performance-related code changes, which may not be fixed with the previously linked PR. Re-opening for the performance team to review.

tothegills commented 4 years ago

The changes to resolve https://github.com/OctopusDeploy/Issues/issues/5911 also resolve this issue.

lock[bot] commented 4 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. If you think you've found a related issue, please contact our support team so we can triage your issue, and make sure it's handled appropriately.