delete templateinstances in foreground where necessary in extended tests#16979
Conversation
openshift-ci-robot
added
the
size/M
|
/lgtm |
openshift-merge-robot
added
the
approved
bparees
added
kind/bug
openshift-merge-robot
added
the
approved
jim-minter
force-pushed
the
issue16775-2
branch
from
a9a1e11 to
1a072ae
Compare
openshift-ci-robot
added
size/L
openshift-merge-robot
removed
the
lgtm
|
|
||
| templateInstance := obj.(*templateapi.TemplateInstance) | ||
| oldTemplateInstance := old.(*templateapi.TemplateInstance) | ||
| // Decode Spec.Template.Objects on both obj and old to Unstructureds. This |
There was a problem hiding this comment.
The Spec.Objects field of a v1.Template is a []RawExtension. The garbage collector gets TemplateInstances (which wrap Templates) into Unstructureds, then posts them back to update the finalisers. In this case the new Objects field received by the API server is semantically identical to the old one, but often the serialisation is different. We were previously rejecting the update because of this. Is this solution OK or is there a better alternative you can suggest?
There was a problem hiding this comment.
can you give an example of this? I'd expect perfect fidelity when using unstructured objects.
There was a problem hiding this comment.
If you deserialise to Unstructured and re-serialise, it doesn't guarantee to maintain the order of keys in a json object. This was the example I found:
"old" value (fetched from etcd)
{"kind":"Secret","apiVersion":"v1","metadata":{"name":"secret1","creationTimestamp":null}}
"new" value (submitted by GC, been through the Unstructured mangler)
{"apiVersion":"v1","kind":"Secret","metadata":{"creationTimestamp":null,"name":"secret1"}}
There was a problem hiding this comment.
The Spec.Objects field of a v1.Template is a []RawExtension. The garbage collector gets TemplateInstances (which wrap Templates) into Unstructureds, then posts them back to update the finalisers. In this case the new Objects field received by the API server is semantically identical to the old one, but often the serialisation is different. We were previously rejecting the update because of this. Is this solution OK or is there a better alternative you can suggest?
I'd open an issue and tag @ironcladlou or fix GC yourself to issue patches instead of updates.
Do you know what is wrong with the semantic equality check that causes it to fail on reordered json fields? https://github.com/openshift/origin/blob/master/vendor/k8s.io/kubernetes/pkg/registry/rbac/helpers.go#L47
There was a problem hiding this comment.
Do you know what is wrong with the semantic equality check that causes it to fail on reordered json fields?
They're []byte blobs where they're compared, not map[string]interface{}
|
/hold |
jim-minter
added
the
do-not-merge/hold
|
/retest |
jim-minter
removed
the
do-not-merge/hold
|
/lgtm The API already requires json, so this will only fail in already failing cases where the templates are unusable. We should still try to make GC issue patches. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bparees, deads2k, jim-minter The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
Upstream issue for GC issuing patches is kubernetes/kubernetes#54430 |
|
Automatic merge from submit-queue. |
Automatic merge from submit-queue (batch tested with PRs 17007, 17003, 17001, 17009). wait for group cache in templateinstance tests fixes #16654 (builds on #16979) @gabemontero fyi
6 participants
fixes #16775 (2nd attempt)
Proposing to comment out the previous "fix" in case it's superstition