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
Introduce PodHasNetwork condition for pods #111358
Conversation
@ddebroy: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
/sig-node |
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
/test pull-kubernetes-conformance-kind-ga-only-parallel |
/label api-review |
/remove-label api-review |
@@ -43,3 +43,13 @@ const ( | |||
LimitedSwap = "LimitedSwap" | |||
UnlimitedSwap = "UnlimitedSwap" | |||
) | |||
|
|||
// Alpha conditions managed by Kubelet that are not yet part of the API. The |
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.
This follows the advice from @liggitt around constants associated with new conditions at #110959 (comment)
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.
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.
this confused me. so thanks for the pointer.
/assign @derekwaynecarr |
Signed-off-by: Deep Debroy <ddebroy@gmail.com>
/test pull-node-e2e-serial containerd |
@ddebroy: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
/test pull-kubernetes-node-kubelet-serial-containerd |
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.
The kubelet changes all look great, and I think the condition will help users!
I had expected to see the kube-apiserver drop usage of the alpha condition in DropDisabledFields
for the associated pod so clients would not see the condition when its not enabled on kube-apiserver feature gate.
@liggitt can you confirm you share the same expectation? by keeping conditions during the alpha phase as a const local to the writer (kubelet), we lack the mirror capability of dropping the field on kube-apiserver side. is there a write-up about conditions that says dropping is not required that I missed?
@@ -43,3 +43,13 @@ const ( | |||
LimitedSwap = "LimitedSwap" | |||
UnlimitedSwap = "UnlimitedSwap" | |||
) | |||
|
|||
// Alpha conditions managed by Kubelet that are not yet part of the API. The |
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.
this confused me. so thanks for the pointer.
/test pull-kubernetes-e2e-kind-ipv6 |
Dropping disabled fields is done for new API fields so we don't allow persisting data in fields an n-1 API server doesn't know about and can't preserve. If this feature is using pod conditions, the fields are not new, just the particular values in the fields, right? I wouldn't expect the API to drop specific pod conditions based on feature gates. |
@liggitt thanks for the reply. /approve |
/milestone v1.25 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ddebroy, derekwaynecarr The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test pull-kubernetes-node-kubelet-serial-containerd |
/hold |
/lgtm |
@ddebroy: 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. |
Validated the logs in |
/remove-hold |
to follow up, what will be the expectation for hostNetwork pods? |
@aojea, for pods with hostNetwork configured, the fields in |
thanks for the prompt response |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR introduces a new
PodHasNetwork
pod condition managed by Kubelet and applied on pods. The condition is set when the pod sandbox has been created and the network configured by the CRI runtime. For use cases of how this condition will be consumed, please see KEP.Example: Pod referencing a missing configmap (and does not specify init containers):
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
As described in the KEP, the decisions around setting this condition is based on
KubeRuntime
's existingPodSandboxChanged
function. This PR moves thePodSandboxChanged
function and a couple of dependent functions to a freshkubeRuntime/utils
location so that the status manager component can import the logic without cyclical dependencies.Please see notes about a API review not necessary at Alpha stage for this in #111358 (comment)
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
KEP-3085: Pod network ready condition