manifests: switch network operator to deployment #36

abhinavdahiya · 2018-11-13T23:20:35Z

openshift/cluster-version-operator#38 (comment)

@deads2k suggests that a deployment should be able to schedule even when node is not-ready.

abhinavdahiya · 2018-11-13T23:50:46Z

sh-4.2$ oc -n openshift-cluster-network-operator get pods
NAME                                       READY     STATUS    RESTARTS   AGE
cluster-network-operator-5cb656c65-s2s5n   0/1       Pending   0          30s
sh-4.2$ oc -n openshift-cluster-network-operator get pods cluster-network-operator-5cb656c65-s2s5n
NAME                                       READY     STATUS    RESTARTS   AGE
cluster-network-operator-5cb656c65-s2s5n   0/1       Pending   0          45s
sh-4.2$ oc -n openshift-cluster-network-operator describe pods cluster-network-operator-5cb656c65-s2s5n
Name:               cluster-network-operator-5cb656c65-s2s5n
Namespace:          openshift-cluster-network-operator
Priority:           0
PriorityClassName:  <none>
Node:               <none>
Labels:             k8s-app=cluster-network-operator
                    pod-template-hash=176212721
Annotations:        <none>
Status:             Pending
IP:
Controlled By:      ReplicaSet/cluster-network-operator-5cb656c65
Containers:
  cluster-network-operator:
    Image:      registry.svc.ci.openshift.org/ci-op-tl6yk2r1/stable@sha256:7210108e0a9bddbe8773dc23d53f92d3ff6ebb2b8e158a1ebc1fa1ba74dcc6bd
    Port:       <none>
    Host Port:  <none>
    Command:
      /bin/cluster-network-operator
      --url-only-kubeconfig=/etc/kubernetes/kubeconfig
    Limits:
      cpu:     20m
      memory:  50Mi
    Requests:
      cpu:     20m
      memory:  50Mi
    Environment:
      NODE_IMAGE:        registry.svc.ci.openshift.org/ci-op-tl6yk2r1/stable@sha256:2915dc4741e62def2bf15886012e2d0ad3cb5dea0db2f4b516275c3537220000
      HYPERSHIFT_IMAGE:  registry.svc.ci.openshift.org/ci-op-tl6yk2r1/stable@sha256:d8c99f7fc22bd8e387ef5cbf85d69c4f1ed59bb17da0944beb33a333d963add7
      POD_NAME:          cluster-network-operator-5cb656c65-s2s5n (v1:metadata.name)
    Mounts:
      /etc/kubernetes/kubeconfig from host-kubeconfig (ro)
      /var/run/secrets/kubernetes.io/serviceaccount from default-token-s29cp (ro)
Conditions:
  Type           Status
  PodScheduled   False
Volumes:
  host-kubeconfig:
    Type:          HostPath (bare host directory volume)
    Path:          /etc/kubernetes/kubeconfig
    HostPathType:
  default-token-s29cp:
    Type:        Secret (a volume populated by a Secret)
    SecretName:  default-token-s29cp
    Optional:    false
QoS Class:       Guaranteed
Node-Selectors:  node-role.kubernetes.io/master=
Tolerations:     node-role.kubernetes.io/master:NoSchedule
                 node.kubernetes.io/memory-pressure:NoSchedule
                 node.kubernetes.io/not-ready:NoSchedule
Events:
  Type     Reason            Age                From               Message
  ----     ------            ----               ----               -------
  Warning  FailedScheduling  4s (x15 over 53s)  default-scheduler  0/3 nodes are available: 3 node(s) were not ready.

squeed · 2018-11-14T12:57:58Z

I assume we'll have to add some tolerations too...

deads2k · 2018-11-14T13:12:31Z

@aveshagarwal @sjenning I thought it may have been because the scheduler wasn't starting on the bootstrap node, but if it is, then it appears something is still wrong :(

aveshagarwal · 2018-11-14T14:09:39Z

@deads2k suggests that a deployment should be able to schedule even when node is not-ready.

I think that is only applicable to daemonsets, not for deployments. If we want to have deployments scheduled to not-ready nodes, it would require adding not-ready tolerations to them.

aveshagarwal · 2018-11-14T14:15:18Z

See here: https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/daemon/util/daemonset_util.go#L57-#L61

aveshagarwal · 2018-11-14T14:16:39Z

Though it does not seem like a normal pattern to have deployments forced to schedule on not-ready nodes.

aveshagarwal · 2018-11-14T14:22:06Z

also can you share the output of "oc describe nodes" for master nodes to see what is going on with them and what do they have w.r.t. taints?

deads2k · 2018-11-14T14:22:48Z

I think that is only applicable to daemonsets, not for deployments. If we want to have deployments scheduled to not-ready nodes, it would require adding not-ready tolerations to them.

