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

Node should default to controller attach detach #12726

Merged

Conversation

smarterclayton
Copy link
Contributor

This should have been changed in 1.4, and I don't see anything in the ansible setup that would set it to true. This means that 1.4 installs are probably running with both kubelet attach/detach and controller attach/detach...

@derekwaynecarr

[test]

This should have been changed in 1.4
@smarterclayton
Copy link
Contributor Author

@sjenning who owns attach detach controller? We need to check that 1.4 isn't broken.

@eparis
Copy link
Member

eparis commented Jan 31, 2017

@smarterclayton why would you ask Derek or Seth? Derek, Seth please ignore.

@gnufied can you run this down today? I was about 90% sure we had switched everything to master attach, but maybe I'm wrong (wouldn't be the first time today)

@eparis
Copy link
Member

eparis commented Jan 31, 2017

@jsafrane @childsb

@smarterclayton
Copy link
Contributor Author

I asked because Seth was debugging master attach issues the other day.

@jsafrane I don't see signs that this is correctly set up in 1.4 from either ansible or openshift code - anything you can do to triage that asap in case we have a significant issues for users and customers on 1.4 clusters is appreciated.

@smarterclayton
Copy link
Contributor Author

[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 996ad48

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13454/) (Base Commit: ea2e42e)

@smarterclayton
Copy link
Contributor Author

@openshift/storage need a reviewer and someone to triage this - if 1.4 is doing attach in both places (which it looks like it is) then I need an assessment whether we need a fix.

@rootfs
Copy link
Member

rootfs commented Feb 1, 2017

LGTM. volumes can be attached/detached via controller or kubelet, but development and testing focus on controller. There are also good amount of fixes in 1.4 to stablize the controller.

@wongma7
Copy link
Contributor

wongma7 commented Feb 1, 2017

We are safe, 1.4 is not doing attach in both places. The controller will obey the node's setting which is just an annotation on the node. So at worst the attach detach controller is sitting idle while the nodes do attach themselves
https://github.com/kubernetes/kubernetes/blob/9ae2dfacf196ca7dbee798ee9c3e1663a5f39473/pkg/controller/volume/attachdetach/attach_detach_controller.go#L277

openshift link: https://github.com/openshift/origin/blob/release-1.4/vendor/k8s.io/kubernetes/pkg/controller/volume/attachdetach/attach_detach_controller.go#L241

@smarterclayton
Copy link
Contributor Author

smarterclayton commented Feb 1, 2017 via email

@eparis
Copy link
Member

eparis commented Feb 1, 2017

you concern is 'admin set the flag on the node, did the node have permission to update the annotation' ?

@smarterclayton
Copy link
Contributor Author

Just that we as a team are 100% sure we know that attach/detach is working correctly at HEAD because otherwise "example scenario" wouldn't even work. If that's the case this goes back to a p2 and do it later.

@smarterclayton
Copy link
Contributor Author

Eric suggested we wait to flip the default until 1.6 which makes sense.

@smarterclayton smarterclayton modified the milestones: 1.6.0, 1.4.x Feb 1, 2017
@eparis
Copy link
Member

eparis commented Feb 1, 2017

I would suggest this not merge until 3.6. I wouldn't want a 3.5 node (default master attach detach) and a 3.3 master (node attach detach) together. 3.4 the master should be g2g so 3.6 node default would be a good idea.

@gnufied
Copy link
Member

gnufied commented Mar 1, 2017

@eparis I think we should push for this as default behaviour on 3.5 itself, rather than waiting for 3.6. Looking at bugs we have been getting because cluster doing node side attach/detach - we have a problem at hand.

The thing is, Node side attach detach is really bad for variety of reasons:

  1. First minor annoyance is, there are no events attached to pod when node side attach/detach fails. We have zero idea of what happened and often users themselves don't have access to logs. It makes debugging super hard.
  2. Race conditions fixes aren't available when node side attach/detach is enabled. upstream has made lot of fixes to improve attach/detach race conditions and make it more stable and all bets are off if user uses nod side attach/detach.

I am happy to verify and double check, what happens when master is 3.3 and node is 3.5 but controller doing attach/detach has been available since >=3.3.

@smarterclayton
Copy link
Contributor Author

[merge] for 1.6

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 996ad48

@openshift-bot
Copy link
Contributor

openshift-bot commented Mar 2, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/718/) (Base Commit: 2cac04a) (Image: devenv-rhel7_6011)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants