Fix dr dependency miss #38088

hzxuzhonghu · 2022-03-23T10:36:01Z

Please provide a description of this PR:

For merged DRs, currently we have only one DR meta(name/namespace) and add only one DR dependency.
And so we will miss xds push for some update.

The pr solves this by recording all DRs' meta, and adding all dependencies.

hzxuzhonghu · 2022-03-25T01:06:36Z

@howardjohn Ready now

hzxuzhonghu · 2022-03-26T01:17:12Z

Kindly ping @howardjohn in case you missed it

ramaraochavali

some comments

ramaraochavali · 2022-03-26T09:32:27Z

pilot/pkg/model/destination_rule.go

@@ -80,37 +83,56 @@ func (ps *PushContext) mergeDestinationRule(p *consolidatedDestRules, destRuleCo
 	}

 	// DestinationRule does not exist for the resolved host so add it
-	p.destRule[resolvedHost] = &destRuleConfig
+	p.destRule[resolvedHost] = convertConsolidateDestRule(&destRuleConfig)


nit: convertConsolidateDestRule -> convertConsolidatedDestRule

ramaraochavali · 2022-03-26T09:32:38Z

pilot/pkg/model/destination_rule.go

 	p.exportTo[resolvedHost] = exportToMap
 }

 // inheritDestinationRule child config inherits settings from parent mesh/namespace
