Fix setting resources in fluentd-gcp plugin #55950

crassirostris · 2017-11-17T16:31:08Z

Currently if some of the variables are not set, scripts prints error, which is not critical, since the function is executed in a separate process, but it leads to the wrong resulting values

fluentd-gcp addon: Fix fluentd deployment on GCP when custom resources are set.

/cc @piosz @x13n
/assign @roberthbailey @mikedanese
Could you please approve?

mikedanese · 2017-11-17T18:16:36Z

/approve no-issue

I can't review until later.

x13n · 2017-11-20T08:48:10Z

cluster/gce/gci/configure-helper.sh

+      --containers=fluentd-gcp -o yaml > ${temp_fluentd_gcp_yaml}; then
+    mv ${temp_fluentd_gcp_yaml} ${fluentd_gcp_yaml}
+  else
+    rm ${temp_fluentd_gcp_yaml}


Maybe keep this file for debugging purposes? Also, how about logging to stderr that setting fluentd resources failed?

Maybe keep this file for debugging purposes?

I don't want to leave this file in the same directory, to avoid addon-manager picking it up accidentally

Also, how about logging to stderr that setting fluentd resources failed?

Makes sense, done

x13n · 2017-11-20T08:52:20Z

cluster/gce/gci/configure-helper.sh

+    if [[ -n "${request_resources}" ]]; then
+      request_resources="${request_resources},"
+    fi
+    request_resources="memory=${FLUENTD_GCP_MEMORY_LIMIT}"


Shouldn't this be REQUEST?

Done, thanks

x13n · 2017-11-20T08:52:56Z

cluster/gce/gci/configure-helper.sh

+  fi
+  local request_resources=""
+  if [[ -n "${FLUENTD_GCP_CPU_REQUEST:-}" ]]; then
+    request_resources="cpu=${FLUENTD_GCP_MEMORY_LIMIT}"


FLUENTD_GCP_CPU_REQUEST?

Done, thanks

crassirostris · 2017-11-22T10:53:07Z

@x13n Thanks! PTAL

x13n · 2017-11-22T11:04:35Z

cluster/gce/gci/configure-helper.sh

+    fi
+    request_resources="memory=${FLUENTD_GCP_MEMORY_REQUEST}"
+  fi
+  if [[ -n "${request_resources}" ]]; then


This will be always set to true, since this var is set in line 1951. Maybe use -z instead of -n? Same in similar condition above.

No, [[ -n "" ]] evaluates to false

Hm, you're right :-)

x13n · 2017-11-22T11:07:13Z

cluster/gce/gci/configure-helper.sh

+      --containers=fluentd-gcp -o yaml > ${temp_fluentd_gcp_yaml}; then
+    mv ${temp_fluentd_gcp_yaml} ${fluentd_gcp_yaml}
+  else
+    echo "Failed to update fluentd resources"


Maybe add ">&2" to use stderr instead of stdout? Also, if the yaml file is going to be deleted, maybe just print its contents as well? It will be easier to figure out what went wrong if this fails.

Signed-off-by: Mik Vyatskov <vmik@google.com>

x13n · 2017-11-22T12:13:32Z

/lgtm

k8s-github-robot · 2017-11-22T12:13:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crassirostris, mikedanese, x13n

Associated issue requirement bypassed by: mikedanese

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~cluster/addons/fluentd-gcp/OWNERS~~ [crassirostris,mikedanese]
~~cluster/gce/OWNERS~~ [mikedanese]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-11-22T12:13:57Z

[MILESTONENOTIFIER] Milestone Pull Request Current

@crassirostris @mikedanese @roberthbailey @x13n

Note: This pull request is marked as priority/critical-urgent, and must be updated every 3 days during code slush.

Example update:

ACK.  In progress
ETA: DD/MM/YYYY
Risks: Complicated fix required

Pull Request Labels

sig/instrumentation: Pull Request will be escalated to these SIGs if needed.
priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
kind/bug: Fixes a bug discovered during the current release.

Help

k8s-github-robot · 2017-11-22T13:21:24Z

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

…-#55950-upstream-release-1.8 Automatic merge from submit-queue. Automated cherry pick of #55950 Cherry pick of #55950 on release-1.8. #55950: Fix setting resources in fluentd-gcp plugin

crassirostris added area/logging kind/bug Categorizes issue or PR as related to a bug. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. labels Nov 17, 2017

k8s-ci-robot assigned mikedanese and roberthbailey Nov 17, 2017

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 17, 2017

k8s-ci-robot requested a review from piosz November 17, 2017 16:31

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 17, 2017

crassirostris requested review from cheftako, piosz and x13n and removed request for piosz and cheftako November 17, 2017 16:31

crassirostris force-pushed the fluentd-gcp-setting-resources-fix branch from dd9096b to 9da2d25 Compare November 17, 2017 17:28

abgworrall mentioned this pull request Nov 17, 2017

[job failure] gci-gke #55189

Closed

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 17, 2017

x13n reviewed Nov 20, 2017

View reviewed changes

crassirostris added this to the v1.9 milestone Nov 22, 2017

k8s-github-robot added the milestone/incomplete-labels label Nov 22, 2017

crassirostris force-pushed the fluentd-gcp-setting-resources-fix branch from 9da2d25 to 375cef1 Compare November 22, 2017 10:51

crassirostris added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. status/approved-for-milestone labels Nov 22, 2017

k8s-github-robot added milestone/needs-attention and removed milestone/incomplete-labels labels Nov 22, 2017

crassirostris added the status/in-progress label Nov 22, 2017

k8s-github-robot removed the milestone/needs-attention label Nov 22, 2017

x13n reviewed Nov 22, 2017

View reviewed changes

Fix setting resources in fluentd-gcp plugin

e9322b9

Signed-off-by: Mik Vyatskov <vmik@google.com>

crassirostris force-pushed the fluentd-gcp-setting-resources-fix branch from 375cef1 to e9322b9 Compare November 22, 2017 11:41

k8s-ci-robot assigned x13n Nov 22, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 22, 2017

k8s-github-robot merged commit f85649c into kubernetes:master Nov 22, 2017

crassirostris mentioned this pull request Nov 24, 2017

Automated cherry pick of #55950 #56342

Merged

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Nov 24, 2017

crassirostris mentioned this pull request Feb 13, 2018

Enable scaling fluentd-gcp resources using ScalingPolicy. #59657

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix setting resources in fluentd-gcp plugin #55950

Fix setting resources in fluentd-gcp plugin #55950

crassirostris commented Nov 17, 2017 •

edited by jpbetz

Loading

mikedanese commented Nov 17, 2017

x13n Nov 20, 2017

crassirostris Nov 22, 2017

x13n Nov 20, 2017

crassirostris Nov 22, 2017

x13n Nov 20, 2017

crassirostris Nov 22, 2017

crassirostris commented Nov 22, 2017

x13n Nov 22, 2017

crassirostris Nov 22, 2017

x13n Nov 22, 2017

x13n Nov 22, 2017 •

edited

Loading

crassirostris Nov 22, 2017

x13n commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

Fix setting resources in fluentd-gcp plugin #55950

Fix setting resources in fluentd-gcp plugin #55950

Conversation

crassirostris commented Nov 17, 2017 • edited by jpbetz Loading

mikedanese commented Nov 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crassirostris commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

x13n Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

x13n commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

crassirostris commented Nov 17, 2017 •

edited by jpbetz

Loading

x13n Nov 22, 2017 •

edited

Loading