Disable full push on scale from 1->0->1 #40866

howardjohn · 2022-09-08T00:00:21Z

This reverts #36882. At the time,
that PR was needed because EDS ServiceAccounts and CDS ServiceAccounts
were decoupled; Since #39133, this is
no longer true, and the fix in 36882 is not needed any longer.

This PR removes the test added in 36882 (since it tests low level
details that are not relevant anymore). It improves the existing
TestEndpointFlipFlops test -- while that test would have caught the
regression, it didn't actually set any service accounts so it was
missed. The update changes it to correctly detect the behavior (it now
fails without this PR, passes with it).

istio-testing · 2022-09-08T00:00:23Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

howardjohn · 2022-09-08T00:00:26Z

/test all

Fixes istio#39652 This reverts istio#36882. At the time, that PR was needed because EDS ServiceAccounts and CDS ServiceAccounts were decoupled; Since istio#39133, this is no longer true, and the fix in 36882 is not needed any longer. This PR *removes* the test added in 36882 (since it tests low level details that are not relevant anymore). It improves the existing TestEndpointFlipFlops test -- while that test *would* have caught the regression, it didn't actually set any service accounts so it was missed. The update changes it to correctly detect the behavior (it now fails without this PR, passes with it).

hzxuzhonghu · 2022-09-09T01:41:30Z

cc

hzxuzhonghu

Could not see how you diable full push.

howardjohn · 2022-09-09T02:34:15Z

before on delete we clean up the SAs, so it ends up empty. Then when the pod comes back up, new SA is added, which is a change so it triggers a full push. Now we avoid changing the SA when it's removed so it doesn't change when the new one comes back up

…

On Thu, Sep 8, 2022, 7:30 PM Zhonghu Xu ***@***.***> wrote: ***@***.**** commented on this pull request. Could not see how you diable full push. — Reply to this email directly, view it on GitHub <#40866 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXITIZ4SXCHBG7YTG4TV5KOOFANCNFSM6AAAAAAQHG56VU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

howardjohn · 2022-09-09T02:34:43Z

