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

Fixes for attach-detach controller enablement on existing nodes #10748

Merged
merged 4 commits into from
Sep 2, 2016
Merged

Fixes for attach-detach controller enablement on existing nodes #10748

merged 4 commits into from
Sep 2, 2016

Conversation

pmorie
Copy link
Contributor

@pmorie pmorie commented Aug 31, 2016 •
edited
Loading

@pmorie
Copy link
Contributor Author

pmorie commented Aug 31, 2016

[test]

@pmorie pmorie force-pushed the attach-detach-picks branch 2 times, most recently from fcc7480 to ecda5d0 Compare August 31, 2016 21:26
@pmorie
Copy link
Contributor Author

pmorie commented Aug 31, 2016

@liggitt want to give this a look-see and ensure I haven't destroyed the patch we carried from #6310?

@smarterclayton
Copy link
Contributor

I want to know whether the upstream patch ever got applied.

@eparis
Copy link
Member

eparis commented Sep 1, 2016

it was LGTM, but flake in unit test twice.

@pmorie
Copy link
Contributor Author

pmorie commented Sep 1, 2016

@smarterclayton the upstream is LGTM, in the submit queue now.

[test]

liggitt
liggitt reviewed Sep 1, 2016
View reviewed changes
vendor/k8s.io/kubernetes/pkg/kubelet/kubelet.go
return true
}

if kl.cloud == nil {
Copy link
Contributor

@liggitt liggitt Sep 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

this is a carry we have (88abe47). We should keep the carry distinct from the upstream pick commit, since we'll drop the upstream commit in the next rebase, but keep the carry commit. I guess that means this PR needs to revert the carry, pick the upstream, then reapply the carry on top?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@liggitt I'm cool with doing it that way.

@liggitt
Copy link
Contributor

liggitt commented Sep 1, 2016

does this primarily impact nodes upgraded from previous releases? have sufficient upgrade tests been run upstream? trying to understand the potential risk and impact

@pmorie
Copy link
Contributor Author

pmorie commented Sep 1, 2016

@liggitt yes, it fixes an issue where a node upgraded from a previous release that has the configuration item set to enable controller attach/detach will not annotate the node resource to tell the controller to do the attach. So, the node will wait for the controller to do the attach, and the controller never actually does it, which breaks all attachable volumes (basically every PV type except hostpath and nfs).

@smarterclayton
Copy link
Contributor

Does a previous release have enable controller detach set?

On Thu, Sep 1, 2016 at 12:03 PM, Paul Morie [email protected]
wrote:

@liggitt https://github.com/liggitt yes, it fixes an issue where a node
upgraded from a previous release that has the configuration item set to
enable controller attach/detach will not annotate the node resource to tell
the controller to do the attach. So, the node will wait for the controller
to do the attach, and the controller never actually does it, which breaks
all attachable volumes (basically every PV type except hostpath and nfs).

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10748 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p7urFLJFyTUKxQBMIxt4mH1qkTakks5qlvdIgaJpZM4JyBeJ
.

@liggitt liggitt added this to the 1.3.0 milestone Sep 1, 2016
@liggitt liggitt self-assigned this Sep 1, 2016
@pmorie pmorie force-pushed the attach-detach-picks branch from ecda5d0 to 958b522 Compare September 1, 2016 20:07
liggitt
liggitt reviewed Sep 1, 2016
View reviewed changes
vendor/k8s.io/kubernetes/pkg/kubelet/kubelet.go
}
}

return true
Copy link
Contributor

Choose a reason for hiding this comment

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

move inside the existingNode.Spec.ExternalID == addr.Address block

@pmorie pmorie force-pushed the attach-detach-picks branch from 958b522 to 6532c6a Compare September 1, 2016 21:01
@liggitt
Copy link
Contributor

liggitt commented Sep 1, 2016

LGTM

@liggitt liggitt added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2016
@pmorie pmorie force-pushed the attach-detach-picks branch from 6532c6a to 3918d6b Compare September 1, 2016 21:17
@pmorie
UPSTREAM: revert: 88abe47: <carry>: Tolerate node ExternalID changes …
1e2d29c 
…with no cloud provider
@pmorie
UPSTREAM: 31531: Add log message in Kuelet when controller attach/det…
544fa81 
…ach is enabled
@pmorie
UPSTREAM: 31730: Make it possible to enable controller-managed attach…
a0ed8e7 
…-detach on existing nodes
@pmorie
UPSTREAM: <carry>: Tolerate node ExternalID changes with no cloud pro…
b21d368 
…vider

Previously, if the kubelet tried to register itself with the API server,
and was rejected due to the external ID changing, it would delete the
node object and recreate it.  This commit causes it to tolerate
a change in ExternalID when the ExternalID is not being provided by a
cloud provider, assuming the new ExternalID is either the node's
(metadata) name, or one of node's addresses.
@pmorie pmorie force-pushed the attach-detach-picks branch from 3918d6b to b21d368 Compare September 1, 2016 21:18
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to b21d368

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8641/)

@pmorie
Copy link
Contributor Author

pmorie commented Sep 2, 2016

@liggitt tag and bag? 🎨 👝

@liggitt
Copy link
Contributor

liggitt commented Sep 2, 2016

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to b21d368

@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 2, 2016 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8641/) (Image: devenv-rhel7_4972)

openshift-bot pushed a commit that referenced this pull request Sep 2, 2016
Merge pull request #10748 from pmorie/attach-detach-picks
d4bc571 
Merged by openshift-bot
@openshift-bot openshift-bot merged commit d4bc571 into openshift:master Sep 2, 2016
@openshift-bot openshift-bot merged commit b21d368 into openshift:master Sep 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files.
Projects
None yet
Milestone
1.3.0
Development

Successfully merging this pull request may close these issues.
5 participants
@pmorie @smarterclayton @eparis @liggitt @openshift-bot