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.

Sign up for GitHub

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

Jump to bottom

[fluentd-gcp addon] Update to use Stackdriver Agent image. #59128

Merged
merged 1 commit into from
Feb 27, 2018
Merged

[fluentd-gcp addon] Update to use Stackdriver Agent image. #59128

merged 1 commit into from
Feb 27, 2018

Conversation

bmoyles0117
Copy link
Contributor

@bmoyles0117 bmoyles0117 commented Jan 31, 2018 •
edited by crassirostris
Loading

Update the fluentd DaemonSet to use the Stackdriver Logging Agent container image.
The Stackdriver Logging Agent container image uses fluentd v0.14.25.
We add a special label to each log record as a signal to logging backends to handle both new and legacy resource types.

Release note:

[fluentd-gcp addon] Switch to the image, provided by Stackdriver.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 31, 2018
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jan 31, 2018
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 9, 2018
@crassirostris
Copy link

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 14, 2018
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 14, 2018
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2018
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 17, 2018
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2018
@crassirostris
Copy link

Well, I'm happy to merge it

@x13n @piosz FYI

Just a couple of things left to do:

  • Squash commits into one
  • Add a meaningful description in the PR. Something that says explicitly what's being replaced and why
  • Add a release note, something like [fluentd-gcp addon] Switch to the image, provided by Stackdriver.
  • Change the version in the DaemonSet manifest file to something like v3.0.0. I mean all of the occurrences of v2.0.16. Also, there's a file nearby, scaler-deployment.yaml, you should change the version there also

@x13n
Copy link
Member

x13n commented Feb 19, 2018

Does it use roughly the same amount of cpu and memory as fluentd-gcp?

@crassirostris
Copy link

@x13n Yup, Bryan tested it and it showed roughly similar resource consumption under load

@bmoyles0117
Copy link
Contributor Author

Hold off on approving for now, the image being used here is not an official release. Waiting for an upstream bug to be resolved before we're able to build an official release.

@crassirostris crassirostris added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 20, 2018
@crassirostris
Copy link

@bmoyles0117 Sure. You still can do the things I outlined meanwhile

@crassirostris crassirostris self-assigned this Feb 22, 2018
@x13n x13n mentioned this pull request Feb 23, 2018
Closed
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 25, 2018
@bmoyles0117
Copy link
Contributor Author

Are we able to merge despite these tests failing?

@serathius
Copy link
Contributor

I don't thinks so :(
Looks like pull-kubernetes-verify is broken by hack/verify-bazel.sh we need to wait for fix.

@bmoyles0117
Copy link
Contributor Author

When you say wait for a fix, what is it that we're waiting for, and who is able to fix it?

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 26, 2018
@gmarek
Copy link
Contributor

gmarek commented Feb 26, 2018

/approve no-issue

crassirostris
crassirostris reviewed Feb 26, 2018
View reviewed changes
cluster/gce/gci/configure-helper.sh Outdated
@@ -2244,6 +2244,7 @@ EOF
local -r fluentd_gcp_yaml="${dst_dir}/fluentd-gcp/fluentd-gcp-ds.yaml"
local -r fluentd_gcp_configmap_yaml="${dst_dir}/fluentd-gcp/fluentd-gcp-configmap.yaml"
update-event-exporter ${event_exporter_yaml}
sed -i -e "s@{{ fluentd_gcp_version }}@${FLUENTD_GCP_VERSION}@g" "${fluentd_gcp_yaml}"
Copy link

Choose a reason for hiding this comment

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

I think there's a bug. If FLUENTD_GCP_VERSION is not registered (and it's not registered e.g. in GKE), script will crash with unbounded value

${FLUENTD_GCP_VERSION} -> ${FLUENTD_GCP_VERSION:-<some-default>}

@crassirostris crassirostris removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 26, 2018
@crassirostris
Copy link

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 26, 2018
@bmoyles0117
Update to use Stackdriver Agent image.
84a86cf 
Prometheus is enabled by default.
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2018
@serathius
Copy link
Contributor

Fix for pull-kubernetes-verify #60442

@bmoyles0117 bmoyles0117 changed the title Update to use Stackdriver Agent image. [fluentd-gcp addon] Update to use Stackdriver Agent image. Feb 26, 2018
@piosz piosz self-assigned this Feb 26, 2018
@piosz
Copy link
Member

piosz commented Feb 26, 2018

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bmoyles0117, crassirostris, gmarek, piosz

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

@BenTheElder
Copy link
Member

fix PR for verify merged
/test pull-kubernetes-verify

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@ixdy
Copy link
Member

ixdy commented Feb 27, 2018

/retest

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

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

@k8s-github-robot k8s-github-robot merged commit 7480fac into kubernetes:master Feb 27, 2018
@yujuhong yujuhong mentioned this pull request Mar 8, 2018
Closed
@x13n x13n mentioned this pull request Mar 16, 2018
Merged
@qingling128 qingling128 mentioned this pull request Apr 3, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crassirostris crassirostris crassirostris left review comments

@x13n x13n x13n left review comments

@igorpeshansky igorpeshansky igorpeshansky approved these changes

@qingling128 qingling128 qingling128 approved these changes

@piosz piosz Awaiting requested review from piosz

Assignees

@crassirostris crassirostris

@piosz piosz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.