OCP4 OLM integration and visual display in OLM deloyment status page #28

myeung18 · 2019-06-13T19:25:05Z

Signed-off-by: Marco Yeung [email protected]

OLM integration on OCP4
operator status display fixes for OLM
add few custom resource examples for OLM and/or CLI

bmozaffa · 2019-06-16T21:59:13Z

pkg/controller/interconnect/interconnect_controller.go

-	for _, pod := range pods {
-		if pod.GetObjectMeta().GetDeletionTimestamp() == nil {
-			podNames = append(podNames, pod.Name)
+func getPodStatus(r *ReconcileInterconnect, instance *v1alpha1.Interconnect) v1alpha1.InterconnectPodStatus {


@myeung18 I've moved this logic to the shared library now, please refactor to use it as done in corresponding PR

refactoring is done

grs · 2019-06-26T19:00:08Z

Could we separate out the examples from the OLM fixes? I'm hesitant to include a lot of different examples without any verification as it becomes easy for them to go stale. I think they also need some explanation, so perhaps worth doing in conjunction with some improved docs.

In any case they seem a separate issue from the OLM fixes.

myeung18 · 2019-06-26T19:18:01Z

Could we separate out the examples from the OLM fixes? I'm hesitant to include a lot of different examples without any verification as it becomes easy for them to go stale. I think they also need some explanation, so perhaps worth doing in conjunction with some improved docs.

In any case they seem a separate issue from the OLM fixes.

Sure, let me separate those examples from OLM changes.

update: merge has split out, this branch only has the OLM related changes.

grs · 2019-07-12T18:29:00Z

deploy/olm-catalog/qdr-operator/0.1.0/catalog-source.yaml

+                    - "urn:alm:descriptor:com.tectonic.ui:label"
+                - description: Listeners for incoming connections to the router
+                  displayName: Listener Port
+                  path: listeners.port


I don't think displaying the first listener port is particularly useful. There doesn't seem to be a way to handle lists effectively yet. From what I can see this mainly affects the overview page in the openshift console. I'd be inclined to keep things simple and just display the role and placement (since those are really the defining characteristics of the deployment) for the time being.

we will display role and placement. I took out others.

grs · 2019-07-12T18:30:25Z

deploy/olm-catalog/qdr-operator/0.1.0/catalog-source.yaml

                statusDescriptors:
                - description: The current revision of the Interconnect cluster
                  displayName: Revision Number
                  path: revNumber
                - description: The current pods
                  displayName: Pods
                  path: pods
-                - description: The current conditions
-                  displayName: Conditions


I think the conditions are worth keeping (though I think the operator's use of them needs to be improved a little). There is an x-descriptor available that will display them.

Conditions added back. Please refer to the pic above

grs · 2019-07-12T18:40:12Z

pkg/controller/interconnect/interconnect_controller.go

-	}
-	podNames := getPodNames(podList.Items)
+	reqLogger.Info("Collecting Pod Status")
+	dStatus := getPodStatus(r, instance)


I'm not quite following the logic here. It looks like the pod statuses are retrieved for whichever placement option is used, but then it is ignored unless placement is 'Every'? Or am I missing something?

That was not good, agree.
I change to check with DeploymentStatus.Starting slice. If the Starting slice is not empty, we will continue the loop.

@myeung18 Why? What does starting have to do with it?

Starting mean there are pods that is still creating/starting and they are not in ready status. So we should continue the loop, unless there is better way to do so.

That does not seem correct to me. Starting means a specific state. If the pod starts faster, you may never see it. After it has started, it goes away, which probably indicates a bug in your code right now. Please test thoroughly to make sure you get the right behavior.

if the pods status is somehow changed, that means it is different from the existing podstaus in instance object, this should fall to the or condition check where both statuses are compared to determine if the status should be updated.

Oh right, it's an or. So why do you need the first condition?

The daemonset pod creation somehow is slower. After the initial update and requeue. The controller will get exact same status data with all pods in starting state. This event won’t get the chance to enter the if block. The If block is not executed and the requeue is not called. Then no status update event comes in even after all the pods are created. Adding this starting slice check meanly wants to have that requeue get called.

That's not the expected flow. Instead, you should watch created daemon set objects. That would then ensure that any changes to the deployment is reflected in the OLM status.

Changed to watch DaemonSet and removed that Starting slice check. The Controller will now update deployment pod status if there is newer status comes: new and old status objects are compared.

grs · 2019-07-12T18:41:04Z

pkg/controller/interconnect/interconnect_controller.go

+	// List the pods for this deployment
+	var status olm.DeploymentStatus
+	depFound := &appsv1.Deployment{}
+	err := r.client.Get(context.TODO(), types.NamespacedName{Name: instance.Name, Namespace: instance.Namespace}, depFound)


It might be cleaner to use the placement value to determine which method to call for the pod statuses?

myeung18 · 2019-07-15T20:05:22Z

I have a new change(or fix) in this branch. A one line change below fixes the issue when we create the operator in cluster scope and it failed to receive events from correct namespaces to create resources. This change will make operator work in cluster scope as well as singe namespace scope.

https://github.com/myeung18/qdr-operator/blob/db2e3bae3870bf9d8f46f2d29d5b36bb943c5d58/deploy/olm-catalog/qdr-operator/0.1.0/qdr-operator.v0.1.0.clusterserviceversion.yaml#L156

myeung18 · 2019-07-16T20:34:56Z

clusterPermission is added so that interconnect can query cert-manager through OLM deployment. However, this clusterPermission works only in OLM 0.8.1 or above.
https://github.com/myeung18/qdr-operator/blob/195b3072da72858a59a90e5f1104c4f646eff2b1/deploy/olm-catalog/qdr-operator/0.1.0/qdr-operator.v0.1.0.clusterserviceversion.yaml#L118-L135

To validate the existence of cert-manager, a recommended way should be using discovery api, which doesn't need the cluster role binding.
example: https://github.com/operator-framework/operator-lifecycle-manager/blob/master/pkg/lib/operatorstatus/clusteroperatorwriter.go#L72

Signed-off-by: Marco Yeung <[email protected]>

bmozaffa suggested changes Jun 16, 2019

View reviewed changes

myeung18 force-pushed the ocpfourolm branch 7 times, most recently from 345f942 to d541fe0 Compare June 21, 2019 19:31

myeung18 changed the title ~~OCP4 OLM integration and visual display in OLM deloyment status page~~ [WIP] OCP4 OLM integration and visual display in OLM deloyment status page Jun 21, 2019

myeung18 force-pushed the ocpfourolm branch from d541fe0 to 05a4bd2 Compare June 25, 2019 21:09

myeung18 changed the title ~~[WIP] OCP4 OLM integration and visual display in OLM deloyment status page~~ OCP4 OLM integration and visual display in OLM deloyment status page Jun 25, 2019

myeung18 force-pushed the ocpfourolm branch 2 times, most recently from db09570 to 87bc599 Compare July 1, 2019 18:10

grs reviewed Jul 12, 2019

View reviewed changes

myeung18 force-pushed the ocpfourolm branch from 87bc599 to d13b1b9 Compare July 15, 2019 14:35

myeung18 changed the title ~~OCP4 OLM integration and visual display in OLM deloyment status page~~ WIP - OCP4 OLM integration and visual display in OLM deloyment status page Jul 15, 2019

myeung18 force-pushed the ocpfourolm branch 2 times, most recently from 8ea7e2e to db2e3ba Compare July 15, 2019 19:58

myeung18 force-pushed the ocpfourolm branch from db2e3ba to 195b307 Compare July 16, 2019 20:28

myeung18 force-pushed the ocpfourolm branch from 195b307 to afea61d Compare July 17, 2019 18:13

myeung18 changed the title ~~WIP - OCP4 OLM integration and visual display in OLM deloyment status page~~ OCP4 OLM integration and visual display in OLM deloyment status page Sep 16, 2019

myeung18 force-pushed the ocpfourolm branch from afea61d to 735e4e4 Compare October 1, 2019 15:07

grs approved these changes Oct 2, 2019

View reviewed changes

OCP4 OLM integration and visual display in OLM deloyment status page

35601d3

Signed-off-by: Marco Yeung <[email protected]>

myeung18 force-pushed the ocpfourolm branch from 735e4e4 to 35601d3 Compare October 7, 2019 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCP4 OLM integration and visual display in OLM deloyment status page #28

OCP4 OLM integration and visual display in OLM deloyment status page #28

myeung18 commented Jun 13, 2019

bmozaffa Jun 16, 2019

myeung18 Jun 25, 2019 •

edited

Loading

grs commented Jun 26, 2019

myeung18 commented Jun 26, 2019 •

edited

Loading

grs Jul 12, 2019

myeung18 Jul 15, 2019

grs Jul 12, 2019

myeung18 Jul 15, 2019 •

edited

Loading

grs Jul 12, 2019

myeung18 Jul 16, 2019 •

edited

Loading

bmozaffa Jul 16, 2019

myeung18 Jul 16, 2019 •

edited

Loading

bmozaffa Jul 16, 2019

myeung18 Jul 16, 2019

bmozaffa Jul 16, 2019

myeung18 Jul 16, 2019

bmozaffa Jul 16, 2019

myeung18 Jul 17, 2019

grs Jul 12, 2019

myeung18 Jul 16, 2019

myeung18 commented Jul 15, 2019 •

edited

Loading

myeung18 commented Jul 16, 2019 •

edited

Loading

OCP4 OLM integration and visual display in OLM deloyment status page #28

Are you sure you want to change the base?

OCP4 OLM integration and visual display in OLM deloyment status page #28

Conversation

myeung18 commented Jun 13, 2019

Choose a reason for hiding this comment

myeung18 Jun 25, 2019 • edited Loading

Choose a reason for hiding this comment

grs commented Jun 26, 2019

myeung18 commented Jun 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

myeung18 Jul 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myeung18 Jul 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myeung18 Jul 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

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

myeung18 commented Jul 15, 2019 • edited Loading

myeung18 commented Jul 16, 2019 • edited Loading

myeung18 Jun 25, 2019 •

edited

Loading

myeung18 commented Jun 26, 2019 •

edited

Loading

myeung18 Jul 15, 2019 •

edited

Loading

myeung18 Jul 16, 2019 •

edited

Loading

myeung18 Jul 16, 2019 •

edited

Loading

myeung18 commented Jul 15, 2019 •

edited

Loading

myeung18 commented Jul 16, 2019 •

edited

Loading