run bundle: Verify operator is installed/upgraded by bharathi-tenneti · Pull Request #3766 · operator-framework/operator-sdk

bharathi-tenneti · 2020-08-20T15:21:52Z

Description:
Verification logic will handle verifying the operator is installed beginning with the starting CSV and upgraded through the ending CSV.
Log what happened and final state (e.g. operator is running in namespace foo, watching namespace bar.
Use CSV status and deployment status as a way to verify the operator.

jianzhangbjz · 2020-08-24T06:30:47Z

+	}
+
+	if csvKey.Name == "" {
+		return v1alpha1.InstallPlanFailed, fmt.Errorf("could not find installed CSV in install plan")


Seems like here should return the CSV phase, not the InstallPlan's.

jianzhangbjz · 2020-08-24T06:54:11Z

+			return true, nil
+		}
+		return false, nil
+	}, ctx.Done()); err != nil {


I'm confused here, when failed to get the CSV, why still need to check the Deployment? Based on my understanding, both CSV and deployment work well, we can say the operator is installed successfully. Anyone of them failed, we can say the operator failed to install.

As I understand, this is to get the deployment reason for failure.
@joelanford Is that what you were suggesting?

The idea is that if the CSV never succeeds, we should try to be helpful and tell a user why the CSV didn't succeed.

One common reason is that the operator deployment hasn't rolled out.

I think we should only check the deployment status if the error from the wait.PollImmediateUntil call is context.DeadlineExceeded.

Thank you! I see now, yeah, we should try to return the helpful info. Besides the Deployment object, the rule policy(role, clusterrole, rolebinding ...) leads to many failures.

jianzhangbjz · 2020-08-24T06:56:24Z

@bharathi-tenneti Could you help add the unit test or e2e test for this feature? Thanks!

camilamacedo86 · 2020-08-24T11:08:26Z

Hi @bharathi-tenneti,

@bharathi-tenneti Could you help add the unit test or e2e test for this feature? Thanks!
+1

IMO: We ought to call the command in :

here to ensure that it works for Ansible.
here to ensure that it works for Helm
here to ensure that it works for Go.

camilamacedo86 · 2020-08-24T11:08:53Z

@@ -17,9 +17,13 @@ package internal
 import (


Missing fragment since it is an aditional of a new command.

bharathi-tenneti · 2020-08-26T13:44:28Z

@bharathi-tenneti Could you help add the unit test or e2e test for this feature? Thanks!

e2e tests and unit tests are coming in this sprint.

joelanford · 2020-08-26T17:22:35Z

+		for _, s := range dep.Status.Conditions {
+			if s.Type != "Progressing" && s.Type != "Available" {
+				return nil, fmt.Errorf("Deployment failed: %v", s.Message)
+			}
+		}


I ran a quick test by creating a deployment with an image I know doesn't exist. The deployment conditions I see are:

conditions: - lastTransitionTime: "2020-08-26T17:11:29Z" lastUpdateTime: "2020-08-26T17:11:29Z" message: Deployment does not have minimum availability. reason: MinimumReplicasUnavailable status: "False" type: Available - lastTransitionTime: "2020-08-26T17:11:29Z" lastUpdateTime: "2020-08-26T17:11:29Z" message: ReplicaSet "test-dep-97bfdcd79" is progressing. reason: ReplicaSetUpdated status: "True" type: Progressing

And the underlying pod has this:

conditions: - lastProbeTime: null lastTransitionTime: "2020-08-26T17:11:29Z" status: "True" type: Initialized - lastProbeTime: null lastTransitionTime: "2020-08-26T17:11:29Z" message: 'containers with unready status: [test-fake-jwl-image]' reason: ContainersNotReady status: "False" type: Ready - lastProbeTime: null lastTransitionTime: "2020-08-26T17:11:29Z" message: 'containers with unready status: [test-fake-jwl-image]' reason: ContainersNotReady status: "False" type: ContainersReady - lastProbeTime: null lastTransitionTime: "2020-08-26T17:11:29Z" status: "True" type: PodScheduled containerStatuses: - image: test-fake-jwl-image imageID: "" lastState: {} name: test-fake-jwl-image ready: false restartCount: 0 started: false state: waiting: message: Back-off pulling image "test-fake-jlanford-image" reason: ImagePullBackOff

We probably need to be checking all of the deployment conditions and looking for any of them that are indicative of failure. And then if we find one, perhaps we could try digging further, such that we eventually end up being able to tell the user something like:

failed to run operator: timed out waiting for csv success: deployment unavailable: pod is Pending with ImagePullBackOff: Back-off pulling image "test-fake-jlanford-image"

@joelanford So, DeploymentConditions of Progressing and Available are not considered failure. In current code , we are considering as failure, if the condition is anything else

for _, s := range dep.Status.Conditions { if s.Type != "Progressing" && s.Type != "Available" { return nil, fmt.Errorf("Deployment failed: %v", s.Message) }

Currently, I am returning the Message filed in such case. However, as you have suggested to dig deeper, we need to dig into pods, and grab the error message from there. Is that what you meant?

So, DeploymentConditions of Progressing and Available are not considered failure

We have to look at the condition status too. For example, Available = False and Progressing = False mean something's not working correctly.

joelanford · 2020-08-26T17:24:27Z

+				return nil, fmt.Errorf("Deployment failed: %v", s.Message)
+			}
+		}
+	}


If the error is not nil, but we were unable to determine a root cause, we should still return the error, possibly wrapped with some extra context if we know anything else about it.

joelanford · 2020-08-26T17:26:29Z

+	csvKey := types.NamespacedName{
+		Namespace: o.cfg.Namespace,
+	}
+
+	if csvKey.Name == "" {
+		return nil, fmt.Errorf("could not find installed CSV in install plan")
+	}


This will always result in the error being returned right? Should we pass the install plan to this function so we can find the CSV key?

Moved csvKey login into install Plan function.

openshift-ci-robot · 2020-08-27T22:42:13Z

@bharathi-tenneti: PR needs rebase.

Details

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.

bharathi-tenneti requested review from jmrodri, joelanford and rashmigottipati August 20, 2020 15:21

openshift-ci-robot requested a review from hasbro17 August 20, 2020 15:21

bharathi-tenneti changed the title ~~run bundle: Verify operator is installed/upgraded~~ <WIP> run bundle: Verify operator is installed/upgraded Aug 20, 2020

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 Aug 20, 2020

jianzhangbjz reviewed Aug 24, 2020

View reviewed changes

camilamacedo86 reviewed Aug 24, 2020

View reviewed changes

jmrodri mentioned this pull request Aug 24, 2020

Implement run bundle subcommand to improve OLM integration #3626

Closed

14 tasks

bharathi-tenneti force-pushed the runbundle-verify branch from 12d9a86 to 8927719 Compare August 26, 2020 13:57

bharathi-tenneti changed the title ~~<WIP> run bundle: Verify operator is installed/upgraded~~ run bundle: Verify operator is installed/upgraded Aug 26, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 26, 2020

joelanford reviewed Aug 26, 2020

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 27, 2020

bharathi-tenneti closed this Aug 28, 2020

bharathi-tenneti force-pushed the runbundle-verify branch from c6a8772 to d8a7556 Compare August 28, 2020 15:11

bharathi-tenneti deleted the runbundle-verify branch August 28, 2020 18:50

Conversation

bharathi-tenneti commented Aug 20, 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!

jianzhangbjz commented Aug 24, 2020

Uh oh!

camilamacedo86 commented Aug 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharathi-tenneti commented Aug 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharathi-tenneti Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

openshift-ci-robot commented Aug 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bharathi-tenneti Aug 27, 2020 •

edited

Loading