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

provision kubelet config file for GCE instead of deprecated flags #62183

Merged
merged 1 commit into from Apr 13, 2018

Conversation

@mtaufen
Contributor

mtaufen commented Apr 5, 2018

Many Kubelet flags are now deprecated in favor of the versioned config file format. This PR adopts the versioned config file format in our cluster turn-up scripts.

cluster/kube-up.sh now provisions a Kubelet config file for GCE via the metadata server. This file is installed by the corresponding GCE init scripts.

@mtaufen mtaufen added this to the v1.11 milestone Apr 5, 2018

@mtaufen mtaufen self-assigned this Apr 5, 2018

@k8s-ci-robot k8s-ci-robot requested review from bowei and vishh Apr 5, 2018

@mtaufen mtaufen changed the title from WIP provision kubelet config file instead of deprecated flags to provision kubelet config file for GCE instead of deprecated flags Apr 9, 2018

@mtaufen mtaufen assigned maisem and unassigned mtaufen Apr 9, 2018

@mtaufen

This comment has been minimized.

Contributor

mtaufen commented Apr 9, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Apr 9, 2018

@mtaufen: GitHub didn't allow me to request PR reviews from the following users: filbranden.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @filbranden

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.

local -r dest="$1"
echo "Downloading Kubelet config file, if it exists"
# Fetch kubelet config file from GCE metadata server.
(umask 700;

This comment has been minimized.

@filbranden

filbranden Apr 9, 2018

Member

This umask is almost certainly wrong... It masks the user bits, leaving files group/other writable. Do you mean umask 077 instead?

Also recommend using nesting for the subshell. And the ; is unnecessary. Perhaps something like:

  # Fetch kubelet config file from GCE metadata server.
  (
    umask 077
    local -r ...
    ...
  )

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

I did think it was a bit odd... download-kube-env and download-kubelet-config are also using 700, and I just copied from there :P. Perhaps someone intended the permissions to max out at 700, and didn't invert?

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

Done

This comment has been minimized.

@filbranden

filbranden Apr 10, 2018

Member

I just fixed it on #62301... Will backport to the relevant releases that still have that bug.

Cheers!
Filipe

http://metadata.google.internal/computeMetadata/v1/instance/attributes/kubelet-config; then
# only write to the final location if curl succeeds
mv "${tmp_kubelet_config}" "${dest}"
elif [[ REQUIRE_METADATA_KUBELET_CONFIG_FILE == "true" ]]; then

This comment has been minimized.

@filbranden

filbranden Apr 9, 2018

Member

Use $ to access a variable 😃

Also recommend always using ${...} (for good style) and, while technically not needed right here (since [[ ... ]] built-in is smarter about that), using double quotes is always good practice too:

  elif [[ "${REQUIRE_METADATA_KUBELET_CONFIG_FILE}" == "true" ]]; then

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

Done

fi
)
}
function download-kube-master-certs {
# Fetch kube-env from GCE metadata server.
(umask 700;

This comment has been minimized.

@filbranden

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

Changed to 077

@@ -531,16 +533,98 @@ function build-node-labels {
echo $node_labels
}
# yaml-map-string-stringarray converts the encoded structure to yaml format, and echoes the result

This comment has been minimized.

@filbranden

filbranden Apr 9, 2018

Member

Name in comment doesn't match actual function name...

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

done

This comment has been minimized.

@filbranden

filbranden Apr 10, 2018

Member

Looks great!

You're taking a ${file} argument here but you don't use it anywhere. Just remove it (and shift the other two up.)

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

ya just found that too, caused a parsing bug :D

@@ -531,16 +533,98 @@ function build-node-labels {
echo $node_labels
}
# yaml-map-string-stringarray converts the encoded structure to yaml format, and echoes the result
# under the provided name. If the encoded structure is empty, echoes nothing.
# 1: item separator (e.g. ','')

This comment has been minimized.

@filbranden

filbranden Apr 9, 2018

Member

Quotes are unbalanced, you probably meant ',' only.

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

curse my editor's quote behavior

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

done

cgroupRoot: "/"
clusterDNS:
- $(yaml-quote ${DNS_SERVER_IP})
clusterDomain: $(yaml-quote ${DNS_DOMAIN})

This comment has been minimized.

@filbranden

filbranden Apr 9, 2018

Member

double quote the argument: $(yaml-quote "${DNS_DOMAIN}")

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

done

if [[ "${REGISTER_MASTER_KUBELET:-false}" == "false" ]]; then
# Note: Standalone mode is used by GKE
cat >>$file <<EOF
podCidr: $(yaml-quote ${MASTER_IP_RANGE})

This comment has been minimized.

@filbranden

filbranden Apr 9, 2018

Member

double quote the argument: $(yaml-quote "${MASTER_IP_RANGE}")

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

done

[[ "${HAIRPIN_MODE:-}" == "hairpin-veth" ]] || \
[[ "${HAIRPIN_MODE:-}" == "none" ]]; then
cat >>$file <<EOF
hairpinMode: $(yaml-quote ${HAIRPIN_MODE})

This comment has been minimized.

@filbranden

filbranden Apr 9, 2018

Member

double quote

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

done

# Note: ENABLE_MANIFEST_URL is used by GKE
if [[ "${ENABLE_MANIFEST_URL:-}" == "true" ]]; then
cat >>$file <<EOF
staticPodURL: $(yaml-quote ${MANIFEST_URL})

This comment has been minimized.

@filbranden

filbranden Apr 9, 2018

Member

double quote

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

done

cat >>$file <<EOF
staticPodURL: $(yaml-quote ${MANIFEST_URL})
EOF
write-yaml-map-string-stringarray ',' ':' staticPodURLHeader ${MANIFEST_URL_HEADER} $file

This comment has been minimized.

@filbranden

filbranden Apr 9, 2018

Member

double quote arguments... But see my comments on how I think keeping the redirect >"${file}" in here is better.

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

done w/ double quote
(stage 2)

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

done

@mtaufen

Making changes in stages. Will try echo/redirect again on my second pass.

local -r dest="$1"
echo "Downloading Kubelet config file, if it exists"
# Fetch kubelet config file from GCE metadata server.
(umask 700;

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

Done

http://metadata.google.internal/computeMetadata/v1/instance/attributes/kubelet-config; then
# only write to the final location if curl succeeds
mv "${tmp_kubelet_config}" "${dest}"
elif [[ REQUIRE_METADATA_KUBELET_CONFIG_FILE == "true" ]]; then

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

Done

fi
)
}
function download-kube-master-certs {
# Fetch kube-env from GCE metadata server.
(umask 700;

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

Changed to 077

@@ -531,16 +533,98 @@ function build-node-labels {
echo $node_labels
}
# yaml-map-string-stringarray converts the encoded structure to yaml format, and echoes the result
# under the provided name. If the encoded structure is empty, echoes nothing.
# 1: item separator (e.g. ','')

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

curse my editor's quote behavior

@@ -531,16 +533,98 @@ function build-node-labels {
echo $node_labels
}
# yaml-map-string-stringarray converts the encoded structure to yaml format, and echoes the result

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

done

done
# only output if there is a non-empty map
if [ ${#map[@]} -gt 0 ]; then
cat >>$file <<EOF

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

(stage 2)

done
# only output if there is a non-empty map
if [ ${#map[@]} -gt 0 ]; then
cat >>$file <<EOF

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

(stage 2)

(IFS=,
for v in ${map[$k]}; do
cat >>$file <<EOF
- $(yaml-quote ${v})

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

(stage 2)

local file=$2
rm -f ${file}
cat >>$file <<EOF

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

(stage 2)

cat >>$file <<EOF
staticPodURL: $(yaml-quote ${MANIFEST_URL})
EOF
write-yaml-map-string-stringarray ',' ':' staticPodURLHeader ${MANIFEST_URL_HEADER} $file

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

done w/ double quote
(stage 2)

@filbranden

Looking good! You need to remove the ${file} from the list of arguments in the functions... You're not passing it when calling them, so I think just not taking them as argument is enough already...

@@ -531,16 +533,98 @@ function build-node-labels {
echo $node_labels
}
# yaml-map-string-stringarray converts the encoded structure to yaml format, and echoes the result

This comment has been minimized.

@filbranden

filbranden Apr 10, 2018

Member

Looks great!

You're taking a ${file} argument here but you don't use it anywhere. Just remove it (and shift the other two up.)

if [[ ${#map[@]} -gt 0 ]]; then
echo "${name}:"
for k in "${!map[@]}"; do
echo " ${k}"

This comment has been minimized.

@filbranden

filbranden Apr 10, 2018

Member

echo " ${k}:" with a colon at the end?

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

nice catch, thanks

declare -a values
IFS="${item_sep}" read -ra values <<<"${map[$k]}"
for v in "${values[@]}"; do
echo " - $(yaml-quote "${v}")"

This comment has been minimized.

@filbranden

filbranden Apr 10, 2018

Member

nitpick: I'd use an assignment to avoid the nested quotes:

  declare quoted_value
  quoted_value=$(yaml-quote "${v}")
  echo "    - ${quoted_value}"

Note that this doesn't work!

  declare quoted_value=$(yaml-quote "${v}")

Because if yaml-quote fails (returns non-zero) then that gets ignored since neither errexit or pipefail is triggered... 😞

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

Thanks

This comment has been minimized.

@mtaufen

mtaufen Apr 10, 2018

Contributor

done

fi
}
# yaml-map-string-string converts the encoded structure to yaml format, and echoes the result

This comment has been minimized.

@filbranden

filbranden Apr 10, 2018

Member

Same here, ${file} should be gone...

echo "${name}:"
for k in "${!map[@]}"; do
if [[ "${quote_val_string}" == "true" ]]; then
echo " ${k}: $(yaml-quote ${map[$k]})"

This comment has been minimized.

@filbranden

filbranden Apr 10, 2018

Member

See my comment above about storing yaml-quote result on a variable...

@filbranden

This comment has been minimized.

Member

filbranden commented Apr 10, 2018

LGTM 👍

@@ -230,6 +230,7 @@ HTTP server: The kubelet can also listen for HTTP and respond to a simple API
}
// run the kubelet
glog.V(5).Infof("KubeletConfiguration: %#v", kubeletServer.KubeletConfiguration)

This comment has been minimized.

@mikedanese

mikedanese Apr 11, 2018

Member

still need this?

This comment has been minimized.

@mtaufen

mtaufen Apr 13, 2018

Contributor

I've used this in a number of debugging scenarios, I figure we might as well leave it at v=5.

@mikedanese

This comment has been minimized.

Member

mikedanese commented Apr 11, 2018

/approve

@@ -2406,6 +2407,12 @@ fi
source "${KUBE_HOME}/kube-env"
if [ -f "${KUBE_HOME}/kubelet-config.yaml" ]; then

This comment has been minimized.

@mikedanese

mikedanese Apr 11, 2018

Member

move this into start-kubelet right before you use it?

provision Kubelet config file for GCE
This PR extends the client-side startup scripts to provision a Kubelet
config file instead of legacy flags. This PR also extends the
master/node init scripts to install this config file from the GCE
metadata server, and provide the --config argument to the Kubelet.

@mtaufen mtaufen added this to the v1.11 milestone Apr 13, 2018

@mtaufen

This comment has been minimized.

Contributor

mtaufen commented Apr 13, 2018

/status approved-for-milestone

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Apr 13, 2018

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@maisem @mikedanese @mtaufen

Pull Request Labels
  • sig/cluster-lifecycle sig/node: Pull Request will be escalated to these SIGs if needed.
  • priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
  • kind/feature: New functionality.
Help
@mikedanese

This comment has been minimized.

Member

mikedanese commented Apr 13, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 13, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Apr 13, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikedanese, mtaufen

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 Apr 13, 2018

Automatic merge from submit-queue (batch tested with PRs 62486, 62471, 62183). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit a5f2655 into kubernetes:master Apr 13, 2018

15 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 Job succeeded.
Details
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck 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