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

Move to Patch API on infra-client #2995

Closed
ardikabs opened this issue Mar 21, 2024 · 5 comments · Fixed by #3034
Closed

Move to Patch API on infra-client #2995

ardikabs opened this issue Mar 21, 2024 · 5 comments · Fixed by #3034
Assignees
Labels
area/infra-mgr Issues related to the provisioner used for provisioning the managed Envoy Proxy fleet. kind/bug Something isn't working

Comments

@ardikabs
Copy link
Contributor

ardikabs commented Mar 21, 2024

Description:
Follow up from #2807, which is still happening in the latest version (v1.0.0). I noticed this behavior because the EG is utilizing the Update API instead of the Patch API in infra-client.

if updateChecker() {
specific.SetUID(current.GetUID())
if err := cli.Client.Update(ctx, specific); err != nil {
return fmt.Errorf("for Update: %w", err)
}
}

As a result, instead of only updating the changed field, it turned out to re-applies all fields causing the field to revert to its original value, instead of updating what changed. For example on the Deployment object, if the replicas field is set to null, it will be reverted to null.

So, I tried in my local to use Patch API, and it worked as expected.

[optional Relevant Links:]

Any extra documentation required to understand the issue.

@ardikabs
Copy link
Contributor Author

/cc @arkodg

@arkodg
Copy link
Contributor

arkodg commented Mar 21, 2024

Hey @ardikabs I understand the need for the patch API, but can you help explain your issue a little more, why is the update repeating multiple times

@ardikabs
Copy link
Contributor Author

ardikabs commented Mar 22, 2024

Hey @ardikabs I understand the need for the patch API, but can you help explain your issue a little more, why is the update repeating multiple times

@arkodg
It think i used wrong word about repeating, what i meant, when we enable HPA on EnvoyProxy, then leave the replicas to null. Now the expectation replicas field will be handled by HPA controller, however if we are doing any operation to either Gateway API or Envoy Gateway resource, the reconciliation on EG will trigger to reapply the old Deployment object (with replicas null). In short, it's because CMIIW the Update API is imperative, while in Patch API it could support declaratively on using Apply patch.

@ardikabs
Copy link
Contributor Author

ardikabs commented Mar 22, 2024

Apart from the Update or Patch API, I also observed that the equality check on the Deployment object will always result in false (mean to trigger updateChecker), because the Deployment object in Kubernetes has generated value on some field compared to what's being created in the EG infrastructure layer.

return i.Client.CreateOrUpdate(ctx, key, current, deployment, func() bool {
return !cmp.Equal(current.Spec, deployment.Spec, opts...)
})

This is also the reason why EG keeps reconciling the infrastructure resource on any operation to Gateway API/Envoy Gateway resource, even though there is no changed on the infra layer. For instance, when a user supplies non-default EnvoyDeployment but leaves the pod.securityContext to empty (nil), during the comparison, the current Deployment object would contain the generated value of pod.securityContext as a result it became unequal. Please see the screenshot below when I try to print out the diff using cmp.Diff.

image

@ardikabs
Copy link
Contributor Author

if this makes sense, I would like to work on this @arkodg

@arkodg arkodg added kind/bug Something isn't working area/infra-mgr Issues related to the provisioner used for provisioning the managed Envoy Proxy fleet. and removed triage labels Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/infra-mgr Issues related to the provisioner used for provisioning the managed Envoy Proxy fleet. kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants