Fix hanging updates for deployments #1596

viveklak · 2021-05-28T08:38:32Z

Fix for #1502

In my investigation for the above issue, I was able to determine that there is a TOCTTOU style race in the state maintained by the await logic for deployments. Specifically, the initial state is seeded during Read() where information on replicaset generations, pods and PVCs are all populated. The watch is kicked off subsequently but based on the most recent resource version. The current await logic seems quite brittle against the potential for missed events. Indeed in my repro, I was able to determine that the await logic would hang forever waiting to see updates for an older generation of the replicaset (as referenced during the initial Read()) while the watch began with a state where the expected (old) replicaset was already deleted. This change seeds the watches to begin at the resource versions we initially read.

In some ways I am not thrilled with this approach since we are further perpetuating the current highly stateful nature of the await logic here. As part of the refactor/rearchitecture of the await logic for complex resources such as deployments we should definitely consider an approach where the strong consistency of the event stream is not necessary. The current biggest caveats with this approach:

With setting the resource revision for the watch, there is the possibility that the resource version to read from is no longer retained (Await() is significantly delayed from Read()) in which case we would get a 410 from the api server. I am not sure what we should do in that situation to recover.
The set resource version might cause an additional performance penalty.
Not entirely sure how best to test this. My repro environment was a bit convoluted though I will share the preliminary performance/load testing scaffold I used to track the issue (and verify fix) separately.

viveklak · 2021-05-28T08:40:41Z

provider/pkg/await/deployment.go

+	if dia.deployment != nil {
+		depListOptions.ResourceVersion = dia.deployment.GetResourceVersion()
+	}
+	deploymentWatcher, err := deploymentClient.Watch(context.TODO(), depListOptions)


TODO: handle potential 410 - gone error response here (and other places): https://kubernetes.io/docs/reference/using-api/api-concepts/#410-gone-responses

It seems like the fallback option could be grabbing the latest like it was doing before. Condition 1 is the only one that is comparing generations, and the generation just has to be later. If the specified resource version no longer exists, that implies that condition 1 is true.

I have updated this to restart at the current latest and reset our references to the latest deployment state.

github-actions · 2021-05-28T08:43:00Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

mikhailshilkov

Makes sense to me directionally, and I agree with your concerns. A couple of questions to improve my understanding.

provider/pkg/await/deployment.go

github-actions · 2021-05-28T17:57:37Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

lblackstone

I think these changes make sense, but would want to verify that this fixes the issue before merging.

lblackstone · 2021-05-28T21:08:18Z

provider/pkg/await/deployment.go

+	if dia.deployment != nil {
+		depListOptions.ResourceVersion = dia.deployment.GetResourceVersion()
+	}
+	deploymentWatcher, err := deploymentClient.Watch(context.TODO(), depListOptions)


It seems like the fallback option could be grabbing the latest like it was doing before. Condition 1 is the only one that is comparing generations, and the generation just has to be later. If the specified resource version no longer exists, that implies that condition 1 is true.

github-actions · 2021-06-10T08:27:24Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

github-actions · 2021-06-10T08:27:50Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

github-actions · 2021-06-11T00:21:37Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

github-actions · 2021-06-11T00:24:21Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

github-actions · 2021-06-11T06:32:30Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

viveklak · 2021-06-11T16:56:47Z

@lblackstone this is ready for another review. I am looking into the test failure. I can't seem to reproduce on my own test cluster on EKS while it repros consistently in CI. I expect this to be unrelated to the core of the changes here but I will get to the bottom of it today.

lblackstone · 2021-06-11T17:48:34Z

provider/pkg/await/deployment.go

+			return
+		}
+
+		currentGeneration := dia.deployment.GetAnnotations()[revision]


Is this annotation guaranteed to be present? If it's not set, the return value will be "". (I noticed that this was already present in other logic in this awaiter, but it might be good to be defensive.)

github-actions · 2021-06-15T05:53:43Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

lblackstone · 2021-06-15T18:17:46Z

@viveklak Is this ready to merge?

viveklak · 2021-06-15T18:37:36Z

Yup just rebased. Merging.

Should fix #1502 and #1512

* Revert #1596 * Add changelog entry for 3.4.1

viveklak requested a review from lblackstone May 28, 2021 08:38

viveklak added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label May 28, 2021

viveklak commented May 28, 2021

View reviewed changes

viveklak requested a review from mikhailshilkov May 28, 2021 08:41

viveklak marked this pull request as draft May 28, 2021 08:42

mikhailshilkov reviewed May 28, 2021

View reviewed changes

provider/pkg/await/deployment.go Show resolved Hide resolved

