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 dependencies: aws-sdk-go to 1.6.10; also cadvisor #40095

Merged
merged 3 commits into from
Jan 18, 2017

Conversation

dashpole
Copy link
Contributor

updating cadvisor mainly to include this bugfix #1558

Because cadvisor vendors a newer version of aws than kubernetes, the aws dependency needed to be updated as well.
cc: @justinsb @zmerlynn @timstclair

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 18, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Jenkins Bazel Build failed for commit 9737b41. Full PR test history. cc @dashpole

The magic incantation to run this job again is @k8s-bot bazel test this. Please help us cut down flakes by linking to an open flake 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 k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Jan 18, 2017
@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit ca23355. Full PR test history. cc @dashpole

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake 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-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit ca23355. Full PR test history. cc @dashpole

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake 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-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit ca23355. Full PR test history. cc @dashpole

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake 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-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit ca23355. Full PR test history. cc @dashpole

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake 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.

@timstclair
Copy link

I think we should consider splitting the aws update into a separate PR. Also, the cAdvisor update to the AWS API didn't change cAdvisor code at all, so I think you should be able to update cAdvisor without the AWS dependency.

@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit ca23355. Full PR test history. cc @dashpole

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake 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-ci-robot
Copy link
Contributor

Jenkins verification failed for commit 9737b41. Full PR test history. cc @dashpole

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake 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.

@justinsb
Copy link
Member

I don't know how you got it to go green, but I'm not going to look a magic gift horse in the mouth.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 18, 2017
@dashpole
Copy link
Contributor Author

Oh, its going to remove the lgtm because I dont have a release note

@k8s-github-robot k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jan 18, 2017
@justinsb justinsb changed the title updated cadvisor to latest version; updated aws dependency to same as cadvisor Update dependencies: aws-sdk-go to 1.6.10; also cadvisor Jan 18, 2017
@justinsb justinsb added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Jan 18, 2017
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit a739333. Full PR test history. cc @dashpole

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake 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-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit a739333. Full PR test history. cc @dashpole

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake 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-ci-robot
Copy link
Contributor

Jenkins kops AWS e2e failed for commit a739333. Full PR test history. cc @dashpole

The magic incantation to run this job again is @k8s-bot kops aws e2e test this. Please help us cut down flakes by linking to an open flake 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.

@dashpole
Copy link
Contributor Author

@k8s-bot cvm gke e2e test this
@k8s-bot gci gke e2e test this
@k8s-bot kops aws e2e test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 40105, 40095)

@k8s-github-robot k8s-github-robot merged commit a768f15 into kubernetes:master Jan 18, 2017
@k8s-ci-robot
Copy link
Contributor

Jenkins CRI GCE e2e failed for commit a739333. Full PR test history. cc @dashpole

The magic incantation to run this job again is @k8s-bot cri e2e test this. Please help us cut down flakes by linking to an open flake 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.

@dashpole dashpole deleted the update_godeps branch January 27, 2017 17:20
@dashpole
Copy link
Contributor Author

@calebamiles this is the PR that updated the cAdvisor dependency. However, IIRC, this introduced problems with the AWS builds, and may not be a clean cherrypick. @justinb, is this safe to cherrypick into 1.4?

@calebamiles
Copy link
Contributor

Thanks @dashpole, @justinsb would this be ok to cherry pick onto the {1.4, 1.5} release branches?

cc: @euank, @saad-ali, @jessfraz

@justinsb
Copy link
Member

Updating aws-sdk-go should be fine. I don't believe we had any genuine problems, just problems with godeps.

I presume the motivation is the cadvisor fixes?

@dashpole
Copy link
Contributor Author

@justinsb yes, specifically a fix that prevents it from leaking goroutines if the user doesnt have the "find" command.

@jessfraz
Copy link
Contributor

jessfraz commented Feb 1, 2017

whoa this is pretty big is it necessary for 1.4

@jessfraz
Copy link
Contributor

jessfraz commented Feb 1, 2017

updating the deps in the release branches is not exactly simple/fun, is this fixing a specific bug

@dashpole
Copy link
Contributor Author

dashpole commented Feb 1, 2017

This fixes cadvisor #1558. I am not quite sure what constitutes a "neccessary" bug fix. This issue is pretty major for those experiencing it, as it leaks goroutines if the image doesnt have GNU find.

@jessfraz
Copy link
Contributor

jessfraz commented Feb 1, 2017

is something shelling out to find?

@dashpole
Copy link
Contributor Author

dashpole commented Feb 1, 2017

Yes, the way that we estimate inode usage inside a container's writeable layer uses the find command.

@jessfraz
Copy link
Contributor

jessfraz commented Feb 1, 2017

ok I guess we can cherrypick, thanks for the explanation, i might send a PR to cadvisor :)

@dashpole
Copy link
Contributor Author

dashpole commented Feb 1, 2017

If you are curious, we are considering doing this in pure go instead of using find. google/cadvisor#1576

@jessfraz
Copy link
Contributor

jessfraz commented Feb 1, 2017 via email

@dashpole
Copy link
Contributor Author

@jessfraz any idea if this was cherrypicked?

k8s-github-robot pushed a commit that referenced this pull request Mar 22, 2017
Automatic merge from submit-queue

[Release 1.5] Update cadvisor godeps to v0.25.0

This PR updates the cAdvisor Godeps for the 1.5 branch.  This includes a number of critical bugfixes including:
[cadvisor#1558](google/cadvisor#1558), [cadvisor#1573](google/cadvisor#1573)

This is a large change on account of the aws dependency update.  However, many affected users have requested that this be cherrypicked into 1.5 and 1.4 (coming soon)

```release-note
- Disable thin_ls due to excessive iops
- Ignore .mount cgroups, fixing dissappearing stats
- Fix wc goroutine leak
- Update aws-sdk-go dependency to 1.6.10
```

cc @calebamiles [remember this?](#40095 (comment)) @jessfraz @timstclair
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants