Closed yuwenma closed 1 year ago
Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all
/assign @justinsb
@justinsb Regarding our discussions about RESTMappings, I think it involves two problems (see the third commit) and it's kind of complicated in the code, so I'd like to explain a little bit more:
CRD and its CR objects are deployed in reverse order and it results in missing RESTMapping
We uses the k-d-p/ControllerRESTMapper
to keep track and cache all the RESTMappings in history (ControllerRESTMapper is initialized on the k-d-p/Reconciler level).
During the applyset Apply
, If one manifest apply fails, we continue with the next manifest. The failed apply will trigger a re-deploy, so the CR object can be applied successfully. And both CRD and CR RESTMappings are cached in the ControllerRESTMapper
.
This guarantees that if the CR object is ordered before CRD, both of them can still be deployed successfully in two Apply
runs, and both RESTMappings are cached.
k-d-p/ applyset
needs to provide kubectl.ApplySet
the RESTMappings info so that it can efficiently find the right objects to prune.
This actually involves two different kinds of RESTMappings
applyset.kubernetes.io/contains-group-resources
annotation to get the previously applied manifests GVRs.Besides caching all the RESTMappings as an attribute to ApplysetApplier
, we initialize the kubectl ApplySet
in each Apply
run, and we provide both caching-all RESTMapping and current-only RESTMapping separately to the kubectl ApplySet
kubectl.ApplySet
will use the caching-all RESTMapper to parse the parent annotation to get the previously applied GVRs.ApplySet
will register the current manifest's RESTMappings after it's successfully applied to the kubectlapply.ApplySet
(a different var than the caching-all RESTMappings)@justinsb Besides the RESTMapping, another problem is about the tradeoff between the reliability and efficiency of updating the parent object in the cluster. I'd like to know if the following change makes sense to you:
ApplyOnce
updates the parent (first call, Patch
) labels and annotations with the RESTMappings infos (requires both the current and caching-all, since it is a "superset"), this is the BeforeApply
methodPatch
) labels and annotations with the RESTMappings info (requires the current-only RESTMappings, this is a "last") Status.Patch
)The problem of the previous workflow is that we cannot get the current manifests RESTMappings before apply efficiently, I think you pointed it out here but we cannot return errors either because it will end up the ApplyOnce
with no-op.
I remove the BeforeApply
completely. The downside is that if the operator cashes during manifests apply, the parent may not be able to track the current manifest GVRs, and this can end up as missed pruning objects.
ApplyOnce
applies manifest, and then caches each manifest's RESTMapping for kubectl.ApplySet
if the apply succeeds.ApplySet
as runtime.Object
, and the labels and annotations is updated at the end of the pruning. Reconcile
after apply succeeds. (first call, Patch
) Status.Patch
)Thanks - this looks great now, and appears to be additive, so I propose we merge and iterate on anything we find.
/approve /lgtm
[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
This PR adds the server-side pruning logic for ApplysetApplier.
How to use
Use Cases
First Time Reconcile
applyset.kubernetes.io/id: PARENT_ID
applyset.kubernetes.io/part-of: PARENT_ID
applyset.kubernetes.io/contains-group-resources
which contains all the manifests GRapplyset.kubernetes.io/additional-namespaces
which contains all the namespaces those manifests are in.applyset.kubernetes.io/tooling
whose default value is the your CR object's kind.New Manifest GVR
If you upgrade the declarative CR version, which contains new manifests GroupVersionResource, you can expect:
applyset.kubernetes.io/contains-group-resources
will include new GVRapplyset.kubernetes.io/part-of
Manifest no longer needed
If you upgrade the declarative CR version, which no longer contain certain manifests, you can expect:
applyset.kubernetes.io/contains-group-resources
will be updated.You can find logs like below