Skip drain on Single Node deployment #2457
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,14 +22,18 @@ import ( | |
| "k8s.io/client-go/tools/record" | ||
|
|
||
| apicfgv1 "github.com/openshift/api/config/v1" | ||
| configv1 "github.com/openshift/api/config/v1" | ||
| fakeconfigv1client "github.com/openshift/client-go/config/clientset/versioned/fake" | ||
| configv1informer "github.com/openshift/client-go/config/informers/externalversions" | ||
| mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" | ||
| ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common" | ||
| daemonconsts "github.com/openshift/machine-config-operator/pkg/daemon/constants" | ||
| "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned/fake" | ||
| informers "github.com/openshift/machine-config-operator/pkg/generated/informers/externalversions" | ||
| "github.com/openshift/machine-config-operator/pkg/version" | ||
| "github.com/openshift/machine-config-operator/test/helpers" | ||
| "github.com/stretchr/testify/assert" | ||
| utilrand "k8s.io/apimachinery/pkg/util/rand" | ||
| ) | ||
|
|
||
| var ( | ||
|
|
@@ -44,6 +48,7 @@ type fixture struct { | |
| kubeclient *k8sfake.Clientset | ||
| schedulerClient *fakeconfigv1client.Clientset | ||
|
|
||
| ccLister []*mcfgv1.ControllerConfig | ||
| mcpLister []*mcfgv1.MachineConfigPool | ||
| nodeLister []*corev1.Node | ||
|
|
||
|
|
@@ -72,9 +77,10 @@ func (f *fixture) newController() *Controller { | |
| i := informers.NewSharedInformerFactory(f.client, noResyncPeriodFunc()) | ||
| k8sI := kubeinformers.NewSharedInformerFactory(f.kubeclient, noResyncPeriodFunc()) | ||
| ci := configv1informer.NewSharedInformerFactory(f.schedulerClient, noResyncPeriodFunc()) | ||
| c := New(i.Machineconfiguration().V1().ControllerConfigs(), i.Machineconfiguration().V1().MachineConfigPools(), k8sI.Core().V1().Nodes(), | ||
| ci.Config().V1().Schedulers(), f.kubeclient, f.client) | ||
|
|
||
| c.ccListerSynced = alwaysReady | ||
| c.mcpListerSynced = alwaysReady | ||
| c.nodeListerSynced = alwaysReady | ||
| c.schedulerListerSynced = alwaysReady | ||
|
|
@@ -87,6 +93,9 @@ func (f *fixture) newController() *Controller { | |
| k8sI.Start(stopCh) | ||
| k8sI.WaitForCacheSync(stopCh) | ||
|
|
||
| for _, c := range f.ccLister { | ||
| i.Machineconfiguration().V1().ControllerConfigs().Informer().GetIndexer().Add(c) | ||
| } | ||
| for _, c := range f.mcpLister { | ||
| i.Machineconfiguration().V1().MachineConfigPools().Informer().GetIndexer().Add(c) | ||
| } | ||
|
|
@@ -97,6 +106,7 @@ func (f *fixture) newController() *Controller { | |
| for _, c := range f.schedulerLister { | ||
| ci.Config().V1().Schedulers().Informer().GetIndexer().Add(c) | ||
| } | ||
|
|
||
| return c | ||
| } | ||
|
|
||
|
|
@@ -148,6 +158,20 @@ func (f *fixture) runController(pool string, expectError bool) { | |
| } | ||
| } | ||
|
|
||
| func newControllerConfig(name string, topology configv1.TopologyMode) *mcfgv1.ControllerConfig { | ||
| return &mcfgv1.ControllerConfig{ | ||
| TypeMeta: metav1.TypeMeta{APIVersion: mcfgv1.SchemeGroupVersion.String()}, | ||
| ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{daemonconsts.GeneratedByVersionAnnotationKey: version.Raw}, Name: name, UID: types.UID(utilrand.String(5))}, | ||
| Spec: mcfgv1.ControllerConfigSpec{ | ||
| Infra: &configv1.Infrastructure{ | ||
| Status: configv1.InfrastructureStatus{ | ||
| ControlPlaneTopology: topology, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| // checkAction verifies that expected and actual actions are equal and both have | ||
| // same attached resources | ||
| func checkAction(expected, actual core.Action, t *testing.T) { | ||
|
|
@@ -188,6 +212,8 @@ func filterInformerActions(actions []core.Action) []core.Action { | |
| if len(action.GetNamespace()) == 0 && | ||
| (action.Matches("list", "machineconfigpools") || | ||
| action.Matches("watch", "machineconfigpools") || | ||
| action.Matches("list", "controllerconfigs") || | ||
| action.Matches("watch", "controllerconfigs") || | ||
| action.Matches("list", "nodes") || | ||
| action.Matches("watch", "nodes")) { | ||
| continue | ||
|
|
@@ -758,14 +784,17 @@ func TestSetDesiredMachineConfigAnnotation(t *testing.T) { | |
|
|
||
| func TestShouldMakeProgress(t *testing.T) { | ||
| f := newFixture(t) | ||
| cc := newControllerConfig(ctrlcommon.ControllerConfigName, configv1.TopologyMode("")) | ||
| mcp := helpers.NewMachineConfigPool("test-cluster-infra", nil, helpers.InfraSelector, "v1") | ||
| mcpWorker := helpers.NewMachineConfigPool("worker", nil, helpers.WorkerSelector, "v1") | ||
| mcp.Spec.MaxUnavailable = intStrPtr(intstr.FromInt(1)) | ||
|
|
||
| nodes := []*corev1.Node{ | ||
| newNodeWithLabel("node-0", "v1", "v1", map[string]string{"node-role/worker": "", "node-role/infra": ""}), | ||
| newNodeWithLabel("node-1", "v0", "v0", map[string]string{"node-role/worker": "", "node-role/infra": ""}), | ||
| } | ||
|
|
||
| f.ccLister = append(f.ccLister, cc) | ||
| f.mcpLister = append(f.mcpLister, mcp, mcpWorker) | ||
| f.objects = append(f.objects, mcp, mcpWorker) | ||
| f.nodeLister = append(f.nodeLister, nodes...) | ||
|
|
@@ -799,8 +828,11 @@ func TestShouldMakeProgress(t *testing.T) { | |
|
|
||
| func TestEmptyCurrentMachineConfig(t *testing.T) { | ||
| f := newFixture(t) | ||
| cc := newControllerConfig(ctrlcommon.ControllerConfigName, configv1.TopologyMode("")) | ||
| mcp := helpers.NewMachineConfigPool("test-cluster-master", nil, helpers.MasterSelector, "") | ||
| mcp.Spec.MaxUnavailable = intStrPtr(intstr.FromInt(1)) | ||
|
|
||
| f.ccLister = append(f.ccLister, cc) | ||
| f.mcpLister = append(f.mcpLister, mcp) | ||
| f.objects = append(f.objects, mcp) | ||
| f.run(getKey(mcp, t)) | ||
|
|
@@ -834,6 +866,7 @@ func TestPaused(t *testing.T) { | |
|
|
||
| func TestShouldUpdateStatusOnlyUpdated(t *testing.T) { | ||
| f := newFixture(t) | ||
| cc := newControllerConfig(ctrlcommon.ControllerConfigName, configv1.TopologyMode("")) | ||
| mcp := helpers.NewMachineConfigPool("test-cluster-infra", nil, helpers.InfraSelector, "v1") | ||
| mcpWorker := helpers.NewMachineConfigPool("worker", nil, helpers.WorkerSelector, "v1") | ||
| mcp.Spec.MaxUnavailable = intStrPtr(intstr.FromInt(1)) | ||
|
|
@@ -842,6 +875,7 @@ func TestShouldUpdateStatusOnlyUpdated(t *testing.T) { | |
| newNodeWithLabel("node-1", "v1", "v1", map[string]string{"node-role/worker": "", "node-role/infra": ""}), | ||
| } | ||
|
|
||
| f.ccLister = append(f.ccLister, cc) | ||
| f.mcpLister = append(f.mcpLister, mcp, mcpWorker) | ||
| f.objects = append(f.objects, mcp, mcpWorker) | ||
| f.nodeLister = append(f.nodeLister, nodes...) | ||
|
|
@@ -859,6 +893,7 @@ func TestShouldUpdateStatusOnlyUpdated(t *testing.T) { | |
|
|
||
| func TestShouldUpdateStatusOnlyNoProgress(t *testing.T) { | ||
| f := newFixture(t) | ||
| cc := newControllerConfig(ctrlcommon.ControllerConfigName, configv1.TopologyMode("")) | ||
| mcp := helpers.NewMachineConfigPool("test-cluster-infra", nil, helpers.InfraSelector, "v1") | ||
| mcpWorker := helpers.NewMachineConfigPool("worker", nil, helpers.WorkerSelector, "v1") | ||
| mcp.Spec.MaxUnavailable = intStrPtr(intstr.FromInt(1)) | ||
|
|
@@ -867,6 +902,7 @@ func TestShouldUpdateStatusOnlyNoProgress(t *testing.T) { | |
| newNodeWithLabel("node-1", "v0", "v1", map[string]string{"node-role/worker": "", "node-role/infra": ""}), | ||
| } | ||
|
|
||
| f.ccLister = append(f.ccLister, cc) | ||
| f.mcpLister = append(f.mcpLister, mcp, mcpWorker) | ||
| f.objects = append(f.objects, mcp, mcpWorker) | ||
| f.nodeLister = append(f.nodeLister, nodes...) | ||
|
|
@@ -884,6 +920,7 @@ func TestShouldUpdateStatusOnlyNoProgress(t *testing.T) { | |
|
|
||
| func TestShouldDoNothing(t *testing.T) { | ||
| f := newFixture(t) | ||
| cc := newControllerConfig(ctrlcommon.ControllerConfigName, configv1.TopologyMode("")) | ||
| mcp := helpers.NewMachineConfigPool("test-cluster-infra", nil, helpers.InfraSelector, "v1") | ||
| mcpWorker := helpers.NewMachineConfigPool("worker", nil, helpers.WorkerSelector, "v1") | ||
| mcp.Spec.MaxUnavailable = intStrPtr(intstr.FromInt(1)) | ||
|
|
@@ -894,6 +931,7 @@ func TestShouldDoNothing(t *testing.T) { | |
| status := calculateStatus(mcp, nodes) | ||
| mcp.Status = status | ||
|
|
||
| f.ccLister = append(f.ccLister, cc) | ||
| f.mcpLister = append(f.mcpLister, mcp, mcpWorker) | ||
| f.objects = append(f.objects, mcp, mcpWorker) | ||
| f.nodeLister = append(f.nodeLister, nodes...) | ||
|
|
@@ -904,6 +942,46 @@ func TestShouldDoNothing(t *testing.T) { | |
| f.run(getKey(mcp, t)) | ||
| } | ||
|
|
||
| func TestControlPlaneTopology(t *testing.T) { | ||
|
Contributor
There was a problem hiding this comment. Sorry, I'm a bit confused by what this is trying to test. Is it checking to see if the controllerconfig topology propagates to node annotations? Where is that being checked?
Contributor
Author
There was a problem hiding this comment. Here, we are currently checking that with a valid controlPlaneToplogy i.e. Later on, I am thinking of extending the test to also perform
Contributor
There was a problem hiding this comment. Hmm, sorry for the dumb question, but I'm still not sure how we are testing that. If an action change happens as a bug, which line of code would return the error? I see we do the same for the above
Contributor
Author
There was a problem hiding this comment. when in test run() calls runController(), node-controller syncHandler(pool) gets called at https://github.com/openshift/machine-config-operator/blob/master/pkg/controller/node/node_controller_test.go#L114 . As you know when this runs, various actions can be generated based on what all operation on node has been performed. In test we filter out list and watch actionhttps://github.com/openshift/machine-config-operator/blob/master/pkg/controller/node/node_controller_test.go#L121as these actions doesn't really update anything. For other actions like patch, additional action will be check in later line during checkAction() |
||
| f := newFixture(t) | ||
| cc := newControllerConfig(ctrlcommon.ControllerConfigName, configv1.SingleReplicaTopologyMode) | ||
| mcp := helpers.NewMachineConfigPool("test-cluster-infra", nil, helpers.InfraSelector, "v1") | ||
| mcpWorker := helpers.NewMachineConfigPool("worker", nil, helpers.WorkerSelector, "v1") | ||
| mcp.Spec.MaxUnavailable = intStrPtr(intstr.FromInt(1)) | ||
| annotations := map[string]string{daemonconsts.ClusterControlPlaneTopologyAnnotationKey: "SingleReplica"} | ||
|
|
||
| nodes := []*corev1.Node{ | ||
| newNodeWithLabel("node-0", "v1", "v1", map[string]string{"node-role/worker": "", "node-role/infra": ""}), | ||
| newNodeWithLabel("node-1", "v1", "v1", map[string]string{"node-role/worker": "", "node-role/infra": ""}), | ||
| } | ||
|
|
||
| for _, node := range nodes { | ||
| addNodeAnnotations(node, annotations) | ||
| } | ||
| status := calculateStatus(mcp, nodes) | ||
| mcp.Status = status | ||
|
|
||
| f.ccLister = append(f.ccLister, cc) | ||
| f.mcpLister = append(f.mcpLister, mcp, mcpWorker) | ||
| f.objects = append(f.objects, mcp, mcpWorker) | ||
| f.nodeLister = append(f.nodeLister, nodes...) | ||
| for idx := range nodes { | ||
| f.kubeobjects = append(f.kubeobjects, nodes[idx]) | ||
| } | ||
|
|
||
| f.run(getKey(mcp, t)) | ||
| } | ||
|
|
||
| // adds annotation to the node | ||
| func addNodeAnnotations(node *corev1.Node, annotations map[string]string) { | ||
| if node.Annotations == nil { | ||
| node.Annotations = map[string]string{} | ||
| } | ||
| for k, v := range annotations { | ||
| node.Annotations[k] = v | ||
| } | ||
| } | ||
|
|
||
| func getKey(config *mcfgv1.MachineConfigPool, t *testing.T) string { | ||
| key, err := cache.DeletionHandlingMetaNamespaceKeyFunc(config) | ||
| if err != nil { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is
controlPlaneTopologycoming from? I see in the commit message:ControllerConfig now understands ControlPlaneTopology:TopologyMode set in the cluster.But I don't see how we're syncing that into the controlPlaneTopology field in the controllerconfig, maybe I'm missing something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syncMachineConfigController() calls resourceapply.ApplyControllerConfig that calls resourcemerge.EnsureControllerConfig which calls ensureControllerConfigSpec() and here we check if Infra object has been modified and update controllerconfig if changed . Infra object is the one where
controlPlaneTopologyvalue exist as well along with other data https://github.com/openshift/api/blob/master/config/v1/types_infrastructure.go#L86 . MCO was already reading and updating Infra content, so adding controlPlaneToplogy field in crd populates its value as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And operator reads infrastructure object from cluster at https://github.com/openshift/machine-config-operator/blob/master/pkg/operator/sync.go#L220 and syncs the controllerconfigspec . There are too many things to followup...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, ok thanks for the explanation! Right it gets synced as part of the infra field. Not the clearest of code paths 🤦