mockkubeapiserver: Refactor storage to be pluggable #355
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
justinsb
force-pushed
the
refactor_storage
branch
5 times, most recently
from
c317b37
to
b6aa657
Compare
This helps if we want e.g. to capture all the objects easily.
justinsb
force-pushed
the
refactor_storage
branch
from
b6aa657
to
ccba24e
Compare
| return fmt.Errorf("status was of unexpected type %T", statusObj) | ||
| } | ||
|
|
||
| generation := u.GetGeneration() |
There was a problem hiding this comment.
Just curious, why do we want to setup the generation here?
I saw it is optional and in k-d-p there seems to be also no specific logic around generation.
There was a problem hiding this comment.
Good question :-) I think I added it because otherwise kstatus will not consider the deployment ready (which makes our golden tests unhappy / means that they don't converge)
| // These changes seem to be done synchronously (similar to a mutating webhook) | ||
| labels := u.GetLabels() | ||
| name := u.GetName() | ||
| if labels["kubernetes.io/metadata.name"] != name { |
There was a problem hiding this comment.
Yeah, I think it is legacy, but ... it's there. I think we prefer field selectors now: https://kubernetes.io/docs/concepts/overview/working-with-objects/field-selectors/
Only a handful of fieldSelectors are implemented for some objects (though I think it would be cool if we supported arbitrary CEL expressions). fieldSelector for metadata.name is implemented everywhere I believe, because that is how we can watch one object (you can only pass the watch parameter to LIST requests, IIRC)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb, yuwenma The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -0,0 +1,33 @@ | |||
| package storage | |||
There was a problem hiding this comment.
I like this style in storage, and the hook (I misread the rationale of Hook until seeing the "CRDHook")
| // UpdateCRD is called whenever a CRD is updated. | ||
| // We register the types from the CRD. | ||
| func (s *MemoryStorage) UpdateCRD(ev *storage.WatchEvent) error { | ||
| // TODO: Deleted / changed CRDs |
There was a problem hiding this comment.
What is the expected behavior of "delete CRD". Is this for the case when CRD belongs to a versioned manifest?
There was a problem hiding this comment.
If a CRD is deleted, we should unregister it from the apiserver, so it isn't discoverable, you can't query it etc. That's ... not implemented yet!
| } | ||
|
|
||
| type resourceVersionClock struct { | ||
| now atomic.Int64 |
There was a problem hiding this comment.
I missed the atomic in the previous review. This is cool.
There was a problem hiding this comment.
Yes, in general it's premature optimization vs sync.Mutex, but here I find it more ergonomic (as well as being possibly more efficient)
| } | ||
| } | ||
|
|
||
| ev := storage.BuildWatchEvent(gvk, "ADDED", obj) |
There was a problem hiding this comment.
Just to make sure I understand the code correctly: this "ADDED" is a no-op and here we mainly use the watchCallback (next line), right?
There was a problem hiding this comment.
This is the "real" watch code, the hooks are a sort of "fast-path built-in watch". When you start a watch with kube-apiserver with resourceVersion unspecified, you get a synthetic ADDED event for every object that exists at that time.
It's actually a streaming LIST (which is otherwise missing from kube), and a little more efficient on the client (at least) in terms of memory, because you only have to process one object at a time, instead of a whole page of objects. The downside is that it's hard to tell when the initial LIST is over, and you have all the objects. In the past few versions of kube-apiserver, they added a bookmark notification when the initial list was completed, which is how you can tell. Maybe I should add bookmark notifications :-) More info here: https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/3157-watch-list/README.md
|
/lgtm |
k8s-ci-robot
added
the
lgtm
This helps if we want e.g. to capture all the objects easily.