provider/pkg/await/deployment.go Outdated Show resolved Hide resolved

viveklak mentioned this pull request May 28, 2021

Pulumi gets stuck waiting on a deployment to be updated while it has updated #1502

Closed

lblackstone reviewed May 28, 2021

View reviewed changes

viveklak force-pushed the vl/FixDepStuck branch 2 times, most recently from 46b4932 to 2fa873d Compare June 10, 2021 08:22

viveklak marked this pull request as ready for review June 11, 2021 00:18

viveklak added area/await-logic and removed impact/no-changelog-required This issue doesn't require a CHANGELOG update labels Jun 11, 2021

lblackstone approved these changes Jun 11, 2021

View reviewed changes

Vivek Lakshmanan added 7 commits June 14, 2021 22:48

Fix hanging updates for deployments

cb302dc

Feedback

44204d5

Handle 410 error from watch

f19b4cf

Fix linting

f8ac275

Add changelog

6bdaa90

Fix istio test

8a46d1d

Handle empty generation annotation

4baef10

viveklak force-pushed the vl/FixDepStuck branch from 5436cca to 4baef10 Compare June 15, 2021 05:49

viveklak merged commit 1f95956 into master Jun 15, 2021

pulumi-bot deleted the vl/FixDepStuck branch June 15, 2021 18:37

This was referenced Jun 15, 2021

"Attempted to roll forward to new ReplicaSet, but minimum number of Pods did not become live" #1512

Closed

Istio test flake waiting with Deployment failing to be ready #1622

Closed

renovate bot mentioned this pull request Jun 17, 2021

fix(deps): update dependency @pulumi/pulumi to v3.126.0 sourcegraph/deploy-k8s-helper#80

Open

1 task

viveklak pushed a commit that referenced this pull request Jun 25, 2021

Revert #1596

790e081

viveklak mentioned this pull request Jun 25, 2021

Prep hotfix v3.4.1 #1637

Merged

viveklak pushed a commit that referenced this pull request Jun 25, 2021

Revert #1596 (#1636)

9e7f108

* Revert #1596 * Add changelog entry for 3.4.1

viveklak mentioned this pull request Jun 25, 2021

pulumi up stuck on "Waiting for app ReplicaSet be marked available" #1628

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix hanging updates for deployments #1596

Fix hanging updates for deployments #1596

viveklak commented May 28, 2021 •

edited

Loading

viveklak May 28, 2021 •

edited

Loading

lblackstone May 28, 2021

viveklak Jun 11, 2021

github-actions bot commented May 28, 2021

mikhailshilkov left a comment

github-actions bot commented May 28, 2021

lblackstone left a comment

lblackstone May 28, 2021

github-actions bot commented Jun 10, 2021

github-actions bot commented Jun 10, 2021

github-actions bot commented Jun 11, 2021

github-actions bot commented Jun 11, 2021

github-actions bot commented Jun 11, 2021

viveklak commented Jun 11, 2021

lblackstone Jun 11, 2021

github-actions bot commented Jun 15, 2021

lblackstone commented Jun 15, 2021

viveklak commented Jun 15, 2021

Fix hanging updates for deployments #1596

Fix hanging updates for deployments #1596

Conversation

viveklak commented May 28, 2021 • edited Loading

viveklak May 28, 2021 • edited Loading

Choose a reason for hiding this comment

lblackstone May 28, 2021

Choose a reason for hiding this comment

viveklak Jun 11, 2021

Choose a reason for hiding this comment

github-actions bot commented May 28, 2021

Does the PR have any schema changes?

mikhailshilkov left a comment

Choose a reason for hiding this comment

github-actions bot commented May 28, 2021

Does the PR have any schema changes?

lblackstone left a comment

Choose a reason for hiding this comment

lblackstone May 28, 2021

Choose a reason for hiding this comment

github-actions bot commented Jun 10, 2021

Does the PR have any schema changes?

github-actions bot commented Jun 10, 2021

Does the PR have any schema changes?

github-actions bot commented Jun 11, 2021

Does the PR have any schema changes?

github-actions bot commented Jun 11, 2021

Does the PR have any schema changes?

github-actions bot commented Jun 11, 2021

Does the PR have any schema changes?

viveklak commented Jun 11, 2021

lblackstone Jun 11, 2021

Choose a reason for hiding this comment

github-actions bot commented Jun 15, 2021

Does the PR have any schema changes?

lblackstone commented Jun 15, 2021

viveklak commented Jun 15, 2021

viveklak commented May 28, 2021 •

edited

Loading

viveklak May 28, 2021 •

edited

Loading