add OperatorGroup #480

jpeeler · 2018-09-24T21:34:12Z

So far the reconcile loop only looks at the namespace selector and writes the results to the status and an annotation on each deployment.

deploy/chart/templates/14-operatorgroup.crd.yaml

deploy/chart/templates/30_15-operatorgroup.crd.yaml

jpeeler · 2018-09-24T22:07:40Z

pkg/controller/operators/olm/operator.go

+	var b strings.Builder
+	nsCount := len(namespaceList.Items)
+	for i := 0; i < nsCount-1; i++ {
+		fmt.Fprintf(&b, "%v,", namespaceList.Items[i])


This might require .Name here, but maybe there's a better way to do all this in general?

If you don't need to use .Name (which I think we may need to), you can use strings.Join(namespaceList.Items, ",") here as an alternative.

I think it makes sense for namespaces in the status to just be a list of strings. Normally when we do object references we try and include GVK but namespaces are pretty ingrained (e.g. metadata.namespace is just a string on every kube object).

If we make that change, then this part just becomes what @njhale suggested

pkg/controller/operators/olm/operator.go

ecordell · 2018-09-24T21:38:40Z

deploy/chart/templates/14-operatorgroup.crd.yaml

+    controller-tools.k8s.io: "1.0"
+  name: operatorgroups.operators.coreos.com.operators.coreos.com
+spec:
+  group: operators.coreos.com.operators.coreos.com


this looks like a weird merge? should be operators.coreos.com

ecordell · 2018-09-24T21:39:29Z

deploy/chart/templates/14-operatorgroup.crd.yaml

+      - metadata
+      - spec
+  version: v1alpha2
+status:


don't commit the status (it will be ingored anyway, but no reason to have it)

ecordell · 2018-09-26T14:05:04Z

pkg/api/apis/operators/v1alpha2/operatorgroup_types.go

+
+type OperatorGroupSpec struct {
+	Selector       metav1.LabelSelector  `json:"selector"`
+	ServiceAccount corev1.ServiceAccount `json:"serviceAccount"`


I think we want both of these to be omittempty

The reason being that if the selector is not present, that all namespaces are selected by default? What account would be used in the absence of a specified service account?

Yep, and in absence of a serviceaccount we'd use the one that OLM is running as (system:controller:operator-lifecycle-manager)

ecordell · 2018-09-26T14:05:34Z

pkg/api/apis/operators/v1alpha2/operatorgroup_types.go

+}
+
+type OperatorGroupStatus struct {
+	Namespaces []corev1.Namespace `json:"namespaces"`


We probably want a "last updated time" here

pkg/api/client/informers/externalversions/generic.go

pkg/controller/operators/olm/operator.go

ecordell · 2018-09-26T14:11:40Z

pkg/controller/operators/olm/operator.go

+	var b strings.Builder
+	nsCount := len(namespaceList.Items)
+	for i := 0; i < nsCount-1; i++ {
+		fmt.Fprintf(&b, "%v,", namespaceList.Items[i])


I think it makes sense for namespaces in the status to just be a list of strings. Normally when we do object references we try and include GVK but namespaces are pretty ingrained (e.g. metadata.namespace is just a string on every kube object).

If we make that change, then this part just becomes what @njhale suggested

pkg/controller/operators/olm/operator.go

jpeeler · 2018-09-27T21:02:45Z

The failing images test will pass once #489 is merged.

jpeeler · 2018-09-28T14:28:55Z

/test images

ecordell · 2018-09-28T19:34:55Z

pkg/controller/operators/olm/operator.go

+	nsCount := len(namespaceList.Items)
+
+	if len(op.Status.Namespaces) == nsCount {
+		for i, v := range namespaceList.Items {


I think it would be better to implement this as a set (map[string]struct{}{}) instead of a list; I'm not sure if we get order guarantees from List

ecordell · 2018-09-28T19:35:51Z

pkg/controller/operators/olm/operator.go

+
+		for _, deploy := range strategyDetailsDeployment.DeploymentSpecs {
+			deploy.Spec.Template.Annotations["olm.targetNamespaces"] = nsList.String()
+			_, err := a.client.Operators().ClusterServiceVersions(currentNamespace).Update(csv)


We'll have a better chance of this succeeding if we send a Patch update.

ecordell · 2018-09-28T19:40:40Z

pkg/controller/operators/olm/operator.go

+			if err != nil {
+				return fmt.Errorf("CSV update for '%v' failed: %v\n", csvName, err)
+			}
+			a.requeueCSV(csvName, currentNamespace)


this will requeue the same csv multiple times if there're multiple deployments in the same csv

I'm also not sure we need to requeue - if this update causes the csv deployment to go unhealthy, we should detect that as an issue in the CSV anyway.

pkg/controller/operators/olm/operator.go

ecordell · 2018-09-28T19:53:54Z

pkg/controller/operators/olm/operator.go

+		}
+	DoUpdate:
+		if needsUpdate {
+			if err := a.updateDeploymentAnnotation(&op); err != nil {


this is going to update for every single namespace that's new, I don't think we want it to be in the loop. that way you can just break the loop and don't need a label/goto

pkg/controller/operators/olm/operator.go

ecordell · 2018-10-01T19:51:19Z

pkg/controller/operators/olm/operator.go

+		return err
+	}
+	operatorGroupOpts := metav1.ListOptions{LabelSelector: selector.String()}
+	// TODO(jpeeler): this needs to use user impersonation with op.Spec.ServiceAccount


nit: remove this comment (I don't think we'll want to use impersonation here)

ecordell · 2018-10-01T19:54:35Z

pkg/controller/operators/olm/operator.go

+
+	nsCount := len(namespaceList.Items)
+
+	if len(op.Status.Namespaces) == nsCount {


I think it would make sense to pull this out into a helper like namespacesChanged(clusterNamespaces, statusNamespaces) bool

ecordell · 2018-10-01T19:56:22Z

pkg/controller/operators/olm/operator.go

+	op.Status.LastUpdated = timeNow()
+
+	// make string list made up of comma separated namespaces
+	var nsList strings.Builder


why don't we pull out the list of namespaces as just a list of strings before we get here? then you can just strings.join() here

and actually if you you make a new type like type NamespaceNameList []string, you can implement the Stringer interface and have it call strings.join so that this reads more nicely

ecordell · 2018-10-01T19:59:08Z

pkg/controller/operators/olm/operator.go

+		managerPolicyRules := []rbacv1.PolicyRule{}
+		apiEditPolicyRules := []rbacv1.PolicyRule{}
+		apiViewPolicyRules := []rbacv1.PolicyRule{}
+		for _, owned := range csv.Spec.CustomResourceDefinitions.Owned {


you also want to range over csv.Spec.ApiServiceDefinitions which hold descriptions of APIs provided by aggregated apiservers.

ecordell · 2018-10-01T20:07:12Z

pkg/controller/operators/olm/operator.go

+			return fmt.Errorf("could not assert strategy implementation as deployment for CSV %s", csvName)
+		}
+
+		managerPolicyRules := []rbacv1.PolicyRule{}


the rbac generation is broadly doing the right thing, but the installplan takes care of some of this already:

for any CRD it installs, it creates a

<that-crd>-edit ClusterRole

<that-crd>-view ClusterRole

All that's really left for this control loop to do is make sure there's an <operatorgroup>-edit and an <operatorgroup>-view ClusterRole available (which I see that you have!)

So I think most of this will remain the same, you just don't need to worry about the specific edit/view clusterroles

ecordell · 2018-10-01T20:11:23Z

pkg/controller/operators/olm/operator.go

+		if err != nil {
+			return err
+		}
+		for _, sa := range strategyDetailsDeployment.Permissions {


These generated roles aren't for the serviceaccount for the operator to use, they're for end users and administrators.

It's so that a cluster admin can bind jpeeler to etcd-edit in namespace foo to give access to operator-provided services.

(we will need a separate thing to ensure that the serviceaccount for the operator has the correct permissions in target namespaces)

ecordell · 2018-10-03T14:22:31Z

pkg/controller/operators/olm/operator.go

+					Reason:         v1alpha1.CSVReasonCopied,
+					LastUpdateTime: timeNow(),
+				}
+				newCSV.Annotations["OriginalCSV"] = fmt.Sprintf("Namespace:%v, ResourceVersion:%v", csv.GetNamespace(), csv.GetResourceVersion())


👍 can we make this more machine readable <namespace>/<csvname>?

also we should follow the annotation key convention we used before, I think it would be olm.originalCSV?

pkg/controller/operators/olm/operator.go

jpeeler · 2018-10-12T18:34:48Z

@ecordell I'm working on rebasing next, but I just pushed the simple e2e and associated fixes.

csvs in target namespaces should be annotated

also gets rid of stale code

jpeeler · 2018-11-01T16:22:45Z

/retest

jpeeler · 2018-11-01T16:59:09Z

/retest
currently timing out on yum update failures

pkg/lib/queueinformer/queueinformer.go

pkg/controller/operators/olm/operator.go

jpeeler · 2018-11-02T20:18:56Z

/retest

Looks like CI operator is having a bad day:

panic: runtime error: index out of range

goroutine 135 [running]:
github.com/openshift/ci-operator/pkg/steps.waitForPodCompletion(0x1285700, 0xc4204b55c0, 0xc4204ac330, 0xe, 0x1275cc0, 0xc4202aa2d0, 0x7ffe49a0613e, 0xf, 0x0, 0xc420859520)
	/go/src/github.com/openshift/ci-operator/pkg/steps/template.go:572 +0x3f7
github.com/openshift/ci-operator/pkg/steps.(*podStep).Run(0xc4205aa1c0, 0x1275a40, 0xc4200517c0, 0x0, 0x5, 0xc42036a400)
	/go/src/github.com/openshift/ci-operator/pkg/steps/test.go:120 +0xc4f
github.com/openshift/ci-operator/pkg/steps/release.(*assembleReleaseStep).Run(0xc4204315e0, 0x1275a40, 0xc4200517c0, 0xbeef49f166006400, 0x65f0f3ff12, 0x1974400)
	/go/src/github.com/openshift/ci-operator/pkg/steps/release/create_release.go:83 +0x773
github.com/openshift/ci-operator/pkg/steps.runStep(0x1275a40, 0xc4200517c0, 0xc420213140, 0xc420322f00, 0x0)
	/go/src/github.com/openshift/ci-operator/pkg/steps/run.go:105 +0x92
created by github.com/openshift/ci-operator/pkg/steps.Run
	/go/src/github.com/openshift/ci-operator/pkg/steps/run.go:71 +0x3a1```

njhale · 2018-11-05T19:47:47Z

pkg/controller/operators/olm/operator.go

+	client              versioned.Interface
+	resolver            install.StrategyResolverInterface
+	lister              operatorlister.OperatorLister
+	csvLister           map[string]csvlister.ClusterServiceVersionLister


After some fixes in PR-536 get in, we can update this to extend operatorlister.OperatorLister.

njhale

Looks great overall! A few minor things:

pkg/controller/operators/olm/operator.go

njhale · 2018-11-05T20:27:39Z

pkg/controller/operators/olm/operator.go

@@ -21,6 +24,7 @@ import (
 	"k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"


nit: import ordering

Can you elaborate? I just always let goimports do its thing.

This doesn't look split up correctly. It should be:

import( standard packages external stuff internal stuff )

From what I can see, it looks like:

import ( standard packages internal stuff external stuff )

pkg/controller/operators/olm/operator.go

pkg/controller/operators/olm/operator_test.go

pkg/controller/operators/olm/operatorgroup.go

njhale · 2018-11-05T21:59:03Z

pkg/controller/operators/olm/operatorgroup.go

+			}
+		}
+		if k8serrors.IsAlreadyExists(err) {
+			fetchedCSV, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(ns.Name).Get(csv.GetName(), metav1.GetOptions{})


If you check for a pre-existing CSV before attempting to create or update, you can make use of the lister to avoid making an extra API call.

Instead of creating operator groups in a "second phase", create the resources at the same time along with everything else. This removes a small amount of namespace specific code from NewFakeOperator and makes things more consistent.

return error as the last argument

This is already handled in a more clear way in removeDanglingChildCSVs.

Essentially, optimize for the update case rather than create since creates only happen once.

ecordell · 2018-11-06T22:49:11Z

deploy/chart/templates/0000_30_14-operatorgroup.crd.yaml

+        spec:
+          properties:
+            selector:
+              type: object


We should be explicit about accepting a label selector here, see: https://github.com/operator-framework/operator-lifecycle-manager/blob/master/deploy/chart/templates/0000_30_02-clusterserviceversion.crd.yaml#L153-L182

ecordell · 2018-11-06T22:50:48Z

pkg/api/apis/operators/v1alpha2/operatorgroup_types.go

+}
+
+type OperatorGroupStatus struct {
+	Namespaces  []*corev1.Namespace `json:"namespaces"`


I'm still a little concerned that this could get too long. []string seems slightly safer. Perhaps the real answer is to wait for CRDs to support arbitrary subresources and implement a /namespaces subresource (so that we can paginate)

ecordell · 2018-11-06T22:54:06Z

pkg/controller/operators/olm/operator_test.go

-	_, err := opClientFake.KubernetesInterface().CoreV1().Namespaces().Create(&v1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespace}})
-	if err != nil {
-		return nil, err
+	for _, ns := range namespaces {


not a big deal, but if you're passing the objects in like this you can just append the namespaces to k8sObjs and they'll exist in the fake for testing. or you can remove the explicit namespaces param and build the object list in the tests

jpeeler · 2018-11-07T01:42:26Z

/test e2e-aws-olm

ecordell · 2018-11-07T14:34:11Z

/approve
/lgtm

openshift-ci-robot · 2018-11-07T14:36:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ecordell

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

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

to be handled follow ups

jpeeler · 2018-11-07T15:28:25Z

/test e2e-aws-olm

…roup add OperatorGroup

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 24, 2018

openshift-ci-robot requested review from ecordell and njhale September 24, 2018 21:34

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 24, 2018

njhale reviewed Sep 24, 2018

View reviewed changes

deploy/chart/templates/14-operatorgroup.crd.yaml Outdated Show resolved Hide resolved

jpeeler force-pushed the add-operator-group branch from a4e3db4 to 5be1916 Compare September 24, 2018 21:56

njhale reviewed Sep 24, 2018

View reviewed changes

deploy/chart/templates/30_15-operatorgroup.crd.yaml Outdated Show resolved Hide resolved

jpeeler commented Sep 24, 2018

View reviewed changes

njhale reviewed Sep 24, 2018

View reviewed changes

pkg/controller/operators/olm/operator.go Outdated Show resolved Hide resolved

njhale reviewed Sep 25, 2018

View reviewed changes

pkg/controller/operators/olm/operator.go Outdated Show resolved Hide resolved

ecordell requested changes Sep 26, 2018

View reviewed changes

jpeeler force-pushed the add-operator-group branch 2 times, most recently from f58875a to 788d864 Compare September 27, 2018 17:54

ecordell requested changes Sep 28, 2018

View reviewed changes

jpeeler force-pushed the add-operator-group branch 3 times, most recently from 2c80629 to 1e96092 Compare October 1, 2018 17:05

ecordell requested changes Oct 1, 2018

View reviewed changes

jpeeler force-pushed the add-operator-group branch 2 times, most recently from 5d3c426 to abc9379 Compare October 2, 2018 19:32

ecordell reviewed Oct 3, 2018

View reviewed changes

njhale reviewed Oct 4, 2018

View reviewed changes

pkg/controller/operators/olm/operator.go Outdated Show resolved Hide resolved

jpeeler force-pushed the add-operator-group branch 3 times, most recently from 09ef2c4 to bb9d5fa Compare October 12, 2018 18:26

jpeeler force-pushed the add-operator-group branch 2 times, most recently from 084c6f0 to ec652c6 Compare October 15, 2018 17:35

ecordell and others added 5 commits November 1, 2018 12:01

WIP: cvs in operator namespace are being annotated,

2af56a7

csvs in target namespaces should be annotated

fix(olm): refactor e2e and check annotations

22a29b3

fix(olm): set event handler functions correctly

ae457b3

also gets rid of stale code

fix(e2e): slightly improves execution time

d7229f1

fix(olm): operator group annotations and listers

5b7d140

jpeeler force-pushed the add-operator-group branch from f5c3a60 to 5b7d140 Compare November 1, 2018 16:05

njhale reviewed Nov 1, 2018

View reviewed changes

pkg/lib/queueinformer/queueinformer.go Show resolved Hide resolved

ecordell requested changes Nov 2, 2018

View reviewed changes

pkg/controller/operators/olm/operator.go Outdated Show resolved Hide resolved

fix(olm): do not check namespace if casting failed

ac50ddc

njhale reviewed Nov 5, 2018

View reviewed changes

njhale previously requested changes Nov 5, 2018

View reviewed changes

Jeff Peeler added 4 commits November 6, 2018 15:48

fix(unit): create OperatorGroups with initial fakes

cd201c5

Instead of creating operator groups in a "second phase", create the resources at the same time along with everything else. This removes a small amount of namespace specific code from NewFakeOperator and makes things more consistent.

chore(olm): refactor updateNamespaceList

337b156

return error as the last argument

fix(olm): remove duplicated code

ab27044

This is already handled in a more clear way in removeDanglingChildCSVs.

fix(olm): make operator group copy CSVs more efficiently

b49c9bc

Essentially, optimize for the update case rather than create since creates only happen once.

ecordell reviewed Nov 6, 2018

View reviewed changes

openshift-ci-robot assigned ecordell Nov 7, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2018

ecordell merged commit 17359ee into operator-framework:master Nov 7, 2018

ecordell added a commit to ecordell/operator-lifecycle-manager that referenced this pull request Mar 8, 2019

Merge pull request operator-framework#480 from jpeeler/add-operator-g…

bd321e0

…roup add OperatorGroup

njhale added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 19, 2019


		nsCount := len(namespaceList.Items)

		if len(op.Status.Namespaces) == nsCount {

		@@ -21,6 +24,7 @@ import (
		"k8s.io/api/apps/v1"
		corev1 "k8s.io/api/core/v1"

add OperatorGroup #480

add OperatorGroup #480

Conversation

jpeeler commented Sep 24, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpeeler commented Sep 27, 2018

jpeeler commented Sep 28, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpeeler commented Oct 12, 2018

jpeeler commented Nov 1, 2018

jpeeler commented Nov 1, 2018

jpeeler commented Nov 2, 2018

Choose a reason for hiding this comment

njhale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njhale Nov 6, 2018 • 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

Choose a reason for hiding this comment

jpeeler commented Nov 7, 2018

ecordell commented Nov 7, 2018

openshift-ci-robot commented Nov 7, 2018

jpeeler commented Nov 7, 2018

njhale Nov 6, 2018 •

edited

Loading