Isn't that in the output above:

Tolerations:     node-role.kubernetes.io/master:NoSchedule
                 node.kubernetes.io/memory-pressure:NoSchedule
                 node.kubernetes.io/not-ready:NoSchedule

aveshagarwal · 2018-11-14T14:25:55Z

Isn't that in the output above:

           node.kubernetes.io/not-ready:NoSchedule

ok, that seems fine, but lets see what are all the taints on nodes to see if the pods have all required tolerations.

aveshagarwal · 2018-11-14T14:27:13Z

i think we might need the following too:

https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/daemon/util/daemonset_util.go#L88-#L93

aveshagarwal · 2018-11-14T14:28:17Z

or we might need all of the following: https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/daemon/util/daemonset_util.go#L76-#L115

aveshagarwal · 2018-11-14T14:29:44Z

@deads2k what is the reason we are using deployments and not daemonsets, which are specifically (at least one of the main reasons) for bootstrapping purposes?

aveshagarwal · 2018-11-14T14:33:18Z

@deads2k also in 1.12, DS are scheduled by the default scheduler so should work as far as scheduler is running.

aveshagarwal · 2018-11-14T14:41:46Z

node.kubernetes.io/not-ready:NoSchedule

Or at the minimum try to change it to to add a new one: node.kubernetes.io/not-ready:NoExecute and see if helps.

ravisantoshgudimetla · 2018-11-14T16:23:08Z

What version of kube are we on? This could be related to openshift/cluster-version-operator#35 (comment), where we have not enabled TaintNodesByCondition and instead the pod created from deployment go through CheckNodeConditions predicate.

deads2k · 2018-11-14T16:39:21Z

@aveshagarwal if you have a daemonset for an operator, it runs on every node. That's fine with 5 nodes. It's not fine with 500 nodes. In addition, you cannot scale them. The rollout functionality on updates isn't the same, so lease acquisition is weird.

Basically, deployments are the rigth resource. Let's figure out what's wrong in the scheduler or deployment that prevents it from scheduling.

squeed · 2018-11-14T16:44:32Z

fwiw, the network operator daemonset only runs on the masters, so it's not so bad.

aveshagarwal · 2018-11-14T16:48:45Z

@aveshagarwal if you have a daemonset for an operator, it runs on every node. That's fine with 5 nodes. It's not fine with 500 nodes. In addition, you cannot scale them. The rollout functionality on updates isn't the same, so lease acquisition is weird.

Basically, deployments are the rigth resource. Let's figure out what's wrong in the scheduler or deployment that prevents it from scheduling.

I think there is nothing wrong in scheduler as thats how the scheduler has been so far (until 1.12). By default, scheduler eliminates unschedulable nodes (including not-ready), and as deployments are scheduled by the default scheduler, deployments can not be scheduled on unschedulable nodes by the default scheduler. This behavior is different for DS because DS controller does the actual scheduling of DS pods (upto 1.11) so schedules pods on unschedulable nodes (including not-ready).

I think the behavior you are looking for deployments should be achievable in 1.12. Because in 1.12, TaintNodesByCondition is enabled by default and node conditions are managed by using taints. That means no matter what are the conditions on nodes, pods (irrespective of whether via deployments or DS, job, cronjob, statefulsets etc) can be scheduled on any node as far as the pods are assigned right tolerations.

deads2k · 2018-11-14T16:51:00Z

Because in 1.12, TaintNodesByCondition is enabled by default and node conditions are managed by using taints

Can you enable it in 1.11? Is there a reason not to?

aveshagarwal · 2018-11-14T17:02:49Z

Because in 1.12, TaintNodesByCondition is enabled by default and node conditions are managed by using taints

Can you enable it in 1.11? Is there a reason not to?

I would not be comfortable in enabling alpha feature in 1.11 because

In 1.12, the feature has seen some testing so there is a bit of confidence about its stability.
That would also mean bringing those patches that have gone into kube to make it more stable. Dont remember off the top of my head what those are.

Also if the operator stuff is for 4.0 that means 1.12, why would we want to enable the alpha feature in 1.11?

smarterclayton · 2018-11-20T17:29:00Z

I'm confused - why can't we enable this in origin master so that we can verify that this works before we land the rebase and then find out it doesn't?

squeed · 2018-11-20T17:38:56Z

I've already run the operator as a deployment against a vanilla 1.13 cluster. I can test 1.12.

aveshagarwal · 2018-11-20T17:40:08Z

I've already run the operator as a deployment against a vanilla 1.13 cluster. I can test 1.12.

Yes please.

aveshagarwal · 2018-11-20T17:44:40Z

I've already run the operator as a deployment against a vanilla 1.13 cluster. I can test 1.12.

