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.

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

Delete HTTPRoute will trigger two push event loops, is this expected? #50998

Open
spacewander opened this issue May 11, 2024 · 6 comments
Open

Comments

@spacewander
Copy link
Contributor

When deleting an HTTPRoute, two push event loops are triggered:

The first time, the delete event of HTTPRoute:

ads	Push debounce stable[16] 1 for config HTTPRoute/default/http: 100.446333ms since last change

The second time, caused by the change of Gateway's status:

ads	Push debounce stable[17] 1 for config Gateway/default/gateway: 100.1805ms since last change,

I discovered this in Istio 1.21, but since the needsPush in https://github.com/istio/istio/blob/master/pilot/pkg/bootstrap/config_compare.go is not changed in Istio 1.22, the same thing probably occurs. There is special rule to filter out Istio resource change, but not for the Gateway API resource.

Delete a VirtualService won't trigger two push event loops. This behavior makes the deletion of HTTPRoute less efficient because the extra push will require extra recomputing. Is this behavior expected?

@howardjohn
Copy link
Member

it's because it triggers a gateway status change. Probably could be optimized to not push on status change (or, at least, that field in status)

@spacewander
Copy link
Contributor Author

What about adding special handling to the Gateway API resource in https://github.com/istio/istio/blob/master/pilot/pkg/bootstrap/config_compare.go, just like what we have done with the istio's resource?

I would like to submit this PR later if permitted.

@howardjohn
Copy link
Member

The thing that is tricky is we use pushes to trigger recomputing GW API resources. So while we don't use status at all for XDS generation, if we were to do something like if only status changed { skip } the status would fall out of date.

@spacewander
Copy link
Contributor Author

As long as the status is produced by us, we don't need to watch the status change because the status is the reason for our processing.

There may be two cases that we need to watch the status change:

  1. we need to confirm the status is written into API server, and do something after it. Since the status writing is async, for now, I think there is no place that requires this confirmation.
  2. some tools other than the istiod will produce status that needs to be consumed by istiod. Although I may miss something in the Gateway API, but it seems that there is no such requirement so far.

@howardjohn
Copy link
Member

Yeah but the part that writes the status and the part that reads the events doesn't really know who wrote it which makes it challenging.

In Gateway API most objects (routes) can have multiple owners as well. Also, someone could just delete the status - we are expected to add it back

@spacewander
Copy link
Contributor Author

The computing of Gateway API resources in the event loop can be split into two parts:

  1. translate into internal representation
  2. generate xds

Since we want to add the status back, we can't skip part one for the status only change. But we can skip the second part.

For example, we can add a statusOnly field in the PushRequest. If the request only contains status only changes, we can skip the xds generation.

As Gateway API depends on the status widely, it's worth skipping the xds generation for status only changes, which may be a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants