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

expunge the word 'manifest' from Kubelet's config API #60314

Merged
merged 1 commit into from Feb 25, 2018

Conversation

@mtaufen
Contributor

mtaufen commented Feb 23, 2018

The word 'manifest' technically refers to a container-group specification
that predated the Pod abstraction. We should avoid using this legacy
terminology where possible. Fortunately, the Kubelet's config API will
be beta in 1.10 for the first time, so we still had the chance to make
this change.

I left the flags alone, since they're deprecated anyway.

I changed a few var names in files I touched too, but this PR is the
just the first shot, not the whole campaign
(git grep -i manifest | wc -l -> 1248).

Some field names in the Kubelet's now v1beta1 config API differ from the v1alpha1 API: PodManifestPath is renamed to StaticPodPath, ManifestURL is renamed to StaticPodURL, ManifestURLHeader is renamed to StaticPodURLHeader.
@mtaufen

This comment has been minimized.

Contributor

mtaufen commented Feb 23, 2018

/cc @luxas: I noticed kubeadm makes heavy use of the 'manifest' terminology, just fyi

@dashpole

I think PodPath is a little bit vague. Maybe StaticPodPath?

// podManifestPath is the path to the directory containing pod manifests to
// run, or the path to a single manifest file
PodManifestPath string
// podPath is the path to the directory containing pods to

This comment has been minimized.

@dashpole

dashpole Feb 23, 2018

Contributor

nit: add local (static) here as well?

This comment has been minimized.

@dashpole

dashpole Feb 23, 2018

Contributor

is this meant to match v1beta1?

This comment has been minimized.

@mtaufen

mtaufen Feb 23, 2018

Contributor

We shouldn't lock in to the name "Static" either.

This comment has been minimized.

@mtaufen

mtaufen Feb 23, 2018

Contributor

is this meant to match v1beta1?

yes (aside from some type differences handled during conversion)

@mtaufen

This comment has been minimized.

Contributor

mtaufen commented Feb 23, 2018

I think PodPath is a little bit vague. Maybe StaticPodPath?

We shouldn't lock in to the name "Static" either, given all the "we wish static pods could go away" discussions that have happened in the past.

@dashpole

This comment has been minimized.

Contributor

dashpole commented Feb 23, 2018

we wish static pods could go away

I thought this was just a preference for moving away from file-based pods in general. I didn't think it was an issue with the naming.

@thockin

This comment has been minimized.

Member

thockin commented Feb 23, 2018

I don't think the concept of static pods can go away.

@mtaufen

This comment has been minimized.

Contributor

mtaufen commented Feb 23, 2018

I thought this was just a preference for moving away from file-based pods in general. I didn't think it was an issue with the naming.

IIRC "static" pod is defined to be a pod that lacks certain features - e.g. it's visible in the API server but not possible to modify/manage via the API. Being specified via a file is how you get a "static" pod today, but file-based pods don't necessarily have to be "static." If we ever improve on this for pods specified via local files, I'm not sure we should keep using the word "static" to describe them.

(at least this is how I've seen it used, though technically we report these as "mirror" pods in the API server, the association is pretty strong... so maybe I'm off base and static has really meant "file" all along)

I don't think the concept of static pods can go away.

I don't think the concept of pods specified via local files can go away. I'm less sure about static pods.

@mtaufen mtaufen added the sig/node label Feb 23, 2018

@mtaufen mtaufen added this to the v1.10 milestone Feb 23, 2018

@mtaufen

This comment has been minimized.

Contributor

mtaufen commented Feb 23, 2018

Offline conversations settled on StaticPod{Path,URL,URLHeader}, since that's the established term for what happens today. Future extensions get separate names.

expunge the word 'manifest' from Kubelet's config API
The word 'manifest' technically refers to a container-group specification
that predated the Pod abstraction. We should avoid using this legacy
terminology where possible. Fortunately, the Kubelet's config API will
be beta in 1.10 for the first time, so we still had the chance to make
this change.

I left the flags alone, since they're deprecated anyway.

I changed a few var names in files I touched too, but this PR is the
just the first shot, not the whole campaign
(`git grep -i manifest | wc -l -> 1248`).
@mtaufen

This comment has been minimized.

Contributor

mtaufen commented Feb 23, 2018

/retest

@timothysc timothysc removed their request for review Feb 23, 2018

@mtaufen

This comment has been minimized.

Contributor

mtaufen commented Feb 23, 2018

/retest

@Random-Liu

This comment has been minimized.

Member

Random-Liu commented Feb 23, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 23, 2018

@dchen1107

This comment has been minimized.

Member

dchen1107 commented Feb 24, 2018

/lgtm

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 24, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, mtaufen, Random-Liu

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-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 25, 2018

Automatic merge from submit-queue (batch tested with PRs 60324, 60269, 59771, 60314, 59941). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 720c29b into kubernetes:master Feb 25, 2018

13 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation mtaufen authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment