Codify operator controller #280

cynepco3hahue · 2019-04-11T14:45:57Z

Create the machine-api-operator controller via the code and not via manifest.

remove owned-manifests directory
remove owned-manifests from Dockerfiles
codify the machine-api-operator controller

cynepco3hahue · 2019-04-11T14:47:12Z

@enxebre @ingvagabund I still need to add some basic unittests.

ingvagabund

Looks good in overall. For the container naming scheme, both controller and manager reflects almost the same semantics. I.e. manager can run one or more controllers. So it supersedes controller. It's sufficient to just use controller.

pkg/operator/sync.go

cynepco3hahue · 2019-04-15T06:15:58Z

/retest

cynepco3hahue · 2019-04-15T07:50:21Z

/retest

ingvagabund · 2019-04-15T13:00:35Z

@cynepco3hahue can you re-arrange and squash some commits? Plus remove UPSTREAM: <carry>: openshift: Fix the e2e test to work with new deployment commit? You can also squash both commits touching vendored deps.

cynepco3hahue · 2019-04-15T14:24:31Z

@cynepco3hahue can you re-arrange and squash some commits? Plus remove UPSTREAM: <carry>: openshift: Fix the e2e test to work with new deployment commit? You can also squash both commits touching vendored deps.

@ingvagabund done

enxebre · 2019-04-16T07:07:43Z

/hold

enxebre · 2019-04-16T07:43:32Z

cmd/machine-api-operator/start.go

+	eventRecorderScheme := runtime.NewScheme()
+	osconfigv1.Install(eventRecorderScheme)
+
+	eventBroadcaster := record.NewBroadcaster()


what's the motivation behind moving this here?

to make possible to pass fake recorder under the unittest

enxebre · 2019-04-16T07:49:35Z

pkg/operator/sync.go

+		},
+	}
+
+	_true := true


should be this call runAsNonRoot

check https://github.com/openshift/machine-api-operator/pull/280/files/f9c605b0ee5d50bb86e2ac725738cdf7fc4c6c1f..c92f7f847848375db341a76ed4f56f9c35cbeea1#diff-fa45321336db7ad1cedc28bf643a4f97R153

broken link

sorry for that, I do not want to pass again the link, because it will be broken on the next push, but you can search for :

SecurityContext: &corev1.PodSecurityContext{ RunAsNonRoot: pointer.BoolPtr(true), RunAsUser: &user, },

frobware · 2019-04-16T08:07:07Z

pkg/operator/sync.go