-func (ps *PushContext) inheritDestinationRule(parent, child *config.Config) *config.Config {
+func (ps *PushContext) inheritDestinationRule(parent, child *consolidateDestRule) *consolidateDestRule {


consolidateDestRule -> consolidatedDestRule

ramaraochavali · 2022-03-26T09:33:18Z

pilot/pkg/model/destination_rule.go

 	if parent == nil {
 		return child
 	}
 	if child == nil {
 		return parent
 	}

-	parentDR := parent.Spec.(*networking.DestinationRule)
+	if reflect.DeepEqual(parent.sourceRules, child.sourceRules) {


Is it possible to avoid reflect.DeepEqual?

ramaraochavali · 2022-03-26T09:33:51Z

pilot/pkg/model/push_context.go

@@ -115,14 +116,14 @@ type destinationRuleIndex struct {
 	exportedByNamespace map[string]*consolidatedDestRules
 	rootNamespaceLocal  *consolidatedDestRules
 	// mesh/namespace dest rules to be inherited
-	inheritedByNamespace map[string]*config.Config
+	inheritedByNamespace map[string]*consolidateDestRule


nit: consolidatedDestRule

ramaraochavali · 2022-03-26T09:33:58Z

pilot/pkg/model/push_context.go

 }

 func newDestinationRuleIndex() destinationRuleIndex {
 	return destinationRuleIndex{
 		namespaceLocal:       map[string]*consolidatedDestRules{},
 		exportedByNamespace:  map[string]*consolidatedDestRules{},
-		inheritedByNamespace: map[string]*config.Config{},
+		inheritedByNamespace: map[string]*consolidateDestRule{},


ramaraochavali · 2022-03-26T09:34:06Z

pilot/pkg/model/push_context.go

@@ -252,7 +253,13 @@ type consolidatedDestRules struct {
 	// Map of dest rule host to the list of namespaces to which this destination rule has been exported to
 	exportTo map[host.Name]map[visibility.Instance]bool
 	// Map of dest rule host and the merged destination rules for that host
-	destRule map[host.Name]*config.Config
+	destRule map[host.Name]*consolidateDestRule


ramaraochavali · 2022-03-26T09:38:11Z

pilot/pkg/model/push_context.go

+}
+
+// consolidateDestRule represents a dr and from which it is consolidated.
+type consolidateDestRule struct {


consolidatedDestRules already exists. So it would be very confusing to have this struct. I think DestRuleConsolidationSource may be better. If you agree, change sourceRules -> fromRules?

consolidatedDestRules is a wrapper of rules from a single namespace. That one is plural.

How about calling from?

from makes sense

ramaraochavali · 2022-03-26T09:39:22Z

Also can you please add a description to the PR?

hzxuzhonghu · 2022-03-28T02:30:10Z

Make sense, it should include a release note

ramaraochavali · 2022-03-28T08:49:37Z

LGTM will leave it for @howardjohn to take a look

howardjohn · 2022-03-28T15:32:16Z

pilot/pkg/model/destination_rule.go

 	if parent == nil {
 		return child
 	}
 	if child == nil {
 		return parent
 	}

-	parentDR := parent.Spec.(*networking.DestinationRule)
+	if reflect.DeepEqual(parent.from, child.from) {


can we use a more efficient check for equality?

Especially concerned that this is an ordered list - does order really matter here?

#38088 (comment) - that was my concern as well

The order is determined, we have sorted all the drs before merge

I can compare them one by one to improve efficiency

But not sure how much it improves, since i can see many places in k8s use reflect.DeepEqual to compare labels.
I have never tested the two ways. Either make sense to me.

hzxuzhonghu · 2022-03-31T02:25:14Z

@howardjohn Can you ptal

hzxuzhonghu · 2022-04-01T01:03:00Z

/retest

howardjohn · 2022-04-01T01:08:40Z

does this impact CDS cache as well?

…

On Thu, Mar 31, 2022, 6:03 PM Zhonghu Xu ***@***.***> wrote: /retest — Reply to this email directly, view it on GitHub <#38088 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXP5JTIJKWML4VAP42DVCZDM7ANCNFSM5RNSVI6Q> . You are receiving this because you were mentioned.Message ID: ***@***.***>

hzxuzhonghu · 2022-04-01T01:15:30Z

It influences a little, previously the cds cache can not be right in some merging cases

istio-testing · 2022-07-01T02:44:54Z

In response to a cherrypick label: #38088 failed to apply on top of branch "release-1.13":

Applying: Fix missing dr dependency when merging
Using index info to reconstruct a base tree...
M	pilot/pkg/model/config.go
M	pilot/pkg/model/destination_rule.go
M	pilot/pkg/model/push_context.go
M	pilot/pkg/model/push_context_test.go
M	pilot/pkg/model/sidecar.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/model/sidecar.go
CONFLICT (content): Merge conflict in pilot/pkg/model/sidecar.go
Auto-merging pilot/pkg/model/push_context_test.go
CONFLICT (content): Merge conflict in pilot/pkg/model/push_context_test.go
Auto-merging pilot/pkg/model/push_context.go
CONFLICT (content): Merge conflict in pilot/pkg/model/push_context.go
Auto-merging pilot/pkg/model/destination_rule.go
CONFLICT (content): Merge conflict in pilot/pkg/model/destination_rule.go
Auto-merging pilot/pkg/model/config.go
CONFLICT (content): Merge conflict in pilot/pkg/model/config.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix missing dr dependency when merging
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

istio-testing · 2022-07-01T02:44:56Z

In response to a cherrypick label: new issue created for failed cherrypick: #39728

GregHanson · 2022-07-05T22:09:22Z

@hzxuzhonghu was this issue only present in 1.13? Or does this need to be back ported to 1.14 as well?

hzxuzhonghu · 2022-07-06T01:04:01Z

Yes, only exists in version <=1.13

hzxuzhonghu · 2022-07-06T01:05:17Z

Also this should combine with #39730

hzxuzhonghu requested a review from a team as a code owner March 23, 2022 10:36

istio-testing added do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR needs to be rebased before being merged labels Mar 23, 2022

hzxuzhonghu force-pushed the fix-dr-2 branch from 839fb71 to ac5e50f Compare March 24, 2022 06:02

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 24, 2022

hzxuzhonghu changed the title ~~WIP: Fix dr dependency miss~~ Fix dr dependency miss Mar 25, 2022

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Mar 25, 2022

hzxuzhonghu added the release-notes-none Indicates a PR that does not require release notes. label Mar 25, 2022

ramaraochavali reviewed Mar 26, 2022

View reviewed changes

hzxuzhonghu requested review from a team as code owners March 28, 2022 08:27

howardjohn reviewed Mar 28, 2022

View reviewed changes

ramaraochavali approved these changes Mar 29, 2022

View reviewed changes

Fix missing dr dependency when merging

1a6b305

hzxuzhonghu force-pushed the fix-dr-2 branch from 49fa1da to 1a6b305 Compare March 30, 2022 06:37

howardjohn approved these changes Mar 31, 2022

View reviewed changes

istio-testing merged commit 9fcfe23 into istio:master Apr 1, 2022

hzxuzhonghu deleted the fix-dr-2 branch April 1, 2022 01:15

aryan16 pushed a commit to aryan16/istio that referenced this pull request Apr 8, 2022

Fix missing dr dependency when merging (istio#38088)

560a5bf

aryan16 pushed a commit to aryan16/istio that referenced this pull request Apr 11, 2022

Fix missing dr dependency when merging (istio#38088)

5a6fedc

hzxuzhonghu mentioned this pull request Jul 1, 2022

Subset DestinationRule merge cause cds cache is not invalid #39726

Closed

hzxuzhonghu added the cherrypick/release-1.13 label Jul 1, 2022

istio-testing mentioned this pull request Jul 1, 2022

[release-1.13] Fix dr dependency miss #39728

Closed

hzxuzhonghu mentioned this pull request Jul 1, 2022

Add split DRs to rds/cds/eds dependent configs #39730

Merged

Fix dr dependency miss #38088

Fix dr dependency miss #38088

Uh oh!

Conversation

hzxuzhonghu commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hzxuzhonghu commented Mar 25, 2022

Uh oh!

hzxuzhonghu commented Mar 26, 2022

Uh oh!

ramaraochavali left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramaraochavali commented Mar 26, 2022

Uh oh!

hzxuzhonghu commented Mar 28, 2022

Uh oh!

ramaraochavali commented Mar 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu commented Mar 31, 2022

Uh oh!

hzxuzhonghu commented Apr 1, 2022

Uh oh!

howardjohn commented Apr 1, 2022 via email

Uh oh!

hzxuzhonghu commented Apr 1, 2022

Uh oh!

istio-testing commented Jul 1, 2022

Uh oh!

istio-testing commented Jul 1, 2022

Uh oh!

GregHanson commented Jul 5, 2022

Uh oh!

hzxuzhonghu commented Jul 6, 2022

Uh oh!

hzxuzhonghu commented Jul 6, 2022

Uh oh!

Uh oh!

hzxuzhonghu commented Mar 23, 2022 •

edited

Loading