-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
KEP-4753: Expose ownerReferences via downward API #4754
base: master
Are you sure you want to change the base?
Conversation
/cc @thockin |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ArangoGutierrez The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
6ae4faf
to
b57c505
Compare
|
||
## Summary | ||
|
||
Today when a pod wants to pass its 'onwerReferences' to a new object it manage (a ConfigMap, a Secret, etc), it needs to do it call the API server to get it's own ownerReferences and then pass it to the new object. |
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.
Today when a pod wants to pass its 'onwerReferences' to a new object it manage (a ConfigMap, a Secret, etc), it needs to do it call the API server to get it's own ownerReferences and then pass it to the new object. | |
Today when a pod wants to pass its 'ownerReferences' to a new object it manage (a ConfigMap, a Secret, etc), it needs to do it call the API server to get it's own ownerReferences and then pass it to the new object. |
|
||
- <test>: <link to test coverage> | ||
|
||
### Graduation Criteria |
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.
Given that this is not a API change, you may be able to skip alpha stage. WDYT @thockin?
|
||
## Motivation | ||
|
||
Deployments and ReplicaSets can orphan and adopt pods dinamically. Any additional information we provide can change over time. On day 1 a pod named "foobar-deadbeef93-xyz76" is owned by replicaset "foobar-deadbeef93" which is owned by deployment "foobar", but after a node restart, the pod name could change to "foobar-deadbeef93-abc12", leaving the object created by "foobar-deadbeef93-xyz76" orphan, and triggering unwanted behavior. Enabling the pod to pass its ownerReferences to the new object it manages will prevent objects from being orphaned by pods that are owned by a higher level object. |
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.
I'm afraid this motivation doesn't make much sense to me. I think you've got some context in your head that is not in the text.
Why does a pod need to know it's own ownerRefs ? You seem to be implying that the pod will create some other object (e.g. a ConfigMap) and rather than parent that object itself, it wants to create the ConfigMap and set its ownerRef to the pod's ownerRef? Why?
It seems plausible to me that a pod would be able to know its own ownerRefs, but that use-case eludes me.
Help me understand what a pod will actually DO with this information?
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.
how about now?
#### Story 1 | ||
xref https://github.com/kubernetes-sigs/node-feature-discovery/issues/1752 | ||
|
||
CustomResource object created by a pod owned by a DaemonSet is garbage collected when the pod is deleted by unexpected reasons. This triggers unwanted behavior in the system. |
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.
Ahh, the DaemonSet part helps clarify a little. Can you make this even more concrete?
The intention behind this design is not to add extra structs or fields | ||
but to extend the existing `downward API` to include the `metadata.ownerReferences` field. | ||
|
||
### Implementation Details |
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.
Can you please say more about how this will present both as a file and in an env?
Can a user request a specific index of ownerRefs or does it always get all of them?
What does the value of the env var actually look like? OwnerRef is a struct with several fields, are we saying it will be JSON-formatted? Or some other bespoke format? What if I have 2 or 5 or 10 owners? What if I have 0 owners.
Same question, but for file. If I cat
that file - what do I see?
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.
How about now?
b57c505
to
bad8d3d
Compare
Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
bad8d3d
to
c904f3b
Compare
|
||
## Summary | ||
|
||
Today when a pod wants to pass its `ownerReferences` to a new object it manage (a ConfigMap, a Secret, etc), it needs to do it call the API server to get it's own ownerReferences and then pass it to the new object. |
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.
Today when a pod wants to pass its `ownerReferences` to a new object it manage (a ConfigMap, a Secret, etc), it needs to do it call the API server to get it's own ownerReferences and then pass it to the new object. | |
Today, when a pod wants to pass its `ownerReferences` to a new object it manages (a ConfigMap, a Secret, etc), the pod needs it to call the API server to get its own ownerReferences and then pass it to the new object. |
|
||
## Motivation | ||
|
||
DaemonSets and ReplicaSets (via Deployments) can dynamically orphan and adopt pods. Consider a scenario where, on day 1, a pod named foobar-deadbeef93-xyz76 is owned by the ReplicaSet foobar-deadbeef93, which in turn is managed by the Deployment foobar. After a node restart, the pod might be replaced by foobar-deadbeef93-abc12, orphaning the original pod foobar-deadbeef93-xyz76 and potentially causing unwanted behavior. This issue arises when pods create resources like ConfigMaps or CustomResources and these resources are not correctly reassigned to the new pod, leading to orphaned objects. Ensuring that pods can pass their ownerReferences to new objects they manage would prevent this issue, as the ownership hierarchy would be preserved even when pods are recreated. |
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.
After a node restart, the pod might be replaced by foobar-deadbeef93-abc12, orphaning the original pod foobar-deadbeef93-xyz76 and potentially causing unwanted behavior
can this really happen? it this not a bug on the replicaset?
uid: 6be1683f-da9c-4f68-9440-82376231cfa6 | ||
``` | ||
|
||
If `ownerReferences` is empty or not set, the file will be empty. |
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.
what happen if ownerReferences
is updated? is it reflected in the Pod or the Pod keeps using the original value?
|
||
If `ownerReferences` is empty or not set, the file will be empty. | ||
|
||
Is out of the scopt of this KEP to add the `ownerReferences` to the pod's environment variables. |
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.
Is out of the scopt of this KEP to add the `ownerReferences` to the pod's environment variables. | |
Is out of the scope of this KEP to add the `ownerReferences` to the pod's environment variables. |
|
||
e2e testing will consist of the following: | ||
- Create a Deployment and or DaemonSet | ||
- Verify if the pod created by the Deployment/DaemonSet has the ownerReferences of the Deployment/DaemonSet via ENV VAR |
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.
is this correct? you said before env variables are out of scope
Is out of the scopt of this KEP to add the
ownerReferences
to the pod's environment variables.
One-line PR description: Adding a new KEP
Issue link: KEP-4753: Expose
ownerReferences
viavalueFrom
and downward API #4753