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.

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

Update the stackdriver agents yaml to include a deployment for cluster level resources #62043

Conversation

supriyagarg
Copy link
Contributor

@supriyagarg supriyagarg commented Apr 2, 2018

What this PR does / why we need it: This PR introduces a Deployment based on the Stackdriver Metadata Agent, that will allow users to collect cluster level metadata like unscheduled pods, and services. All the services in a cluster are written against the "k8s_cluster" resource.

Extend the Stackdriver Metadata Agent by adding a new Deployment for ingesting unscheduled pods, and services.  

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 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. labels Apr 2, 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 cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 2, 2018
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 2, 2018
@zouyee
Copy link
Member

zouyee commented Apr 3, 2018

/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 Apr 3, 2018
@@ -30,6 +30,8 @@ spec:
- image: gcr.io/stackdriver-agents/stackdriver-metadata-agent:{{ metadata_agent_version }}
imagePullPolicy: IfNotPresent
name: metadata-agent
command: ["/opt/stackdriver/metadata/sbin/metadatad"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use list syntax consistent with the rest of the file (each element in separate line preceded with a dash).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Merged the args into the command list.

@@ -2156,11 +2156,15 @@ EOF
[[ "${METADATA_AGENT_VERSION:-}" != "" ]]; then
metadata_agent_cpu_request="${METADATA_AGENT_CPU_REQUEST:-40m}"
metadata_agent_memory_request="${METADATA_AGENT_MEMORY_REQUEST:-50Mi}"
metadata_agent_cluster_level_cpu_request="${METADATA_AGENT_CLUSTER_LEVEL_CPU_REQUEST:-40m}"
Copy link
Contributor

Choose a reason for hiding this comment

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

@supriyagarg @x13n
Do we have any more up-to-date numbers, so that we can set reasonable defaults?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have any up-to-date numbers. Let me check with others in the agents team.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not block this PR, but set reasonable defaults separately

Copy link
Member

Choose a reason for hiding this comment

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

This is way too high to be acceptable (but the same goes for the DaemonSet one).

Copy link
Contributor Author

@supriyagarg supriyagarg left a comment

Choose a reason for hiding this comment

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

Responded to comments, and udpated implementation to use a ConfigMap

@@ -30,6 +30,8 @@ spec:
- image: gcr.io/stackdriver-agents/stackdriver-metadata-agent:{{ metadata_agent_version }}
imagePullPolicy: IfNotPresent
name: metadata-agent
command: ["/opt/stackdriver/metadata/sbin/metadatad"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Merged the args into the command list.

@@ -2156,11 +2156,15 @@ EOF
[[ "${METADATA_AGENT_VERSION:-}" != "" ]]; then
metadata_agent_cpu_request="${METADATA_AGENT_CPU_REQUEST:-40m}"
metadata_agent_memory_request="${METADATA_AGENT_MEMORY_REQUEST:-50Mi}"
metadata_agent_cluster_level_cpu_request="${METADATA_AGENT_CLUSTER_LEVEL_CPU_REQUEST:-40m}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have any up-to-date numbers. Let me check with others in the agents team.

@kawych
Copy link
Contributor

kawych commented Apr 4, 2018

LGTM, please squash the commits.

@supriyagarg supriyagarg force-pushed the metadata-agent-cluster-deployment branch from 69c71ac to 1ad414d Compare April 4, 2018 20:53
@supriyagarg
Copy link
Contributor Author

Thanks - bumped the version to 0.18.2 and squashed the commits.

@supriyagarg
Copy link
Contributor Author

/retest

@supriyagarg supriyagarg force-pushed the metadata-agent-cluster-deployment branch from 1ad414d to 526daf0 Compare April 4, 2018 23:31
@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 Apr 4, 2018
@supriyagarg supriyagarg force-pushed the metadata-agent-cluster-deployment branch from 526daf0 to ed65b0d Compare April 4, 2018 23:33
@supriyagarg
Copy link
Contributor Author

supriyagarg commented Apr 4, 2018

Update: bumped the version to 0.19.1, and added a liveness probes to both the daemonset and the deployment

@supriyagarg
Copy link
Contributor Author

/retest

@kawych
Copy link
Contributor

kawych commented Apr 5, 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 Apr 5, 2018
@supriyagarg
Copy link
Contributor Author

/assign @MaciekPytel

@MaciekPytel
Copy link
Contributor

@supriyagarg Shouldn't this have a release note (especially if you're planning to cherry-pick 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 release-note-none Denotes a PR that doesn't merit a release note. labels Apr 5, 2018
@MaciekPytel
Copy link
Contributor

/approve
/hold

Please remove hold after you address comments by @x13n.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 5, 2018
@supriyagarg supriyagarg force-pushed the metadata-agent-cluster-deployment branch from ed65b0d to d44c1e9 Compare April 5, 2018 13:25
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 5, 2018
@supriyagarg
Copy link
Contributor Author

Just fixed a typo in the env vars: CLUSETER -> CLUSTER in one place.
The metadata agent env variables are not provider specific - do they still need to be added to PROVIDER_VARS to be used?

@x13n
Copy link
Member

x13n commented Apr 5, 2018

@x13n
Copy link
Member

x13n commented Apr 5, 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 Apr 5, 2018
@supriyagarg supriyagarg force-pushed the metadata-agent-cluster-deployment branch from b0b8634 to e350c46 Compare April 5, 2018 14:10
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 5, 2018
@supriyagarg
Copy link
Contributor Author

Squashed the commits.

@kawych
Copy link
Contributor

kawych commented Apr 5, 2018

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Apr 5, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kawych, MaciekPytel, supriyagarg, x13n

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

@supriyagarg
Copy link
Contributor Author

/retest

1 similar comment
@supriyagarg
Copy link
Contributor Author

/retest

@k8s-ci-robot
Copy link
Contributor

@supriyagarg: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce-100-performance ed65b0df3fbea4f1a1a3f410ce7f6dd10bbac48a link /test pull-kubernetes-e2e-gce-100-performance
pull-kubernetes-e2e-gke e350c46 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 (batch tested with PRs 62043, 62168). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 405ea2b into kubernetes:master Apr 5, 2018
@supriyagarg supriyagarg deleted the metadata-agent-cluster-deployment branch April 5, 2018 16:40
k8s-github-robot pushed a commit that referenced this pull request Apr 6, 2018
…62043-upstream-release-1.10

Automatic merge from submit-queue.

Automated cherry pick of #62043: Update the stackdriver agents yaml to include a deployment

Cherry pick of #62043 on release-1.10.

#62043: Update the stackdriver agents yaml to include a deployment
@@ -7,6 +7,19 @@ metadata:
kubernetes.io/cluster-service: "true"
addonmanager.kubernetes.io/mode: Reconcile
---
apiVersion: v1
kind: ConfigMap
metadata:
Copy link
Contributor

Choose a reason for hiding this comment

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

This ConfigMap is missing labels for addon manager:
kubernetes.io/cluster-service: "true"
addonmanager.kubernetes.io/mode: Reconcile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

7 participants