AEP: VPA native sidecar support #8905
Conversation
k8s-ci-robot
added
do-not-merge/release-note-label-needed
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jklaw90 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/M
jklaw90
changed the title
omerap12
left a comment
omerap12
left a comment
There was a problem hiding this comment.
Thanks for opening this, but generally an AEP should include more detailed information.
For example, it should show what the ContainerTypeInitSidecar will look like in Go, how the ClusterFeeder will be extended, and so on.
Additionally, since this introduces a new feature, it should be guarded behind a feature gate.
I encourage you to review some of the existing AEPs to get a better sense of the expected structure and level of detail.
jklaw90
force-pushed
the
AEP-8905-native-sidecar-support
branch
from
b68b327 to
2b6d2a3
Compare
jklaw90
force-pushed
the
AEP-8905-native-sidecar-support
branch
from
2b6d2a3 to
e98f9d3
Compare
k8s-ci-robot
added
release-note-none
| Both Updater and Admission Controller components retrieve recommendations for init containers (including native sidecars) separately from standard containers using `GetContainersResourcesForPod`. | ||
| The patch generation logic is updated to target `/spec/initContainers` for native sidecar containers, while normal containers will continue to update `/spec/containers`. |
There was a problem hiding this comment.
Personally, I would prefer to include the updated code snippets in the AEP so users can clearly understand what this change looks like.
k8s-ci-robot
added
size/L
|
|
||
| ## Proposal | ||
|
|
||
| The proposal is to introduce a new feature gate `NativeSidecar` in VPA. When enabled, VPA components will recognize and handle native sidecar containers. |
There was a problem hiding this comment.
The proposal of this AEP isn't really to add the feature gate, it's to add a feature that will allow for native side car support.
The feature gate is a mechanism to roll that feature out safely.
I think the feature gate can be mentioned in the details, but it itself isn't the proposal
| The Recommender component identifies native sidecar containers by examining init containers with `restartPolicy: Always` in the `SpecClient`. These containers are assigned the `ContainerTypeInitSidecar` type. | ||
|
|
||
| When the `NativeSidecar` feature gate is enabled, the `ClusterFeeder` processes native sidecars similarly to standard containers: | ||
| - They are stored in a separate `InitSidecarsContainers` map in the pod state |
There was a problem hiding this comment.
Is there value in doing this? What about storing the containers in the existing Containers map?
The names of containers and initContainers are all unique, so there won't be a collision
There was a problem hiding this comment.
Oh, I assume when the update of the pod spec happens, its useful to know where that container lives
| - Admission applies recommendations to native sidecars. | ||
| - Updater will update sidecar container resources in-place or evict. | ||
| - Admission will patch sidecar container resources. | ||
| - When the feature gate `NativeSidecar` is false VPA components will not modify native sidecars. |
There was a problem hiding this comment.
A negative test would be useful too, to ensure that initContainers (that are not sidecars) are not matched
| #### Downgrade | ||
|
|
||
| On downgrade of VPA from 1.6.0 (tentative release version), nothing will change. | ||
| VPAs will continue to work as previously. Checkpoints may contain sidecar resource information until updated, but updater and admission will modify sidecar resources. |
There was a problem hiding this comment.
I assume "will not"?
| VPAs will continue to work as previously. Checkpoints may contain sidecar resource information until updated, but updater and admission will modify sidecar resources. | |
| VPAs will continue to work as previously. Checkpoints may contain sidecar resource information until updated, but updater and admission will not modify sidecar resources. |
4 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Created due to #7229
This PR adds an AEP to add support for native sidecars.
Which issue(s) this PR fixes:
N/A
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: