Skip to content
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

Add blog post for PodHasNetwork condition #36197

Merged
merged 1 commit into from Sep 5, 2022

Conversation

ddebroy
Copy link
Member

@ddebroy ddebroy commented Aug 23, 2022

Blog post for PodHasNetwork condition feature [https://github.com/kubernetes/enhancements/issues/3085]

Supersedes #35645

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/blog Issues or PRs related to the Kubernetes Blog subproject labels Aug 23, 2022
@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Aug 23, 2022
@netlify
Copy link

netlify bot commented Aug 23, 2022

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 2c0be33
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/6310df2cd4f52400093e3b92
😎 Deploy Preview https://deploy-preview-36197--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could publish this if necessary. I see a couple of things I'd prefer to have fixed.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 23, 2022
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 8b7ca860043ea81551160a40e885292995009efa

@sftim
Copy link
Contributor

sftim commented Aug 23, 2022

/retitle Add blog post for PodHasNetwork condition

@k8s-ci-robot k8s-ci-robot changed the title Blog post for PodHasNetwork condition Add blog post for PodHasNetwork condition Aug 23, 2022
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 23, 2022
Copy link
Contributor

@chrisnegus chrisnegus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reads very well. I have a couple of small suggestions.

@sftim
Copy link
Contributor

sftim commented Aug 27, 2022

/lgtm

Tweaks welcome (either before merge, or afterwards)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 27, 2022
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: a30ccee5f224329a523dd385d9dffaecc70722c3

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2022
@sftim
Copy link
Contributor

sftim commented Aug 30, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2022
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: ffab27cc705a287edc4733dff0810ce611d2febe

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some comments and suggestions.. cheers

accurate data around when the pod runtime sandbox was initialized with
networking configured so that the kubelet can proceed to launch user-configured
containers (including init containers) in the pod.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Additionally, some cloud providers may attach additional network interface(s) to the network namespace of a pod, via CNI, some time after the pod is up and running with a loopback interface. In this case the `PodHasNetwork` condition may not reflect whether all network interfaces of the pod are initialized.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you think this needs more clarification in the general context for most pod lifecycles .. feel free to leave it out.. we can discuss further

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this special behavior strictly something that happens in the cloud? (could an on-prem cluster do something similar?)

If we're adding text here, we could make it clear that PodHasNetwork means that for each configured address family, the kubelet sees that [the one IP address that a conformant network plugin should set up for that address family] is configured and up.

Would need better wording than mine, mind!

Copy link
Member Author

@ddebroy ddebroy Aug 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a brief sync about this with Mike yesterday. My understanding is what Mike described above could be possible if some other (privileged) node agent aside from the Kubelet is configuring things in the network namespace of the pod out-of-band from Kubelet's pod sandbox bring-up and network configuration through CRI (i.e. outside of the Kubelet runtime => CRI => Runtime (containerd/crio/etc) => CNI => CNI plugins flow). I will make a note of this.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: CRI (or wider - cloud providers, as suggested by @mikebrow) can do anything it/they want at any point of pod lifecycle. it can call a cni plugin, or multiple of them, or whatever it wants. but that's out of the scope of the description of "kubernetes contract".
i didn't yet red the mentioned KEP so i'm not touching the topic how the data should be explained, but looking on @mikebrow description i would expect that PodHasNetwork will be only describing status of the default network connectivity, ignoring all "whathever will cri call/setup" things which are not the default access to kubeapi network things (like e.g. what is hold in extensions under the https://github.com/k8snetworkplumbingwg/ meaning, or more precisely under the https://docs.google.com/document/d/1Ny03h6IDVy_e_vmElOqR7UdTPAG_RNydhVE1Kx54kFQ/edit#heading=h.hylsbqoj5fxd extension)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the inputs and clarifications about this, @mikebrow, @sftim and @jellonek. I will summarize and call out the above point.

Copy link
Member Author

@ddebroy ddebroy Sep 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarified this. For reference, https://github.com/maiqueb/multus-dynamic-networks-controller appears to be a concrete PoC implementation of what was being referred to above.

@sftim
Copy link
Contributor

sftim commented Aug 31, 2022

@ddebroy would you have capacity to update this based on feedback so far?

@ddebroy
Copy link
Member Author

ddebroy commented Aug 31, 2022

Yes, I will be addressing all the outstanding feedback by EoD.

@aojea
Copy link
Member

aojea commented Aug 31, 2022

/assign

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2022
Signed-off-by: Deep Debroy <ddebroy@gmail.com>
@ddebroy
Copy link
Member Author

ddebroy commented Sep 1, 2022

All comments and feedback above have been addressed.

@sftim
Copy link
Contributor

sftim commented Sep 5, 2022

@aojea did you want to check this further?

@aojea
Copy link
Member

aojea commented Sep 5, 2022

@aojea did you want to check this further?

I don't have enough knowledge of the feature to judge, but it seems to correctly match the KEP, sig-network and CNI implementors will follow up and try to give more feedback for the next stages

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nit about the title. I'm fine to have that fixed either

  • never
  • in a follow-up PR (by anyone)

/lgtm
/approve

Thanks and congratulations!

@@ -0,0 +1,123 @@
---
layout: blog
title: 'Kubernetes 1.25: PodHasNetwork condition for pods'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
title: 'Kubernetes 1.25: PodHasNetwork condition for pods'
title: 'Kubernetes 1.25: PodHasNetwork Condition for Pods'

@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 64187c5cb755f36fb7abbaa6fe008055df21f53c

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sftim

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2022
@k8s-ci-robot k8s-ci-robot merged commit 9a61519 into kubernetes:main Sep 5, 2022
@aojea
Copy link
Member

aojea commented Sep 5, 2022

per example, I just noticed that pod with hostNetwork true are not covered

@sftim
Copy link
Contributor

sftim commented Sep 6, 2022

pod with hostNetwork true are not covered

I think this is OK to publish as-is. We (blog team) could also accept a PR to tweak things, so long as @ddebroy doesn't object.

It's also OK to update after publication; for that situation, I like to mark that there's been an update.

@aojea
Copy link
Member

aojea commented Sep 6, 2022

ah, no, it was not directed to the blog post, I just realized that is a follow up on this feature, I didn't find it covered 😄

configuration by a container runtime (typically in coordination with CNI
plugins). The kubelet starts to pull container images and start individual
containers (including init containers) after the status of the `PodHasNetwork`
condition is set to `True`. Metrics collection services that report latency of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit)

Suggested change
condition is set to `True`. Metrics collection services that report latency of
condition is set to `"True"`. Metrics collection services that report latency of

Conditions are a string value used like a ternary enum.

@ddebroy
Copy link
Member Author

ddebroy commented Sep 6, 2022

ah, no, it was not directed to the blog post, I just realized that is a follow up on this feature, I didn't find it covered 😄

Thanks for pointing that out. I addressed it at kubernetes/kubernetes#111358 (comment) and will clarify this in followup PRs to the docs and blog.

@mikebrow
Copy link
Member

mikebrow commented Sep 7, 2022

ah, no, it was not directed to the blog post, I just realized that is a follow up on this feature, I didn't find it covered 😄

Thanks for pointing that out. I addressed it at kubernetes/kubernetes#111358 (comment) and will clarify this in followup PRs to the docs and blog.

SGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/blog Issues or PRs related to the Kubernetes Blog subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

8 participants