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

[BUG] Worker node cordoned does not change the state of nodes in Longhorn #1287

Closed
sowmyav27 opened this issue May 2, 2020 · 12 comments
Closed
Assignees
Labels
area/kubernetes Kubernetes related like K8s version compatibility component/longhorn-manager Longhorn manager (control plane) highlight Important feature/issue to highlight kind/feature Feature request, new feature priority/1 Highly recommended to implement or fix in this release (managed by PO) require/auto-e2e-test Require adding/updating auto e2e test cases if they can be automated
Milestone

Comments

@sowmyav27
Copy link

Describe the bug
Worker node cordoned does not change the state of nodes in Longhorn

To Reproduce

  • Deploy longhorn in a cluster
  • Cordon a worker node
  • The node is still seen in "Schedulable" state.

Expected behavior
The node should be marked with a state to reflect that it has been cordoned.

Environment:

  • Longhorn version: master - 05/01/2020
  • Kubernetes version: 1.17.5
  • Node OS type and version: rke DO linux
@yasker yasker added this to the v1.0.0 milestone May 2, 2020
@yasker
Copy link
Member

yasker commented May 4, 2020

Related to #1278

@yasker yasker added area/kubernetes Kubernetes related like K8s version compatibility component/longhorn-manager Longhorn manager (control plane) kind/feature Feature request, new feature priority/1 Highly recommended to implement or fix in this release (managed by PO) and removed kind/bug labels May 4, 2020
@yasker
Copy link
Member

yasker commented May 4, 2020

We can automatically disable the Longhorn node schedule when Kubernetes cordon the node. Since the user don't want any workload to be run on the node, it's highly likely they don't want any new replica to be created on the node as well. But just in case, we need to add a setting in the Scheduling section: disable-scheduling-when-cordoned. The default value would be true.

We can add a new node condition type Cordoned. And when the node is cordoned, update the condition regardless of the setting disable-scheduling-when-cordoned. If the setting was set to true, and node is cordoned, we will skip the node in the scheduler.

UI needs to respond to the condition as well by showing the node as unschedulable.

See #1278 (comment) for some information about cordon.

@yasker yasker added highlight Important feature/issue to highlight require/auto-e2e-test Require adding/updating auto e2e test cases if they can be automated labels May 4, 2020
@boknowswiki
Copy link
Contributor

boknowswiki commented May 10, 2020

@smallteeths could you help to clarify two questions?

  1. Since we need to provide a setting for customer to decide if they would like to cordon the node or not if k8s disable schedule the node. What API do you expect, could you show me an example? I am thinking something similar to "Setting/general/Replica Soft Anti-Affinity", please let me know what do you think?

  2. On UI node section, the status shows "Schedulable" in green box, and when you put the mount on the green box "Schedulable", it shows "node xxx is ready", could you help to clarify how does UI get this information? Like from which API?

Thanks,
Bo

@yasker
Copy link
Member

yasker commented May 11, 2020

@boknowswiki For the setting part, you can refer to https://github.com/longhorn/longhorn-manager/blob/master/types/setting.go . The frontend will detect this automatically.

@boknowswiki
Copy link
Contributor

@yasker , do you know from UI side, what's the API call to get the node status? I would like to use that do verify longhorn-manager replying the right status.

@boknowswiki
Copy link
Contributor

boknowswiki commented May 11, 2020

Test replica scheduler: schedule replica based on Disable Scheduling On Cordoned Node setting

1. Set `Disable Scheduling On Cordoned Node` to true.
2. Set `Replica Soft Anti-Affinity` to false.
3. Set cordon on one node.
4. Create a volume and verify the scheduler should fail due to cordoned node.
5. Remove step 4 volume.
6. Set `Disable Scheduling On Cordoned Node` to false.
7. Create a volume and verify the scheduler successfully creates three replicas.

Also I have submitted a PR on the longhorn-test repo:

longhorn/longhorn-tests#314

@yasker , @meldafrawi , please let me know what do you think?

Thanks,
Bo

@boknowswiki
Copy link
Contributor

boknowswiki commented May 12, 2020

After discussed with @yasker , below is the order we check the "Schedulable" condition in the Node object:

  1. Node needs to be in ready condition.
  2. From the nodes which are ready, we check if the schedulable has been enabled or not on each node.
  3. Check the node schedulable and settings related to schedule.
    Note: for the third one, currently we are checking the k8s node status(cordon) and the setting we have "Disable Scheduling On Cordoned Node" for now.

@smallteeths the response for the "get node" API call is the same as we discussed last night, backend will return the final longhorn node status to UI.
Response cases:

  1. K8s node cordoned and the "Disable Scheduling On Cordoned Node" is set, we return "True".
  2. K8s node uncordoned and "Disable Scheduling On Cordoned Node" is set, we return "False".
  3. K8s node cordoned and "Disable Scheduling On Cordoned Node" is not set, we return "False".
  4. K8s node uncordoned and "Disable Scheduling On Cordoned Node" is not set, we return "False".

@yasker , @smallteeths : Please let me know if there is any misunderstanding or unclarity.

@yasker
Copy link
Member

yasker commented May 12, 2020

@boknowswiki LGTM.

@boknowswiki
Copy link
Contributor

boknowswiki commented May 14, 2020

Pre-merged Checklist

  • Does the PR include the explanation for the fix or the feature?
  • Is the backend code merged?
  • Is the reproduce steps/test steps documented?
  • If labeled: area/ui Has the UI issue filed or ready to be merged?
  • If labeled: require-automation-e2e Has the end-to-end test plan been merged? Have QAs agreed on the automation test case?
  • if labeled: require-automation-engine Has the engine integration test been merged?
  • if labeled: require-doc Has the necessary document PR submitted or merged?

@sowmyav27
Copy link
Author

sowmyav27 commented May 18, 2020

Verified on latest master - 05/18/2020 - ui tag: wangsiye/longhorn-ui:941d022

Validation: PASSED

  • When a node is cordoned in Rancher, the node shows up as Unschedulable in Longhorn. (verified on master also)
  • Create a volume, verified replica does not schedule on this node which is cordoned when Disable Scheduling On Cordoned Node: True (by default on longhorn)

Tested scenario:

  • Worker nodes - 4
  • Create a volume with 4 replicas
  • cordon a worker node w1.
  • delete replica on w1.
  • Volume will be in degraded state with Scheduling Failure Replica Scheduling Failure.
  • Add a node to the custom cluster.
  • The volume becomes healthy with 4 replicas running. (one on the newly added node)

@boknowswiki
Copy link
Contributor

@meldafrawi , I will start to work on this test case today. Just make sure we don't duplicate our work. Please let me know if you have worked on this issue already.

@meldafrawi
Copy link
Contributor

Validation: PASSED
Longhorn Version: v1.0.0-rc1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kubernetes Kubernetes related like K8s version compatibility component/longhorn-manager Longhorn manager (control plane) highlight Important feature/issue to highlight kind/feature Feature request, new feature priority/1 Highly recommended to implement or fix in this release (managed by PO) require/auto-e2e-test Require adding/updating auto e2e test cases if they can be automated
Projects
Status: Closed
Development

No branches or pull requests

4 participants