-
Notifications
You must be signed in to change notification settings - Fork 14
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
Resource client patch for apply status #517
Merged
+490
−117
Merged
Changes from 39 commits
Commits
Show all changes
42 commits
Select commit
Hold shift + click to select a range
2016348
Add patch to resource client to cut api calls in half
b3d9c49
Move to apply status since consul api doesn't have patch
ed3ded9
Implement the other apply status functions
932d53b
Get rid of debug info
80eb2ce
Clean up extra code
d58ce2c
Actually use the bytes from the provided status
791427f
Add changelog
e5f2e19
Pod resource client to implement interface
ac38ae7
Api client to implement interface
63a18d3
Namespace resource client to implement interface
26b02e8
Use more performant one with standard k8s clients
a62d491
Namespace client needs too
76c3a35
Codegen
245e753
Fix reporter unit test
fe923b5
Fix reconciler unit tests
3762a69
Codegen
e3d5cf9
Update interface to use status client
3eac75a
Codegen
0f5cca9
Use namespaced status by default
5bb6c80
Fix reporter unit test
d911353
Fix reconciler unit test
7b0c2a4
Prefer merge patch type
16fbc92
Ensure we render all fields
dbfe07d
Codegen
770de5d
Go mod tidy
9ebc5c0
Merge refs/heads/master into resource_client_patch
soloio-bulldozer[bot] c0bf2f8
Prefer json patch type
f5c5bbe
Merge branch 'resource_client_patch' of github.com:solo-io/solo-kit i…
965798f
Ensure statuses is always defaulted
26eeb55
Prefer json encoding
b6bf50a
Codegen
b2896a8
Improve comment readability
0c2cbec
Get rid of gogo
7de9cec
Get rid of gogo
ce6d2ed
Get rid of gogo, with codegen
dd12a82
Move shared impl to shared package
9b56c85
Move shared impl for jsonpatch to shared package
48d1f46
Move other clients to shared impl for jsonpatch
80b78a0
Remove gogo
7ed5390
Only clone if we are going to write status
b9532a5
Be clear we are patching the status
fa41615
Add note in changelog about rendering enums as strings
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
changelog: | ||
- type: FIX | ||
resolvesIssue: false | ||
description: Improve scalability of v2 status reporter by using k8s patch instead of read then write. | ||
issueLink: https://github.com/solo-io/gloo/issues/7076 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
package shared | ||
|
||
import ( | ||
"bytes" | ||
"fmt" | ||
|
||
"github.com/golang/protobuf/jsonpb" | ||
"github.com/solo-io/solo-kit/pkg/api/v1/clients" | ||
"github.com/solo-io/solo-kit/pkg/api/v1/resources" | ||
"github.com/solo-io/solo-kit/pkg/errors" | ||
) | ||
|
||
// ApplyStatus is used by clients that don't support patch updates to resource statuses (e.g. consul, files, in-memory) | ||
func ApplyStatus(rc clients.ResourceClient, statusClient resources.StatusClient, inputResource resources.InputResource, opts clients.ApplyStatusOpts) (resources.Resource, error) { | ||
name := inputResource.GetMetadata().GetName() | ||
namespace := inputResource.GetMetadata().GetNamespace() | ||
res, err := rc.Read(namespace, name, clients.ReadOpts{ | ||
Ctx: opts.Ctx, | ||
Cluster: opts.Cluster, | ||
}) | ||
|
||
if err != nil { | ||
return nil, errors.Wrapf(err, "error reading before applying status") | ||
} | ||
|
||
inputRes, ok := res.(resources.InputResource) | ||
if !ok { | ||
return nil, errors.Errorf("error converting resource of type %T to input resource to apply status", res) | ||
} | ||
|
||
statusClient.SetStatus(inputRes, statusClient.GetStatus(inputResource)) | ||
updatedRes, err := rc.Write(inputRes, clients.WriteOpts{ | ||
Ctx: opts.Ctx, | ||
OverwriteExisting: true, | ||
}) | ||
|
||
if err != nil { | ||
return nil, errors.Wrapf(err, "error writing to apply status") | ||
} | ||
return updatedRes, nil | ||
} | ||
|
||
// GetJsonPatchData returns the json patch data for the input resource. | ||
// Prefer using json patch for single api call status updates when supported (e.g. k8s) to avoid ratelimiting | ||
// to the k8s apiserver (e.g. https://github.com/solo-io/gloo/blob/a083522af0a4ce22f4d2adf3a02470f782d5a865/projects/gloo/api/v1/settings.proto#L337-L350) | ||
func GetJsonPatchData(inputResource resources.InputResource) ([]byte, error) { | ||
namespacedStatuses := inputResource.GetNamespacedStatuses().GetStatuses() | ||
if len(namespacedStatuses) != 1 { | ||
// we only expect our namespace to report here; we don't want to blow away statuses from other reporters | ||
return nil, errors.Errorf("unexpected number of namespaces in input resource: %v", len(inputResource.GetNamespacedStatuses().GetStatuses())) | ||
} | ||
ns := "" | ||
for loopNs := range inputResource.GetNamespacedStatuses().GetStatuses() { | ||
ns = loopNs | ||
} | ||
status := inputResource.GetNamespacedStatuses().GetStatuses()[ns] | ||
|
||
buf := &bytes.Buffer{} | ||
var marshaller jsonpb.Marshaler | ||
marshaller.EnumsAsInts = false // prefer jsonpb over encoding/json marshaller since it renders enum as string not int (i.e., state is human-readable) | ||
marshaller.EmitDefaults = false // keep status as small as possible | ||
err := marshaller.Marshal(buf, status) | ||
if err != nil { | ||
return nil, errors.Wrapf(err, "marshalling input resource") | ||
} | ||
|
||
bytes := buf.Bytes() | ||
patch := fmt.Sprintf(`[{"op": "replace", "path": "/status/statuses/%s", "value": %s}]`, ns, string(bytes)) // only replace our status so other reporters are not affected (e.g. blue-green of gloo) | ||
data := []byte(patch) | ||
return data, nil | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently we write out ints for the states. If we want to switch to writing strings we should call it out in the changelog. I assume that this change won't affect any tooling around statuses because the unmarshaller will handle it the same regardless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
happy to call that out in the changelog. yeah we currently use the jsonpb to unmarshal so it can handle either format, so im just opting for better readability