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

Add trigger in callbacks to request resource updates #3

Closed
wants to merge 2 commits into from

Conversation

valerian-roche
Copy link
Owner

No description provided.

Copy link

@s-matyukevich s-matyukevich left a comment

Choose a reason for hiding this comment

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

This looks good to me, but if we want to use this in our fork I don't think we need sotw changes - it would make it harder to merge upstream changes later.
Also, looks like upstream won't accept something like this, but we can try.

@valerian-roche
Copy link
Owner Author

This looks good to me, but if we want to use this in our fork I don't think we need sotw changes - it would make it harder to merge upstream changes later.
Also, looks like upstream won't accept something like this, but we can try.

I can remove the sotw part. To upstream we can ask again Alec as in this case it doesn't break the public api, though it's far more of a workaround than a decent thing

@valerian-roche
Copy link
Owner Author

Some context on the new commit:
there was a bug in the control-plane when using delta-ads: when a client with an existing configuration would send its request for a non-wildcard case (e.g. eds), the initial versions were assigned to the watch state but then discarded when recording the subscription (https://github.com/envoyproxy/go-control-plane/blob/89a37c1326000d5b4ed8cc07494d07185ec37eba/pkg/server/delta/v3/server.go#L216). This was due to ResourceVersions on the state being used for both subscription and known resource tracking
As a consequence, the control-plane ended up always sending back all resources when not in wildcard case
In PR envoyproxy#559, in order to properly track resources in the context of the new definition of wildcard, I split the two considerations (what the client is watching from what the client knows). This actually fixes this issue (known resources are now properly considered and not sent again)

Sadly when a node does connect to a new control-plane, it first sends a CDS req then a CLA/eds one.
At the time of the cluster change in CDS, the force-push trigger gets dropped by

// There's no existing watch
, and when the eds request comes, this force-push has been lost and the versions of eps are matching.
Now that we don't send the resources for no reason each time, our workaround is no longer working

This commit is therefore changing a bit the definition of the watch (I believe for the better, though the naming is now kind of funky). The stream state for a given type can now be created without a watch, and the part "what the client should know" can be set prior to a watch being opened.
If no request ever comes for the type, this will simply be discarded when the stream is destroyed.
If a request does come, this state is updated and the previous "force-push" is kept. This still means that we will not force-push a resource which is not watched (as was before)

@valerian-roche
Copy link
Owner Author

Now opened on the datadog fork

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

Successfully merging this pull request may close these issues.

2 participants