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

Update cadvisor to v0.29.1 #60867

Merged
merged 1 commit into from
Mar 8, 2018
Merged

Update cadvisor to v0.29.1 #60867

merged 1 commit into from
Mar 8, 2018

Conversation

Random-Liu
Copy link
Member

Update cadvisor to v0.29.1 to include a bug fix for containerd integration. google/cadvisor#1894

Release note:

none

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 7, 2018
@Random-Liu Random-Liu added this to the v1.10 milestone Mar 7, 2018
@Random-Liu Random-Liu added cherrypick-candidate area/cadvisor sig/node Categorizes an issue or PR as relevant to SIG Node. kind/bug Categorizes issue or PR as related to a bug. and removed milestone/incomplete-labels cherrypick-candidate area/cadvisor labels Mar 7, 2018
@Random-Liu Random-Liu added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Mar 7, 2018
@dashpole
Copy link
Contributor

dashpole commented Mar 7, 2018

/lgtm
/retest

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

dashpole commented Mar 7, 2018

/priority critical-urgent

@k8s-ci-robot k8s-ci-robot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Mar 7, 2018
@dashpole dashpole removed the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Mar 7, 2018
@Random-Liu
Copy link
Member Author

Random-Liu commented Mar 7, 2018 •
edited
Loading

/cc @thockin for approval of the Godep change. Thanks!

@dims
Copy link
Member

dims commented Mar 7, 2018

cc @cblecker

@dashpole
Copy link
Contributor

dashpole commented Mar 7, 2018

It is worth noting that this only affects the containerd integration, so it is very low risk for the release.

cblecker
cblecker requested changes Mar 7, 2018
View reviewed changes
Copy link
Member

@cblecker cblecker left a comment

Choose a reason for hiding this comment

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

/lgtm cancel
/hold

Removing status/approved-for-milestone until the release team reviews. We have to be careful during code freeze.

@Random-Liu: Can you please review the comments below, as well as explain what is the risk to containerd installs if this doesn't make the release? This doesn't appear to be a very risky change, but we need to understand the pros/cons

cc: @dchen1107 @derekwaynecarr @jdumars

Godeps/Godeps.json Outdated
@@ -1,6 +1,6 @@
{
"ImportPath": "k8s.io/kubernetes",
"GoVersion": "go1.9",
"GoVersion": "go1.10",
Copy link
Member

Choose a reason for hiding this comment

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

This should not be changed.

Copy link
Member Author

@Random-Liu Random-Liu Mar 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, forgot that I switched to golang 1.10. Will update.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Godeps/Godeps.json Outdated
@@ -462,47 +462,47 @@
},
{
"ImportPath": "github.com/containerd/containerd/api/services/containers/v1",
"Comment": "v1.0.0-beta.2-159-g27d450a0",
"Comment": "v1.0.0-beta.2-159-g27d450a",
Copy link
Member

Choose a reason for hiding this comment

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

Please upgrade your git version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@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. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 7, 2018
@jdumars
Copy link
Member

jdumars commented Mar 7, 2018

Yes, I'd love to know more risk context here. Thanks!

@Random-Liu
Copy link
Member Author

Random-Liu commented Mar 7, 2018 •
edited
Loading

Can you please review the comments below, as well as explain what is the risk to containerd installs if this doesn't make the release?

Why we need this change?

If this doesn't make the release, users will sometimes see partial container monitoring stats for Kubernetes+containerd integration, because of the race condition described in google/cadvisor#1894 (comment).

This doesn't appear to be a very risky change, but we need to understand the pros/cons

What is the risk?

  1. This change is only specific to containerd integration with cadvisor. The code path won't run for Docker and other container runtimes, so this won't be risky for other container runtimes.
  2. For containerd integration, this only added a fixed retry loop (5 times) around an existing logic to avoid the known race condition.
    a. The behavior in normal case is exactly the same with before.
    b. The behavior in error case is:
    • If one of the retries succeeds, it recovers to the original behavior.
    • If all retries fail, it fall back to the original error handling with ~1.5s delay. Since it is already the error case and should also be very rare, I don't think it make too much a difference.

@cblecker @jdumars Given so, I think this change is a low risky change which fixes a bug for users of Kubernetes+containerd. :)

Thanks for asking for the risk context!

cblecker
cblecker approved these changes Mar 8, 2018
View reviewed changes
Copy link
Member

@cblecker cblecker left a comment

Choose a reason for hiding this comment

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

Thanks for the information, @Random-Liu!

From a bumping godep perspective, this lgtm. From the side of inclusion in the 1.10 release, based on the information provided, I think this is a low-risk bug fix. I can't speak authoritatively for this area, however. If sig-node agrees, then I'd support inclusion in 1.10.

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 8, 2018
@cblecker
Copy link
Member

cblecker commented Mar 8, 2018

/hold cancel
Withdrawing my hold :)

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 8, 2018
@jdumars
Copy link
Member

jdumars commented Mar 8, 2018

I'm adding approved for milestone but someone else needs to LGTM this.

@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@Random-Liu @cblecker @dashpole

Pull Request Labels
  • sig/node: 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

@dims
Copy link
Member

dims commented Mar 8, 2018

@jdumars from sig-node? it does LGTM 👍 to me

@dashpole
Copy link
Contributor

dashpole commented Mar 8, 2018

/lgtm
as well. Seems like we have consensus.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2018
@cblecker
Copy link
Member

cblecker commented Mar 8, 2018

based on the previous lgtm from @dashpole, the current lgtm from @dims, and the risk information provided:
/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, dashpole, Random-Liu

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

@jdumars
Copy link
Member

jdumars commented Mar 8, 2018

Nice work everyone!

@k8s-github-robot
Copy link

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

@dashpole
Copy link
Contributor

dashpole commented Mar 8, 2018

/retest

@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 bcfdb39 into kubernetes:master Mar 8, 2018
@Random-Liu Random-Liu deleted the update-cadvisor branch March 12, 2018 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cblecker cblecker cblecker approved these changes

@brendandburns brendandburns Awaiting requested review from brendandburns

@dchen1107 dchen1107 Awaiting requested review from dchen1107

Assignees

@cblecker cblecker

@dashpole dashpole

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. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.10
Development

Successfully merging this pull request may close these issues.
7 participants
@Random-Liu @dashpole @dims @jdumars @cblecker @k8s-github-robot @k8s-ci-robot