doc: Update existing resources with owner refs #2013
Conversation
asmacdo
added
language/ansible
openshift-ci-robot
added
the
size/L
|
/retest |
1 similar comment
|
/retest |
There was a problem hiding this comment.
Overall content looks good, I think this is tied more to the owner reference injection settings (as described here than dependentWatches, so it probably should go in its own doc with a link in the advanced_options page.
Couple other small questions/comments, but nice work!
|
Hi @fabianvf, Could we not do it be done automatically by using the |
|
@camilamacedo86 The issue is that we don't know whether a resource we're receiving in the proxy is one that we created previously, we'll only see updates coming in (and we don't want the operator to own every resource it touches, just the ones that it creates). |
|
Once all the changes @fabianvf suggested are made I'll take another look at it. |
asmacdo
force-pushed
the
1977-doc-belated-owner-reference
branch
from
5e156a2 to
4ec3749
Compare
Operator deployed resources created while dependent watches are disabled cannot have owner information retroactively injected by the proxy. This patch documents the process of adding that information to existing resources manually. Fixes operator-framework#1977
asmacdo
force-pushed
the
1977-doc-belated-owner-reference
branch
from
4ec3749 to
45ac3f8
Compare
|
/retest |
| - apiVersion: cache.example.com/v1alpha1 | ||
| kind: Memcached | ||
| name: example-memcached | ||
| uid: ad834522-d9a5-4841-beac-991ff3798c00 |
There was a problem hiding this comment.
Hi @fabianvf,
Is required to add the UID? If I would like to do it for a pod for example and the pod be re-created automatically by the API platform which is the default behaviour would not it assume a new UID? So, should not we be able to do that without informing a UID?
There was a problem hiding this comment.
Yes, the UID field is required to construct an ownerReference. This is to correctly garbage collect if the owner is deleted and then re-created
There was a problem hiding this comment.
WDYT about we add a note with this info @asmacdo?
There was a problem hiding this comment.
To me, the delete/recreate edge case seems like an implementation detail which we should leave out to keep the guide simple. I'm happy to add it if it would be helpful though.
| resource is a cluster level resource. | ||
|
|
||
| `annotation` structure: | ||
| * operator-sdk/primary-resource: {metadata.namepace}/{metadata.name} |
There was a problem hiding this comment.
I think we would need here specify what is a primary-resource or add a link for where it is defined.
There was a problem hiding this comment.
What other info do you think is needed, other than how to construct it? I don't see it anywhere in our docs, but it seems like construction is all the user will need to know.
|
|
||
| ### Owner References and Annotations | ||
|
|
||
| Dependent resources *within the same namespace as the owning CR* are |
| apiVersion: v1 | ||
| kind: Namespace | ||
| ``` | ||
| #### playbook.yml |
There was a problem hiding this comment.
| #### playbook.yml | |
| ```suggestion | |
| **<add scallfold path>/playbook.yml** |
There was a problem hiding this comment.
There is no path or scaffolding for this playbook.yml, it's just an example that the user could copy into a file and use as a starting point for doing this migration.
There was a problem hiding this comment.
The way I wrote this is a little misleading. I'll reword some things to make this clear.
| uid: "{{ extra_owner_data.resources[0].metadata.uid }}" | ||
| ``` | ||
|
|
||
| #### `each_resource.yml` |
There was a problem hiding this comment.
Which means each_resource.yml? It is not a file in the scaffold project?
| #### `each_resource.yml` | |
| **for each resource** | |
| Following an example for each resource that .... |
There was a problem hiding this comment.
No, it's a separate file that the user would need to create, these files just serve as a starting point for automating the migration.
| If you have many resources to update, it may be easier to use the | ||
| following (unsupported) playbook. | ||
|
|
||
| #### vars.yml |
There was a problem hiding this comment.
I do not think that we should use #### for name of files. The reason for that is that the ### means a subsection, so would make sense we have a subsection in an index with vars.yaml ?
There was a problem hiding this comment.
I skipped the ## level, so on the next commit, I'll:
s/###/##/ and s/####/###/
| name: example-memcached | ||
| uid: ad834522-d9a5-4841-beac-991ff3798c00 | ||
| ``` | ||
|
|
There was a problem hiding this comment.
WDYT?
| **NOTE**: The UID field is required to construct an ownerReference. Then, this is to correctly garbage collect if the owner is deleted and then re-created. So, the owner reference is not lost if the resource needs to be re-created by the API. |
Is my understand correctly @fabianvf ?
There was a problem hiding this comment.
@asmacdo great work 🥇
I added just few NOTES suggestions based on the clarifications of comments.
Besides these small fews nits all is 100%.
/lgtm
/approved
|
/retest |
|
@asmacdo: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Operator deployed resources created while dependent watches are disabled
cannot have owner information retroactively injected by the proxy. This
patch documents the process of adding that information to existing
resources manually.