it is fairly subtle... but we do have a test for it at least (and now it works... before it didn't unfortunately)

…

On Thu, Sep 8, 2022, 7:34 PM John Howard ***@***.***> wrote: before on delete we clean up the SAs, so it ends up empty. Then when the pod comes back up, new SA is added, which is a change so it triggers a full push. Now we avoid changing the SA when it's removed so it doesn't change when the new one comes back up On Thu, Sep 8, 2022, 7:30 PM Zhonghu Xu ***@***.***> wrote: > ***@***.**** commented on this pull request. > > Could not see how you diable full push. > > — > Reply to this email directly, view it on GitHub > <#40866 (review)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAEYGXITIZ4SXCHBG7YTG4TV5KOOFANCNFSM6AAAAAAQHG56VU> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

hzxuzhonghu · 2022-09-09T02:37:58Z

In case all pods of a service is down, and at this time another event triggers a full push, we will result in having incorrect cluster.

howardjohn · 2022-09-09T02:41:57Z

Why? the cluster is unchanged during 1 pod, 0 pod, and then back to 1 pod - it always has the last service account when there is 0 pods to avoid a full push the remove the SA when it doesn't matter (if there are no endpoints SA is never used anyways)

…

On Thu, Sep 8, 2022, 7:38 PM Zhonghu Xu ***@***.***> wrote: In case all pods of a service is down, and at this time another event triggers a full push, we will result in having incorrect cluster. — Reply to this email directly, view it on GitHub <#40866 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXLNCQQN6CD6FVVYOEDV5KPJFANCNFSM6AAAAAAQHG56VU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

hzxuzhonghu · 2022-09-09T02:43:39Z

But the cluster upstream tls setting can have the sas of deleted pods, we could know whether they will come back

howardjohn · 2022-09-09T02:45:23Z

I believe that when we scale back yo we will 'reset' the service accounts. So typically we come back up with the same account and no update needed. If it's a different account, however, it should clear the old one out and trigger a full push. Let me add a test for that case

…

On Thu, Sep 8, 2022, 7:43 PM Zhonghu Xu ***@***.***> wrote: But the cluster upstream tls setting can have the sas of deleted pods, we could know whether they will come back — Reply to this email directly, view it on GitHub <#40866 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXMOTEBFCGBS2Z3LR5TV5KP6NANCNFSM6AAAAAAQHG56VU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

istio-testing · 2022-09-15T14:10:16Z

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

Applying: Disable full push on scale from 1->0->1
Using index info to reconstruct a base tree...
A	pilot/pkg/model/endpointshards.go
M	pilot/pkg/xds/eds_test.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/xds/eds_test.go
CONFLICT (content): Merge conflict in pilot/pkg/xds/eds_test.go
CONFLICT (modify/delete): pilot/pkg/model/endpointshards.go deleted in HEAD and modified in Disable full push on scale from 1->0->1. Version Disable full push on scale from 1->0->1 of pilot/pkg/model/endpointshards.go left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Disable full push on scale from 1->0->1
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-09-15T14:10:18Z

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

istio-testing · 2022-09-15T14:10:18Z

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

Applying: Disable full push on scale from 1->0->1
Using index info to reconstruct a base tree...
M	pilot/pkg/model/endpointshards.go
M	pilot/pkg/xds/eds_test.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/xds/eds_test.go
CONFLICT (content): Merge conflict in pilot/pkg/xds/eds_test.go
Auto-merging pilot/pkg/model/endpointshards.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 Disable full push on scale from 1->0->1
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-09-15T14:10:20Z

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

istio-testing · 2022-09-15T14:10:55Z

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

Applying: Disable full push on scale from 1->0->1
Using index info to reconstruct a base tree...
A	pilot/pkg/model/endpointshards.go
M	pilot/pkg/xds/eds_test.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/xds/eds_test.go
CONFLICT (content): Merge conflict in pilot/pkg/xds/eds_test.go
CONFLICT (modify/delete): pilot/pkg/model/endpointshards.go deleted in HEAD and modified in Disable full push on scale from 1->0->1. Version Disable full push on scale from 1->0->1 of pilot/pkg/model/endpointshards.go left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Disable full push on scale from 1->0->1
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-09-15T14:10:57Z

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

istio-testing · 2022-09-15T14:10:58Z

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

Applying: Disable full push on scale from 1->0->1
Using index info to reconstruct a base tree...
M	pilot/pkg/model/endpointshards.go
M	pilot/pkg/xds/eds_test.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/xds/eds_test.go
CONFLICT (content): Merge conflict in pilot/pkg/xds/eds_test.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 Disable full push on scale from 1->0->1
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-09-15T14:10:59Z

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

istio-testing · 2022-09-15T14:11:34Z

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

Applying: Disable full push on scale from 1->0->1
Using index info to reconstruct a base tree...
M	pilot/pkg/model/endpointshards.go
M	pilot/pkg/xds/eds_test.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/xds/eds_test.go
CONFLICT (content): Merge conflict in pilot/pkg/xds/eds_test.go
Auto-merging pilot/pkg/model/endpointshards.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 Disable full push on scale from 1->0->1
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-09-15T14:11:36Z

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

istio-testing · 2022-09-15T14:11:38Z

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

Applying: Disable full push on scale from 1->0->1
Using index info to reconstruct a base tree...
A	pilot/pkg/model/endpointshards.go
M	pilot/pkg/xds/eds_test.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/xds/eds_test.go
CONFLICT (content): Merge conflict in pilot/pkg/xds/eds_test.go
CONFLICT (modify/delete): pilot/pkg/model/endpointshards.go deleted in HEAD and modified in Disable full push on scale from 1->0->1. Version Disable full push on scale from 1->0->1 of pilot/pkg/model/endpointshards.go left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Disable full push on scale from 1->0->1
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-09-15T14:11:39Z

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

istio-testing · 2022-09-15T14:12:12Z

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

Applying: Disable full push on scale from 1->0->1
Using index info to reconstruct a base tree...
M	pilot/pkg/model/endpointshards.go
M	pilot/pkg/xds/eds_test.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/xds/eds_test.go
CONFLICT (content): Merge conflict in pilot/pkg/xds/eds_test.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 Disable full push on scale from 1->0->1
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-09-15T14:12:14Z

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

howardjohn · 2022-09-15T17:53:53Z

This should not be cherrypicked.

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

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 8, 2022

howardjohn force-pushed the san/stop-full-push branch from 89b3390 to 82095ed Compare September 8, 2022 17:36

istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 8, 2022

howardjohn changed the title ~~experiment dropping scale up full pushes~~ Disable full push on scale from 1->0->1 Sep 8, 2022

add note

45de4bf

howardjohn marked this pull request as ready for review September 8, 2022 17:38

howardjohn requested review from a team as code owners September 8, 2022 17:38

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

fix note

e129b1f

johnma14 approved these changes Sep 8, 2022

View reviewed changes

howardjohn assigned stevenctl Sep 8, 2022

hzxuzhonghu reviewed Sep 9, 2022

View reviewed changes

Add new SA case

c6723a7

istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 9, 2022

johnma14 added cherrypick/release-1.13 labels Sep 15, 2022

istio-testing mentioned this pull request Sep 15, 2022

[release-1.13] Disable full push on scale from 1->0->1 #41001

Closed

istio-testing mentioned this pull request Sep 15, 2022

[release-1.14] Disable full push on scale from 1->0->1 #41002

Closed

istio-testing mentioned this pull request Sep 15, 2022

[release-1.13] Disable full push on scale from 1->0->1 #41003

Closed

istio-testing mentioned this pull request Sep 15, 2022

[release-1.15] Disable full push on scale from 1->0->1 #41004

Closed

istio-testing mentioned this pull request Sep 15, 2022

[release-1.14] Disable full push on scale from 1->0->1 #41005

Closed

istio-testing mentioned this pull request Sep 15, 2022

[release-1.13] Disable full push on scale from 1->0->1 #41006

Closed

istio-testing mentioned this pull request Sep 15, 2022

[release-1.15] Disable full push on scale from 1->0->1 #41007

Closed

johnma14 removed cherrypick/release-1.13 labels Sep 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable full push on scale from 1->0->1 #40866

Disable full push on scale from 1->0->1 #40866

howardjohn commented Sep 8, 2022 •

edited

Loading

istio-testing commented Sep 8, 2022

howardjohn commented Sep 8, 2022

hzxuzhonghu commented Sep 9, 2022

hzxuzhonghu left a comment

howardjohn commented Sep 9, 2022 via email

howardjohn commented Sep 9, 2022 via email

hzxuzhonghu commented Sep 9, 2022

howardjohn commented Sep 9, 2022 via email

hzxuzhonghu commented Sep 9, 2022

howardjohn commented Sep 9, 2022 via email

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

howardjohn commented Sep 15, 2022

Disable full push on scale from 1->0->1 #40866

Disable full push on scale from 1->0->1 #40866

Conversation

howardjohn commented Sep 8, 2022 • edited Loading

istio-testing commented Sep 8, 2022

howardjohn commented Sep 8, 2022

hzxuzhonghu commented Sep 9, 2022

hzxuzhonghu left a comment

Choose a reason for hiding this comment

howardjohn commented Sep 9, 2022 via email

howardjohn commented Sep 9, 2022 via email

hzxuzhonghu commented Sep 9, 2022

howardjohn commented Sep 9, 2022 via email

hzxuzhonghu commented Sep 9, 2022

howardjohn commented Sep 9, 2022 via email

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

istio-testing commented Sep 15, 2022

howardjohn commented Sep 15, 2022

howardjohn commented Sep 8, 2022 •

edited

Loading