Allow to change the cluster default network type by pliurh · Pull Request #438 · openshift/cluster-network-operator

pliurh · 2020-01-07T14:51:32Z

Allow users to change the cluster default network type by updating the 'networkType' field of the network.config.openshift.io CR. This not only support the use case of migrating the cluster network from Openshift SDN to OVN-Kubernetes, but also the use case vise Versa.

danwinship · 2020-01-08T15:22:12Z

/hold
We can't allow this in general; it would completely break the cluster. We need to have some way of allowing it as part of the supported migration procedure, but blocking it otherwise.

danwinship · 2020-01-08T15:23:32Z

bindata/network/openshift-sdn/sdn-ovs.yaml

+                #!/bin/bash
+                /usr/share/openvswitch/scripts/ovs-ctl stop
+                rm -rf /run/openvswitch/*
+                rm -rf /var/lib/openvswitch/*


We don't want to do this on ordinary termination; it would mean that every upgrade would tear down the network. (Likewise for the ovn-kube change.)

danwinship · 2020-01-08T15:24:53Z

bindata/network/openshift-sdn/sdn.yaml

+        lifecycle:
+          preStop:
+            exec:
+              command: ["rm","-f","/etc/cni/net.d/80-openshift-network.conf"]


there is already a preStop handler at the end of the container spec that does this

danwinship · 2020-01-08T15:25:23Z

bindata/network/ovn-kubernetes/monitor.yaml

  ports:
  - name: metrics
-    port: 9101
+    port: 9103


(this could be done as a separate PR...)

danwinship · 2020-01-08T15:26:42Z

pkg/controller/statusmanager/status_manager.go

 			objToDelete.SetNamespace(currentObj.Namespace)
 			objToDelete.SetGroupVersionKind(gvk)
+			if objToDelete.GetKind() == "Namespace" {
+				continue


Why was this added? We want to delete unused namespaces...
(Though... I guess probably we need to fix it to do that last.)

Due to some reason, the 'namespace' deletion is always stuck at the "Terminating" state forever, even when all the related objects in that namespace have been deleted. And it will block the same Namespace creation if we what to perform a rollback from ovn-kubernetes to openshift-sdn.

I agree that we need to remove the unused namespaces anyway. But I think maybe we can fix it later.

Due to some reason, the 'namespace' deletion is always stuck at the "Terminating" state forever, even when all the related objects in that namespace have been deleted.

Maybe there is something else in that Namespace that isn't in the related objects then?

Yes, there are some objects which are not managed by the CNO. I am not sure what is the proper way to delete all of them, then delete the namespace itself.

hm. do you remember what objects?

I did some further tests. It turns out that the namespace will be stuck in 'Terminating' before the cluster reboot, but can be deleted eventually after that. Therefore I remove the lines above.

danwinship · 2020-01-08T15:27:37Z

pkg/controller/statusmanager/status_manager.go

 		oldStatus := co.Status.DeepCopy()
 		status.deleteRelatedObjectsNotRendered(co)
+		status.deleteDaemonSetNotRendered(co)
+		status.deleteDeploymentNotRendered(co)


why is this needed? The DaemonSets and Deployments should also be in RelatedObjects

The origin intention is to remove the deployment and daemonset before other objects. But I found it is unnecessary after more tests. It shall be removed.

pliurh · 2020-01-13T03:15:58Z

/hold
We can't allow this in general; it would completely break the cluster. We need to have some way of allowing it as part of the supported migration procedure, but blocking it otherwise.

Agree. What's your suggestion on how to do it? How about adding an annotation like maintenance: "migration" in the cluster CR to indicate if this kind of operation can be allowed or not?

danwinship · 2020-01-13T14:56:25Z

An annotation sounds good. It should be more namespaced than that though. Maybe something like

"networkoperator.openshift.io/network-migration": ""

(Standard annotation semantics are that if there's nothing to say about the annotation beyond the fact that it exists, then you just give it an empty-string value rather than "true" or something like that.)

You still need to figure out exactly what that entails though; you don't want CNO to start changing DaemonSets right away because that would interfere with the controlled node-by-node migration plan... do you stop the CNO during the migration? Is the rest of the code for dealing with migrations posted somewhere?

pliurh · 2020-01-13T15:09:37Z

An annotation sounds good. It should be more namespaced than that though. Maybe something like
"networkoperator.openshift.io/network-migration": ""
(Standard annotation semantics are that if there's nothing to say about the annotation beyond the fact that it exists, then you just give it an empty-string value rather than "true" or something like that.)

You still need to figure out exactly what that entails though; you don't want CNO to start changing DaemonSets right away because that would interfere with the controlled node-by-node migration plan... do you stop the CNO during the migration? Is the rest of the code for dealing with migrations posted somewhere?

In 4.4, we plan to take the cluster reboot approach, which means all the nodes will be rebooted during the migration. Therefore, the whole cluster shall be considered as out of service during the migration. So all the pod will be connected to the new network backend after reboot. I gave a live demo at the Group C demo on Jan 8th, however, the demo record has not been added to the agenda doc yet. But here is a previous demo record, which shows the whole procedure of the 'cluster reboot' migration.

pliurh · 2020-01-20T02:51:44Z

/test e2e-gcp

pliurh · 2020-01-20T06:02:25Z

/test e2e-gcp

danwinship

So this is looking pretty simple now. We still need to figure out what's going wrong with deleting the namespace

danwinship · 2020-01-22T11:03:43Z

pkg/controller/operconfig/operconfig_controller.go


+const (
+	// The Network Migration Annotation which indicates if the operator is allowed to change the cluster network type.
+	MigrationAnno = "networkoperator.openshift.io/network-migration"


move this to pkg/names/names.go and spell out "Annotation" in full, please

danwinship · 2020-01-23T09:56:21Z

Thanks. Can you squash the commits?

Allow users to change the cluster default network type by updating the 'networkType' field of the network.config.openshift.io CR. This not only supports the use case of migrating the cluster network from Openshift SDN to OVN-Kubernetes, but also the case vise Versa. By default, this operation is not allowed by the operator. Users need to add annotation 'networkoperator.openshift.io/network-migration: ""' to the Network.operator.openshift.io CR 'cluster' before executing.

pliurh · 2020-01-23T13:58:33Z

Thanks. Can you squash the commits?

Done.

pliurh · 2020-01-24T13:41:29Z

So this is looking pretty simple now. We still need to figure out what's going wrong with deleting the namespace

Before rebooting, the cluster is in an unstable status. The stuck namespace has a finalizer 'kubernetes', which blocks the deletion. I think it should be fine, as long as, the namespace can be deleted successfully after the cluster back to normal.

p.s. There is a way to force delete the namespace kubernetes/kubernetes#60807 (comment), it could be a way out if the namespace deletion went wrong.

danwinship · 2020-01-24T14:01:43Z

/lgtm
/hold cancel

openshift-ci-robot · 2020-01-24T14:02:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, pliurh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [danwinship]

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

openshift-bot · 2020-01-24T17:11:55Z

/retest

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

openshift-bot · 2020-01-24T17:36:34Z

/retest

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

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 7, 2020

openshift-ci-robot requested review from alexanderConstantinescu and danwinship January 7, 2020 14:51

pliurh force-pushed the cold_migration branch 2 times, most recently from 2929c8b to 7cb3a11 Compare January 8, 2020 08:19

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 8, 2020

danwinship requested changes Jan 8, 2020

View reviewed changes

pliurh mentioned this pull request Jan 13, 2020

Change the metrics port for OVN-Kubernetes #443

Merged

pliurh force-pushed the cold_migration branch from 7cb3a11 to fa7972b Compare January 13, 2020 14:56

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 13, 2020

pliurh force-pushed the cold_migration branch 2 times, most recently from 9dc3053 to b455578 Compare January 13, 2020 15:25

danwinship reviewed Jan 22, 2020

View reviewed changes

pliurh force-pushed the cold_migration branch from b455578 to 0cdfe0a Compare January 22, 2020 14:59

pliurh force-pushed the cold_migration branch from ae7064a to 288b502 Compare January 23, 2020 13:56

danwinship approved these changes Jan 24, 2020

View reviewed changes

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 24, 2020

openshift-ci-robot assigned danwinship Jan 24, 2020

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

openshift-merge-robot merged commit bfc8b01 into openshift:master Jan 24, 2020

Conversation

pliurh commented Jan 7, 2020

Uh oh!

danwinship commented Jan 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pliurh commented Jan 13, 2020

Uh oh!

danwinship commented Jan 13, 2020

Uh oh!

pliurh commented Jan 13, 2020

Uh oh!

pliurh commented Jan 20, 2020

Uh oh!

pliurh commented Jan 20, 2020

Uh oh!

danwinship left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danwinship commented Jan 23, 2020

Uh oh!

pliurh commented Jan 23, 2020

Uh oh!

pliurh commented Jan 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danwinship commented Jan 24, 2020

Uh oh!

openshift-ci-robot commented Jan 24, 2020

Uh oh!

openshift-bot commented Jan 24, 2020

Uh oh!

openshift-bot commented Jan 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pliurh commented Jan 24, 2020 •

edited

Loading