+			PriorityClassName: "system-node-critical",
+			NodeSelector:      map[string]string{"node-role.kubernetes.io/master": ""},
+			SecurityContext: &corev1.PodSecurityContext{
+				RunAsNonRoot: &_true,


Prefer k8s.io/utils/pointer.BoolPtr(true).

// BoolPtr returns a pointer to a bool func BoolPtr(b bool) *bool { return &b }

frobware

What do we see as the advantage of code over a declarative template?

cynepco3hahue · 2019-04-16T08:22:35Z

What do we see as the advantage of code over a declarative template?

Because it gives a more flexible way to update the deployment in the case of different feature gates

enxebre · 2019-04-16T08:28:00Z

pkg/operator/operator_test.go

+)
+
+var (
+	knownDate   = metav1.Time{Time: time.Date(1985, 06, 03, 0, 0, 0, 0, time.Local)}


where is knownDate used?

Thanks, missed it, remaining from copy-paste

enxebre · 2019-04-16T08:28:15Z

pkg/operator/operator_test.go

+var (
+	knownDate   = metav1.Time{Time: time.Date(1985, 06, 03, 0, 0, 0, 0, time.Local)}
+	alwaysReady = func() bool { return true }
+	noTimestamp = metav1.Time{}


where is noTimestamp used?

enxebre · 2019-04-16T08:56:37Z

pkg/operator/operator_test.go

+
+	kubeActions := filterInformerActions(f.kubeClient.Actions())
+	for i, action := range kubeActions {
+		if len(f.kubeActions) < i+1 {


couldn't we just check once len(kubeActions) !-= len (f.kubeActions)?
can we call this may be actualKubeActions for easier readibility?

I prefer to have to checks under the unittest to make debugging easier, but I will rename variables

frobware · 2019-04-16T09:05:42Z

Because it gives a more flexible way to update the deployment in the case of different feature gates

For 4.1 do we have this requirement?

cynepco3hahue · 2019-04-16T09:44:22Z

For 4.1 do we have this requirement?

At least from what I understood from @enxebre, it is one of the requirements to introduce new controllers under the machine-api-operator.

ingvagabund · 2019-04-16T10:43:05Z

cmd/machine-api-operator/start.go

@@ -75,6 +81,16 @@ func runStartCmd(cmd *cobra.Command, args []string) {
 }

 func startControllers(ctx *ControllerContext) error {


Can you create additional function initRecorder:

func initRecorder(kubeClient kubernetes.Interface) eventBroadcaster.EventRecorder { eventRecorderScheme := runtime.NewScheme() osconfigv1.Install(eventRecorderScheme) eventBroadcaster := record.NewBroadcaster() eventBroadcaster.StartLogging(glog.Infof) eventBroadcaster.StartRecordingToSink(&coreclientsetv1.EventSinkImpl{Interface: kubeClient.CoreV1().Events("")}) return eventBroadcaster.NewRecorder(eventRecorderScheme, v1.EventSource{Component: "machineapioperator"}) }

, change startControllers implementation into:

func startControllers(ctx *ControllerContext) { kubeClient := ctx.ClientBuilder.KubeClientOrDie(componentName) recorder := initRecorder(kubeClient) go operator.New( componentNamespace, componentName, startOpts.imagesFile, config, ctx.KubeNamespacedInformerFactory.Apps().V1().Deployments(), kubeClient, ctx.ClientBuilder.OpenshiftClientOrDie(componentName), recorder, ).Run(2, ctx.Stop) }

and replace:

if err := startControllers(ctrlCtx); err != nil { glog.Fatalf("error starting controllers: %v", err) }

with startControllers(ctrlCtx)?

startControllers always returns nil so it does not make sense to check for err != nil and fail.

startControllers did not change

sorry forgot that I disabled auto-save option

Thank you. Can you also remove error return type of the function? And remove return nil?

enxebre · 2019-05-14T10:31:38Z

@cynepco3hahue needs rebase

cynepco3hahue · 2019-05-15T08:09:24Z

@enxebre Rebased, can you take a look please?

enxebre · 2019-05-15T10:58:07Z

pkg/operator/operator_test.go

-				featureGateLister: featureGateInformer.Lister(),
-			}
+			stopCh := make(<-chan struct{})
+			optr := newFakeOperator([]runtime.Object{d}, []runtime.Object{infra}, stopCh)


why are we passing the deployment here? shouldn't the expected behaviour for it to be created by the fake client on sync?
Also can we validate that for noop providers is not created?

cynepco3hahue · 2019-05-15T13:49:37Z

/retest

cynepco3hahue · 2019-05-15T14:11:35Z

@enxebre Can you please review again?

enxebre · 2019-05-15T14:41:26Z

can we now get rid of PopulateTemplate?
over all lgtm PTAL @frobware

cynepco3hahue · 2019-05-15T14:46:27Z

can we now get rid of PopulateTemplate?

Thanks, missed it.

enxebre · 2019-05-16T07:32:57Z

pkg/operator/sync.go

+			TolerationSeconds: pointer.Int64Ptr(120),
+		},
+		{
+			Key:               "node.alpha.kubernetes.io/unreachable",


shouldn't this be - key: "node.kubernetes.io/not-ready"

true, thanks

enxebre · 2019-05-16T07:33:00Z

pkg/operator/sync.go

+			Operator: corev1.TolerationOpExists,
+		},
+		{
+			Key:               "node.alpha.kubernetes.io/not-ready",


shou;ldn't this be key: "node.kubernetes.io/not-ready"?

true, thanks

ingvagabund · 2019-05-16T11:27:03Z

pkg/operator/operator_test.go

 			}

-			err = optr.sync("test-key")
-			if !tc.expectedNoop {


@enxebre IIRC, you authored this unit test. Is it ok to check for !assert.NoError(t, err, "unexpected sync failure") no matter what tc.expectedNoop is set to?

@ingvagabund by the way before the all correct provider's tests failed to deploy, because of missing deployment file, I do not sure if at proper test

pkg/operator/sync.go

ingvagabund · 2019-05-17T09:26:06Z

/approve

openshift-ci-robot · 2019-05-17T09:26:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund

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~~ [ingvagabund]

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

frobware · 2019-05-17T09:51:44Z

pkg/operator/sync.go

+			NodeSelector:      map[string]string{"node-role.kubernetes.io/master": ""},
+			SecurityContext: &corev1.PodSecurityContext{
+				RunAsNonRoot: pointer.BoolPtr(true),
+				RunAsUser:    &user,


For consistency use pointer.Int64Ptr().

frobware

Just the one nit (#280 (comment)), then LGTM.

cynepco3hahue · 2019-05-26T10:30:36Z

/retest

openshift-ci-robot · 2019-05-26T10:33:26Z

@cynepco3hahue: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/actuator-pkg-staleness	`2dec0f6`	link	`/test actuator-pkg-staleness`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

enxebre · 2019-05-27T10:48:39Z

pkg/operator/operator_test.go

 // TestOperatorSync_NoOp tests syncing to ensure that the mao reports available
 // for platforms that are no-ops.
 func TestOperatorSync_NoOp(t *testing.T) {
 	cases := []struct {
-		platform     configv1.PlatformType


for future PRs of this size it be nice if we stick to the necessary changes so avoid unnecessary changes for the sake of easier review and smoother workflow

it necessary change, because this unittest worked before for cases when the deployment manifests does not exist, but not we do not depend on the deployment manifest, so I needed to change the test

sorry I meant the import names, e.g configv1 <-> v1

enxebre · 2019-05-27T10:49:13Z

pkg/operator/operator_test.go

 				}
+				return true, nil
+			})
+


nit: this is worth a comment

enxebre · 2019-05-27T10:49:25Z

/lgtm

openshift-ci-robot requested review from spangenberg and vikaschoudhary16 April 11, 2019 14:46

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 11, 2019

ingvagabund requested changes Apr 14, 2019

View reviewed changes

pkg/operator/sync.go Outdated Show resolved Hide resolved

pkg/operator/sync.go Outdated Show resolved Hide resolved

pkg/operator/sync.go Outdated Show resolved Hide resolved

pkg/operator/sync.go Outdated Show resolved Hide resolved

This was referenced Apr 14, 2019

Feature gate support #279

Closed

Better naming for controllers deployment #278

Merged

cynepco3hahue force-pushed the codify_operator_controller branch from 149b0b3 to 6e84034 Compare April 14, 2019 10:52

cynepco3hahue force-pushed the codify_operator_controller branch from 6e84034 to f565de3 Compare April 15, 2019 14:24

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 16, 2019

enxebre reviewed Apr 16, 2019

View reviewed changes

frobware reviewed Apr 16, 2019

View reviewed changes

frobware requested changes Apr 16, 2019

View reviewed changes

cynepco3hahue force-pushed the codify_operator_controller branch from f565de3 to 4063b78 Compare April 16, 2019 08:24

enxebre reviewed Apr 16, 2019

View reviewed changes

cynepco3hahue force-pushed the codify_operator_controller branch from 4063b78 to 2ed32aa Compare April 16, 2019 09:59

ingvagabund reviewed Apr 16, 2019

View reviewed changes

cynepco3hahue force-pushed the codify_operator_controller branch 2 times, most recently from 2efea56 to c5fde87 Compare April 16, 2019 13:20

cynepco3hahue force-pushed the codify_operator_controller branch from 41c2e59 to f657b42 Compare May 14, 2019 15:53

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 14, 2019

enxebre reviewed May 15, 2019

View reviewed changes

cynepco3hahue force-pushed the codify_operator_controller branch from f657b42 to c346715 Compare May 15, 2019 12:24

cynepco3hahue force-pushed the codify_operator_controller branch from c346715 to 5d45918 Compare May 15, 2019 14:45

enxebre reviewed May 16, 2019

View reviewed changes

cynepco3hahue force-pushed the codify_operator_controller branch from 5d45918 to 2dc673a Compare May 16, 2019 07:38

ingvagabund reviewed May 16, 2019

View reviewed changes

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

frobware reviewed May 17, 2019

View reviewed changes

frobware requested changes May 17, 2019

View reviewed changes

Codify creation of the machine-api-controller operator

2dec0f6

cynepco3hahue force-pushed the codify_operator_controller branch from 2dc673a to 2dec0f6 Compare May 26, 2019 08:55

enxebre reviewed May 27, 2019

View reviewed changes

pkg/operator/operator_test.go

}

return true, nil

})

Copy link

Member

enxebre May 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this is worth a comment

openshift-ci-robot assigned enxebre May 27, 2019

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

openshift-merge-robot merged commit 5cba6ea into openshift:master May 27, 2019

		@@ -75,6 +81,16 @@ func runStartCmd(cmd *cobra.Command, args []string) {
		}

		func startControllers(ctx *ControllerContext) error {

Codify operator controller #280

Codify operator controller #280

Conversation

cynepco3hahue commented Apr 11, 2019

cynepco3hahue commented Apr 11, 2019

ingvagabund left a comment

Choose a reason for hiding this comment

cynepco3hahue commented Apr 15, 2019

cynepco3hahue commented Apr 15, 2019

ingvagabund commented Apr 15, 2019

cynepco3hahue commented Apr 15, 2019

enxebre commented Apr 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frobware left a comment

Choose a reason for hiding this comment

cynepco3hahue commented Apr 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre Apr 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frobware commented Apr 16, 2019

cynepco3hahue commented Apr 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre commented May 14, 2019

cynepco3hahue commented May 15, 2019

enxebre May 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cynepco3hahue commented May 15, 2019

cynepco3hahue commented May 15, 2019

enxebre commented May 15, 2019

cynepco3hahue commented May 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre May 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ingvagabund commented May 17, 2019

openshift-ci-robot commented May 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frobware left a comment • edited Loading

Choose a reason for hiding this comment

cynepco3hahue commented May 26, 2019

openshift-ci-robot commented May 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre commented May 27, 2019

enxebre Apr 16, 2019 •

edited

Loading

enxebre May 15, 2019 •

edited

Loading

enxebre May 16, 2019 •

edited

Loading

frobware left a comment •

edited

Loading

openshift-ci-robot commented May 26, 2019 •

edited

Loading