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

Jump to bottom

xds: ensure that dependent xDS resources are reconfigured during primary type warming #10381

Merged
merged 5 commits into from
Jun 14, 2021
Merged

xds: ensure that dependent xDS resources are reconfigured during primary type warming #10381

merged 5 commits into from
Jun 14, 2021

Conversation

rboyer
Copy link
Member

@rboyer rboyer commented Jun 11, 2021

Updates to a cluster will clear the associated endpoints, and updates to
a listener will clear the associated routes. Update the incremental xDS
logic to account for this implicit cleanup so that we can finish warming
the clusters and listeners.

Fixes #10379

@rboyer
xds: ensure that dependent xDS resources are reconfigured during prim…
2215dea 
…ary type warming

Updates to a cluster will clear the associated endpoints, and updates to
a listener will clear the associated routes. Update the incremental xDS
logic to account for this implicit cleanup so that we can finish warming
the clusters and listeners.

Fixes #10379
@rboyer rboyer added type/bug Feature does not function as expected theme/envoy/xds Related to Envoy support backport/1.10 labels Jun 11, 2021
@rboyer rboyer added this to the 1.10.0 milestone Jun 11, 2021
@rboyer rboyer requested a review from a team June 11, 2021 21:54
@rboyer rboyer self-assigned this Jun 11, 2021
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging June 11, 2021 22:04 Inactive
@vercel vercel bot temporarily deployed to Preview – consul June 11, 2021 22:04 Inactive
freddygv
freddygv approved these changes Jun 11, 2021
View reviewed changes
Copy link
Contributor

@freddygv freddygv left a comment

Choose a reason for hiding this comment

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

LGTM, added some non-blocking requests around the testing

agent/xds/delta_test.go Outdated

requireProtocolVersionGauge(t, scenario, "v3", 1)

// Deliver a new snapshot (tcp with one tcp upstream)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Deliver a new snapshot (tcp with one tcp upstream)
// Deliver a new snapshot (tcp proxy with two tcp upstreams)

agent/xds/delta_test.go
@@ -327,6 +327,137 @@ func TestServer_DeltaAggregatedResources_v3_BasicProtocol_HTTP2(t *testing.T) {
}
}

func TestServer_DeltaAggregatedResources_v3_BasicProtocol_TCP_clusterChangesImpactEndpoints(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the TODOs at the top of this file still needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I did not come back and add a bunch of additional protocol tests.

agent/xds/delta_test.go Outdated Show resolved Hide resolved
banks
banks reviewed Jun 14, 2021
View reviewed changes
.changelog/10381.txt Outdated
@@ -0,0 +1,3 @@
```release-note:bug
xds: ensure that dependent xDS resources are reconfigured during primary type warming
Copy link
Member

Choose a reason for hiding this comment

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

IIUC we only introduced this bug in 1.10 betas right? Should we make that clear here - this isn't fxing a bug from 1.9.x or earlier right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe I could change the message to be something like xds: (beta-only) ... so that when we generate the flattened GA changelog we just omit the beta-only messages?

banks
banks approved these changes Jun 14, 2021
View reviewed changes
Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

LGTM! Great job getting this fix in, I like that we have a test that could reproduce 👏

@vercel vercel bot temporarily deployed to Preview – consul June 14, 2021 18:58 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging June 14, 2021 18:58 Inactive
@rboyer rboyer requested review from banks and freddygv June 14, 2021 19:33
freddygv
freddygv approved these changes Jun 14, 2021
View reviewed changes
Copy link
Contributor

@freddygv freddygv left a comment

Choose a reason for hiding this comment

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

I think this looks good, just had a couple, likely non-blocking, questions

agent/xds/delta_test.go Outdated Show resolved Hide resolved
agent/xds/listeners.go Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – consul June 14, 2021 22:19 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging June 14, 2021 22:19 Inactive
@rboyer rboyer merged commit 848ad85 into master Jun 14, 2021
@rboyer rboyer deleted the xds-cross-type-dependency-fix branch June 14, 2021 22:20
@hc-github-team-consul-core
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/386296.

@hc-github-team-consul-core
Copy link
Contributor

🍒✅ Cherry pick of commit 848ad85 onto release/1.10.x succeeded!

hc-github-team-consul-core pushed a commit that referenced this pull request Jun 14, 2021
@rboyer @hc-github-team-consul-core
xds: ensure that dependent xDS resources are reconfigured during prim…
f727746 
…ary type warming (#10381)

Updates to a cluster will clear the associated endpoints, and updates to
a listener will clear the associated routes. Update the incremental xDS
logic to account for this implicit cleanup so that we can finish warming
the clusters and listeners.

Fixes #10379
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@freddygv freddygv freddygv approved these changes

@banks banks Awaiting requested review from banks

Assignees

@rboyer rboyer

Labels
theme/envoy/xds Related to Envoy support type/bug Feature does not function as expected
Projects
None yet
Milestone
1.10.0
Development

Successfully merging this pull request may close these issues.

Connect: CDS update makes all service sidecars return only 503
4 participants
@rboyer @hc-github-team-consul-core @banks @freddygv