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

DeepCopy resources that require status updates #1723

Merged
merged 2 commits into from
Aug 1, 2023

Conversation

arkodg
Copy link
Contributor

@arkodg arkodg commented Jul 28, 2023

  • Was seeing constant churn between provider runner publishing resources and gateway-api runner receiving them.

  • Tried to debug it by printing the o/p of cmp.Diff between current and previous values

diff --git a/internal/gatewayapi/runner/runner.go b/internal/gatewayapi/runner/runner.go
index 050394ba..50d09f6f 100644
--- a/internal/gatewayapi/runner/runner.go
+++ b/internal/gatewayapi/runner/runner.go
@@ -8,6 +8,7 @@ package runner
 import (
        "context"

+       "github.com/google/go-cmp/cmp"
        "k8s.io/apimachinery/pkg/runtime/schema"
        "sigs.k8s.io/gateway-api/apis/v1beta1"
        "sigs.k8s.io/yaml"
@@ -49,6 +50,7 @@ func (r *Runner) Start(ctx context.Context) error {
 }

 func (r *Runner) subscribeAndTranslate(ctx context.Context) {
+       prev := &gatewayapi.Resources{}
        message.HandleSubscription(r.ProviderResources.GatewayAPIResources.Subscribe(ctx),
                func(update message.Update[string, *gatewayapi.Resources]) {
                        val := update.Value
@@ -56,6 +58,9 @@ func (r *Runner) subscribeAndTranslate(ctx context.Context) {
                        if update.Delete || val == nil {
                                return
                        }
+                       diff := cmp.Diff(prev, val)
+                       r.Logger.WithValues("output", "diff").Info(diff)
+                       prev = val.DeepCopy()

                        // Translate and publish IRs.
                        t := &gatewayapi.Translator{

Here's the o/p and its empty

2023-07-27T23:55:29.795Z	INFO	gateway-api	runner/runner.go:62		{"runner": "gateway-api", "output": "diff"}
  • Using a DeepCopy for resources that were updating the Status subresource seems to have solved the issue, which implies that watchable doesnt like clients to mutate the value, even though they are meant to be a DeepCopy

Fixes: #1715

* Was seeing constant churn between provider runner publishing resources
and gateway-api runner receiving them.

* Tried to debug it by printing the o/p of `cmp.Diff` between current
  and previous values
```
diff --git a/internal/gatewayapi/runner/runner.go b/internal/gatewayapi/runner/runner.go
index 050394ba..50d09f6f 100644
--- a/internal/gatewayapi/runner/runner.go
+++ b/internal/gatewayapi/runner/runner.go
@@ -8,6 +8,7 @@ package runner
 import (
        "context"

+       "github.com/google/go-cmp/cmp"
        "k8s.io/apimachinery/pkg/runtime/schema"
        "sigs.k8s.io/gateway-api/apis/v1beta1"
        "sigs.k8s.io/yaml"
@@ -49,6 +50,7 @@ func (r *Runner) Start(ctx context.Context) error {
 }

 func (r *Runner) subscribeAndTranslate(ctx context.Context) {
+       prev := &gatewayapi.Resources{}
        message.HandleSubscription(r.ProviderResources.GatewayAPIResources.Subscribe(ctx),
                func(update message.Update[string, *gatewayapi.Resources]) {
                        val := update.Value
@@ -56,6 +58,9 @@ func (r *Runner) subscribeAndTranslate(ctx context.Context) {
                        if update.Delete || val == nil {
                                return
                        }
+                       diff := cmp.Diff(prev, val)
+                       r.Logger.WithValues("output", "diff").Info(diff)
+                       prev = val.DeepCopy()

                        // Translate and publish IRs.
                        t := &gatewayapi.Translator{
```

Here's the o/p and its empty
```
2023-07-27T23:55:29.795Z	INFO	gateway-api	runner/runner.go:62		{"runner": "gateway-api", "output": "diff"}
```

* Using a DeepCopy for resources that were updating the `Status`
  subresource seems to have solved the issue, which implies that
  watchable doesnt like clients to mutate the value, even though they
  are meant to be a `DeepCopy`

Fixes: envoyproxy#1715

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg requested a review from a team as a code owner July 28, 2023 00:09
@arkodg
Copy link
Contributor Author

arkodg commented Jul 28, 2023

cc @LukeShu

@arkodg arkodg added this to the 0.5.0 milestone Jul 28, 2023
@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Merging #1723 (93cae96) into main (ea18f8f) will increase coverage by 0.07%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main    #1723      +/-   ##
==========================================
+ Coverage   64.77%   64.85%   +0.07%     
==========================================
  Files          83       83              
  Lines       11925    11928       +3     
==========================================
+ Hits         7725     7736      +11     
+ Misses       3715     3710       -5     
+ Partials      485      482       -3     
Files Changed Coverage Δ
internal/gatewayapi/runner/runner.go 25.49% <0.00%> (-0.51%) ⬇️
internal/xds/server/runner/runner.go 29.80% <0.00%> (-0.29%) ⬇️
internal/gatewayapi/envoypatchpolicy.go 69.51% <100.00%> (ø)
internal/gatewayapi/route.go 88.36% <100.00%> (ø)

... and 2 files with indirect coverage changes

@arkodg arkodg requested review from a team, zhaohuabing, qicz and chauhanshubham and removed request for a team August 1, 2023 23:00
@@ -57,6 +57,7 @@ func (r *Runner) subscribeToProxyInfraIR(ctx context.Context) {
// Subscribe to resources
message.HandleSubscription(r.InfraIR.Subscribe(ctx),
func(update message.Update[string, *ir.Infra]) {
r.Logger.Info("received an update")
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@@ -51,6 +51,8 @@ func (r *Runner) Start(ctx context.Context) error {
func (r *Runner) subscribeAndTranslate(ctx context.Context) {
message.HandleSubscription(r.ProviderResources.GatewayAPIResources.Subscribe(ctx),
func(update message.Update[string, *gatewayapi.Resources]) {
r.Logger.Info("received an update")
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be a debug log

@arkodg arkodg merged commit 5b72451 into envoyproxy:main Aug 1, 2023
18 checks passed
@arkodg arkodg deleted the reduce-watchable-churn branch August 1, 2023 23:09
arkodg added a commit to arkodg/gateway that referenced this pull request Aug 1, 2023
Relates to envoyproxy#1723 (comment)

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg added the cherrypick/release-v0.5 cherrypick to release/v0.5 label Aug 2, 2023
arkodg added a commit to arkodg/gateway that referenced this pull request Aug 2, 2023
* Was seeing constant churn between provider runner publishing resources
and gateway-api runner receiving them.

* Tried to debug it by printing the o/p of `cmp.Diff` between current
  and previous values
```
diff --git a/internal/gatewayapi/runner/runner.go b/internal/gatewayapi/runner/runner.go
index 050394ba..50d09f6f 100644
--- a/internal/gatewayapi/runner/runner.go
+++ b/internal/gatewayapi/runner/runner.go
@@ -8,6 +8,7 @@ package runner
 import (
        "context"

+       "github.com/google/go-cmp/cmp"
        "k8s.io/apimachinery/pkg/runtime/schema"
        "sigs.k8s.io/gateway-api/apis/v1beta1"
        "sigs.k8s.io/yaml"
@@ -49,6 +50,7 @@ func (r *Runner) Start(ctx context.Context) error {
 }

 func (r *Runner) subscribeAndTranslate(ctx context.Context) {
+       prev := &gatewayapi.Resources{}
        message.HandleSubscription(r.ProviderResources.GatewayAPIResources.Subscribe(ctx),
                func(update message.Update[string, *gatewayapi.Resources]) {
                        val := update.Value
@@ -56,6 +58,9 @@ func (r *Runner) subscribeAndTranslate(ctx context.Context) {
                        if update.Delete || val == nil {
                                return
                        }
+                       diff := cmp.Diff(prev, val)
+                       r.Logger.WithValues("output", "diff").Info(diff)
+                       prev = val.DeepCopy()

                        // Translate and publish IRs.
                        t := &gatewayapi.Translator{
```

Here's the o/p and its empty
```
2023-07-27T23:55:29.795Z	INFO	gateway-api	runner/runner.go:62		{"runner": "gateway-api", "output": "diff"}
```

* Using a DeepCopy for resources that were updating the `Status`
  subresource seems to have solved the issue, which implies that
  watchable doesnt like clients to mutate the value, even though they
  are meant to be a `DeepCopy`

Fixes: envoyproxy#1715

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
(cherry picked from commit 5b72451)
arkodg added a commit that referenced this pull request Aug 2, 2023
* refactor: set defaults in Deployment, else k8s sets them for you, creating infinite reconciliation loop (#1594)

* fix: envoy proxy resource apply bug.

Signed-off-by: qicz <qiczzhu@gmail.com>

* update pointer.

Signed-off-by: qicz <qiczzhu@gmail.com>

* add comment

Signed-off-by: qicz <qiczzhu@gmail.com>

* update cm cmp logic.

Signed-off-by: qicz <qiczzhu@gmail.com>

* fix lint

Signed-off-by: qicz <qiczzhu@gmail.com>

* add probe field default value.

Signed-off-by: qicz <qiczzhu@gmail.com>

* fix uts

Signed-off-by: qicz <qiczzhu@gmail.com>

* align probe

Signed-off-by: qicz <qiczzhu@gmail.com>

* optimize deploy compare logic

Signed-off-by: qicz <qiczzhu@gmail.com>

* add compare deploy uts

Signed-off-by: qicz <qiczzhu@gmail.com>

* rm cm binarydata cmp

Signed-off-by: qicz <qiczzhu@gmail.com>

* rm deploy cmp logic

Signed-off-by: qicz <qiczzhu@gmail.com>

* fix ut

Signed-off-by: qicz <qiczzhu@gmail.com>

* fix lint

Signed-off-by: qicz <qiczzhu@gmail.com>

---------

Signed-off-by: qicz <qiczzhu@gmail.com>
Signed-off-by: qi <qiczzhu@gmail.com>
(cherry picked from commit 9ba9103)

* DeepCopy resources that require status updates (#1723)

* Was seeing constant churn between provider runner publishing resources
and gateway-api runner receiving them.

* Tried to debug it by printing the o/p of `cmp.Diff` between current
  and previous values
```
diff --git a/internal/gatewayapi/runner/runner.go b/internal/gatewayapi/runner/runner.go
index 050394ba..50d09f6f 100644
--- a/internal/gatewayapi/runner/runner.go
+++ b/internal/gatewayapi/runner/runner.go
@@ -8,6 +8,7 @@ package runner
 import (
        "context"

+       "github.com/google/go-cmp/cmp"
        "k8s.io/apimachinery/pkg/runtime/schema"
        "sigs.k8s.io/gateway-api/apis/v1beta1"
        "sigs.k8s.io/yaml"
@@ -49,6 +50,7 @@ func (r *Runner) Start(ctx context.Context) error {
 }

 func (r *Runner) subscribeAndTranslate(ctx context.Context) {
+       prev := &gatewayapi.Resources{}
        message.HandleSubscription(r.ProviderResources.GatewayAPIResources.Subscribe(ctx),
                func(update message.Update[string, *gatewayapi.Resources]) {
                        val := update.Value
@@ -56,6 +58,9 @@ func (r *Runner) subscribeAndTranslate(ctx context.Context) {
                        if update.Delete || val == nil {
                                return
                        }
+                       diff := cmp.Diff(prev, val)
+                       r.Logger.WithValues("output", "diff").Info(diff)
+                       prev = val.DeepCopy()

                        // Translate and publish IRs.
                        t := &gatewayapi.Translator{
```

Here's the o/p and its empty
```
2023-07-27T23:55:29.795Z	INFO	gateway-api	runner/runner.go:62		{"runner": "gateway-api", "output": "diff"}
```

* Using a DeepCopy for resources that were updating the `Status`
  subresource seems to have solved the issue, which implies that
  watchable doesnt like clients to mutate the value, even though they
  are meant to be a `DeepCopy`

Fixes: #1715

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
(cherry picked from commit 5b72451)

* observability: add container port for metrics (#1736)

container port

Signed-off-by: zirain <zirain2009@gmail.com>
(cherry picked from commit 4bba03a)

* docs: Add user docs for EnvoyPatchPolicy (#1733)

* Add user docs for EnvoyPatchPolicy

Relates to #24

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* nits

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* wrap up

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* lint

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* address comments && fix config

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

---------

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
(cherry picked from commit 27b0939)

* e2e & misc fixes for EnvoyPatchPolicy (#1738)

* Add E2E for EnvoyPatchPolicy

* Use LocalReplyConfig to return a custom
status code `406` when there is no valid route match

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
(cherry picked from commit a7784c5)

---------

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Co-authored-by: qi <qiczzhu@gmail.com>
Co-authored-by: zirain <zirain2009@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick/release-v0.5 cherrypick to release/v0.5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Seeing continous provider and gateway-api logs in stdout
3 participants