Did you try with nodes with not-ready conditions?

squeed · 2018-11-27T09:25:29Z

Whoops, the CVO still had an old override for a cluster-network-operator Deployment from ages ago. PR to remove it is openshift/installer#735

abhinavdahiya · 2019-01-25T23:53:09Z

only 2 tests failed Woohoo! with 1.12 rebase in this seems to be ready for review.
cc @squeed

failures from e2e-aws

Failing tests:

[sig-storage] Dynamic Provisioning DynamicProvisioner should provision storage with different parameters [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-storage] Volume limits should verify that all nodes have volume limits [Suite:openshift/conformance/parallel] [Suite:k8s]

/retest

danwinship · 2019-01-26T13:28:24Z

/retest
(Note since I already confused myself about this: This does not conflict with #72, since that only assumes that the SDN pods are deployed via DaemonSet. It doesn't care about how the operator itself is deployed.)

squeed · 2019-01-28T13:08:06Z

/lgtm
/approve

squeed · 2019-01-28T13:08:44Z

We should switch the openshift-sdn controller to a Deployment now too.

openshift-ci-robot · 2019-01-28T13:09:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, squeed

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:

~~OWNERS~~ [squeed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

abhinavdahiya · 2019-01-28T13:56:54Z

/retest

squeed · 2019-01-28T15:25:49Z

I can see from the e2e run that the network operator succeeded, but bootstrap didn't succeed. So that's a flake (albeit a troubling one).

openshift-bot · 2019-01-29T04:28:56Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-01-29T08:31:56Z

/retest

Please review the full test history for this PR and help us cut down flakes.

sjenning · 2019-01-29T16:41:55Z

the file needs to be renamed as well

…ployment This was originally renamed from deployment to daemonset in 8ecd1ba (Refactor the operator for operator-sdk v0.1.0, 2018-11-02, openshift#25). The content was changed back to a Deployment in 19de4ca (manifests: switch network operator to deployment, 2018-11-13, openshift#36), and this commit catches the manifest name back up.

openshift-ci-robot requested review from dcbw and smarterclayton November 13, 2018 23:20

openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 13, 2018

squeed mentioned this pull request Nov 20, 2018

operator: Switch to a deployment to be consistent with other operators #39

Closed

abhinavdahiya force-pushed the master branch from 4e0b494 to 59daee2 Compare January 25, 2019 20:57

manifests: switch network operator to deployment

19de4ca

abhinavdahiya force-pushed the master branch from 59daee2 to 19de4ca Compare January 25, 2019 22:23

openshift-ci-robot assigned squeed Jan 28, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 28, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 28, 2019

openshift-merge-robot merged commit e029709 into openshift:master Jan 29, 2019

wking mentioned this pull request Oct 30, 2019

manifests/0000_70_cluster-network-operator_03: Rename daemonset -> deployment #370

Merged

manifests: switch network operator to deployment #36

manifests: switch network operator to deployment #36

Conversation

abhinavdahiya commented Nov 13, 2018

abhinavdahiya commented Nov 13, 2018

squeed commented Nov 14, 2018

deads2k commented Nov 14, 2018

aveshagarwal commented Nov 14, 2018

aveshagarwal commented Nov 14, 2018

aveshagarwal commented Nov 14, 2018

aveshagarwal commented Nov 14, 2018

deads2k commented Nov 14, 2018

aveshagarwal commented Nov 14, 2018

aveshagarwal commented Nov 14, 2018

aveshagarwal commented Nov 14, 2018

aveshagarwal commented Nov 14, 2018

aveshagarwal commented Nov 14, 2018

aveshagarwal commented Nov 14, 2018

ravisantoshgudimetla commented Nov 14, 2018 • edited

deads2k commented Nov 14, 2018

squeed commented Nov 14, 2018

aveshagarwal commented Nov 14, 2018 • edited

deads2k commented Nov 14, 2018

aveshagarwal commented Nov 14, 2018

smarterclayton commented Nov 20, 2018

squeed commented Nov 20, 2018

aveshagarwal commented Nov 20, 2018

aveshagarwal commented Nov 20, 2018

squeed commented Nov 27, 2018

abhinavdahiya commented Jan 25, 2019

danwinship commented Jan 26, 2019

squeed commented Jan 28, 2019

squeed commented Jan 28, 2019

openshift-ci-robot commented Jan 28, 2019

abhinavdahiya commented Jan 28, 2019

squeed commented Jan 28, 2019

openshift-bot commented Jan 29, 2019

openshift-bot commented Jan 29, 2019

sjenning commented Jan 29, 2019

ravisantoshgudimetla commented Nov 14, 2018 •

edited

aveshagarwal commented Nov 14, 2018 •

edited