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

Use the logging agent's node name as the metadata agent URL. #63353

Merged
merged 1 commit into from
May 8, 2018
Merged

Use the logging agent's node name as the metadata agent URL. #63353

merged 1 commit into from
May 8, 2018

Conversation

bmoyles0117
Copy link
Contributor

@bmoyles0117 bmoyles0117 commented May 2, 2018 •
edited
Loading

The Stackdriver Logging agent should use the node's hostname when it constructs the Stackdriver Metadata Agent's URL, currently, it's using the GKE Master's hostname, which is a bug.

Release note:

[fluentd-gcp addon] Use the logging agent's node name as the metadata agent URL.

@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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. 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 May 2, 2018
@bmoyles0117
Copy link
Contributor Author

@kawych @piosz @rphillips can someone please review and add the ok-to-test label?

kawych
kawych suggested changes May 3, 2018
View reviewed changes
cluster/addons/fluentd-gcp/fluentd-gcp-ds.yaml
- name: NODE_NAME
valueFrom:
fieldRef:
apiVersion: v1
fieldPath: spec.nodeName
- name: STACKDRIVER_METADATA_AGENT_URL
value: http://$(NODE_NAME):8799
Copy link
Contributor

Choose a reason for hiding this comment

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

$(NODE_NAME) is not visible here. Please generate the url in configure-helper.sh and substitute it with sed as it is done now. Please make sure to test that it works as intended before submitting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Node name is visible here via Kubernetes bash variable substitution. This is how we're doing it in our manual installation instructions, without the use of bash sed replacing. You can see the configuration as we apply it verbatim here in our public config: https://storage.googleapis.com/stackdriver-gke-beta/configs/logging-agent.yaml

I can try to find the official documentation for this feature, but it has been tested and validated by our team.

@kawych
Copy link
Contributor

kawych commented May 3, 2018

/ok-to-test
Are you going to cherry-pick it to 1.10 branch too?

@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 May 3, 2018
@bmoyles0117
Copy link
Contributor Author

Yes, we will cherry pick this into 1.10 if we can get it approved in time.

@kawych
Copy link
Contributor

kawych commented May 3, 2018

/lgtm

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

/retest

@piosz
Copy link
Member

piosz commented May 8, 2018

/approve no-issue

@bmoyles0117
Copy link
Contributor Author

/assign @jszczepkowski

@bmoyles0117
Copy link
Contributor Author

/retest

@piosz piosz assigned mwielgus and unassigned jszczepkowski May 8, 2018
@mwielgus
Copy link
Contributor

mwielgus commented May 8, 2018

/approve no-issue

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bmoyles0117, kawych, mwielgus, 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 8, 2018
@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.

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 8, 2018 •
edited
Loading

@bmoyles0117: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gke a0a7686 link /test pull-kubernetes-e2e-gke

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit a743392 into kubernetes:master May 8, 2018
@bmoyles0117 bmoyles0117 mentioned this pull request May 9, 2018
Merged
k8s-github-robot pushed a commit that referenced this pull request May 16, 2018
Merge pull request #63605 from bmoyles0117/automated-cherry-pick-of-#…
9dee819 
…63353-upstream-release-1.10

Automatic merge from submit-queue.

Automated cherry pick of #63353: Use the logging agent's node name as the metadata agent URL.

Cherry pick of #63353 on release-1.10.

#63353: Use the logging agent's node name as the metadata agent URL.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kawych kawych kawych requested changes

Assignees

@kawych kawych

@mwielgus mwielgus

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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@bmoyles0117 @kawych @piosz @mwielgus @k8s-ci-robot @fejta-bot @k8s-github-robot @jszczepkowski