From d987f78f2a02eb155475975ea06373b3d4987fdc Mon Sep 17 00:00:00 2001 From: bharathi-tenneti Date: Thu, 17 Sep 2020 14:43:31 -0400 Subject: [PATCH 1/8] refactor printDeployment and printPodError funcs --- internal/olm/client/client.go | 74 ++++--- internal/olm/client/client_test.go | 298 +++++++++++++++++------------ 2 files changed, 231 insertions(+), 141 deletions(-) diff --git a/internal/olm/client/client.go b/internal/olm/client/client.go index 51ba991f06..66b5e8426e 100644 --- a/internal/olm/client/client.go +++ b/internal/olm/client/client.go @@ -48,6 +48,22 @@ var ErrOLMNotInstalled = errors.New("no existing installation found") var Scheme = scheme.Scheme +// custom error struct to capture deployment errors +// while verifying CSV installs. +type deploymentError struct { + depName string + issue string +} +type deploymentVerifyError []deploymentError + +func (e deploymentVerifyError) Error() string { + var sb strings.Builder + for _, i := range e { + sb.WriteString(i.depName + " has errors: \n" + i.issue + "\n") + } + return sb.String() +} + func init() { if err := olmapiv1alpha1.AddToScheme(Scheme); err != nil { log.Fatalf("Failed to add OLM operator API v1alpha1 types to scheme: %v", err) @@ -223,20 +239,20 @@ func (c Client) DoCSVWait(ctx context.Context, key types.NamespacedName) error { return false, nil } } - err := wait.PollImmediateUntil(time.Second, csvPhaseSucceeded, ctx.Done()) if err != nil && errors.Is(err, context.DeadlineExceeded) { - if depCheckErr := c.printDeploymentErrors(ctx, key, csv); depCheckErr != nil { - return fmt.Errorf("error printing operator resource errors: %v %v", err, depCheckErr) + depCheckErr := c.checkDeploymentErrors(ctx, key, csv) + if _, ok := depCheckErr.(deploymentVerifyError); ok { + return depCheckErr } } return err } -// TODO(btenneti) Refactor function to collect errors into customized error and return. -// printDeploymentErrors function loops through deployment specs of a given CSV, and prints reason +// checkDeploymentErrors function loops through deployment specs of a given CSV, and returns reason // in case of failures, based on deployment condition. -func (c Client) printDeploymentErrors(ctx context.Context, key types.NamespacedName, csv olmapiv1alpha1.ClusterServiceVersion) error { +func (c Client) checkDeploymentErrors(ctx context.Context, key types.NamespacedName, csv olmapiv1alpha1.ClusterServiceVersion) error { + depErr := deploymentVerifyError{} if key.Namespace == "" { return fmt.Errorf("no namespace provided to get deployment failures") } @@ -248,25 +264,39 @@ func (c Client) printDeploymentErrors(ctx context.Context, key types.NamespacedN } depSelectors := ds.Spec.Selector if err := c.KubeClient.Get(ctx, depKey, dep); err != nil { - log.Printf("error getting operator deployment %q: %v", ds.Name, err) + depErr = append(depErr, deploymentError{ + depName: ds.Name, + issue: err.Error(), + }) continue } for _, s := range dep.Status.Conditions { if s.Type == appsv1.DeploymentAvailable && s.Status == corev1.ConditionFalse { - log.Printf("operator deployment %q not available: %s", ds.Name, s.Reason) - if err := c.printPodErrors(ctx, depSelectors, key); err != nil { - return err + podError := c.checkPodErrors(ctx, depSelectors, key) + if podError.Error() == "" { + depErr = append(depErr, deploymentError{ + depName: ds.Name, + issue: s.Reason, + }) + return depErr + } + if _, ok := podError.(deploymentVerifyError); ok { + depErr = append(depErr, deploymentError{ + depName: ds.Name, + issue: podError.Error(), + }) + return depErr } } } } - return nil + return depErr } -// printPodErrors loops through pods, and prints pod errors if any. -func (c Client) printPodErrors(ctx context.Context, depSelectors *metav1.LabelSelector, key types.NamespacedName) error { +// checkPodErrors loops through pods, and returns pod errors if any. +func (c Client) checkPodErrors(ctx context.Context, depSelectors *metav1.LabelSelector, key types.NamespacedName) error { // loop through pods and return specific error message. - podErrors := make(map[string]string) + podErr := deploymentVerifyError{} podList := &corev1.PodList{} podLabelSelectors, err := metav1.LabelSelectorAsSelector(depSelectors) if err != nil { @@ -280,18 +310,16 @@ func (c Client) printPodErrors(ctx context.Context, depSelectors *metav1.LabelSe return fmt.Errorf("error getting Pods: %v", err) } for _, p := range podList.Items { - if p.Status.Phase != corev1.PodSucceeded { - for _, cs := range p.Status.ContainerStatuses { - if !cs.Ready { - podErrors[p.Name] = cs.State.Waiting.Message - } + for _, cs := range p.Status.ContainerStatuses { + if !cs.Ready { + podErr = append(podErr, deploymentError{ + depName: p.Name + cs.Name, + issue: cs.State.Waiting.Message, + }) } } } - if len(podErrors) > 0 { - log.Printf("pod errors: %v\n", podErrors) - } - return nil + return podErr } // GetInstalledVersion returns the OLM version installed in the namespace informed. diff --git a/internal/olm/client/client_test.go b/internal/olm/client/client_test.go index 30225398b0..b45197a790 100644 --- a/internal/olm/client/client_test.go +++ b/internal/olm/client/client_test.go @@ -31,165 +31,227 @@ import ( ) var _ = Describe("Client", func() { - Describe("printDeploymentErrors", func() { + Describe("checkDeploymentErrors", func() { var ( fakeClient client.Client + csv olmapiv1alpha1.ClusterServiceVersion ) BeforeEach(func() { - fakeClient = fake.NewFakeClient( - &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-operator-controller-manager-8687c65f7d-kc44t", - Namespace: "test-operator-system", - Labels: map[string]string{ - "control-plane": "controller-manager", - }, - }, - Status: corev1.PodStatus{ - Phase: corev1.PodRunning, - ContainerStatuses: []corev1.ContainerStatus{ - { - Ready: false, - State: corev1.ContainerState{ - Waiting: &corev1.ContainerStateWaiting{ - Message: "back-off 5m0s restarting failed container)", - Reason: "CrashLoopBackOff", + csv = olmapiv1alpha1.ClusterServiceVersion{ + Spec: olmapiv1alpha1.ClusterServiceVersionSpec{ + DisplayName: "test-operator", + InstallStrategy: olmapiv1alpha1.NamedInstallStrategy{ + StrategySpec: olmapiv1alpha1.StrategyDetailsDeployment{ + DeploymentSpecs: []olmapiv1alpha1.StrategyDeploymentSpec{ + { + Name: "test-operator-controller-manager", + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "control-plane": "controller-manager", + }, + }, + }, + }, + { + Name: "dummy-operator", + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "dummylabel": "dummyvalue", + }, + }, }, }, }, }, }, }, - &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dummypod", - Namespace: "testns", - }, - }, - &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-operator-controller-manager-jjj", - Namespace: "test-operator-system", - Labels: map[string]string{ - "control-plane": "controller-manager", - }, - }, - Status: corev1.PodStatus{ - ContainerStatuses: []corev1.ContainerStatus{ - { - Ready: true, + } + }) + Context("with a valid csv", func() { + It("check error string for pod errors", func() { + fakeClient = fake.NewFakeClient( + + &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-operator-kc44t", + Namespace: "test-operator-system", + Labels: map[string]string{ + "control-plane": "controller-manager", }, }, - }, - }, - - &appsv1.Deployment{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-operator-controller-manager", - Namespace: "test-operator-system", - Labels: map[string]string{ - "control-plane": "controller-manager", + Status: corev1.PodStatus{ + ContainerStatuses: []corev1.ContainerStatus{ + { + Ready: false, + State: corev1.ContainerState{ + Waiting: &corev1.ContainerStateWaiting{ + Message: "back-off 5m0s restarting failed container", + }, + }, + }, + }, }, }, - Status: appsv1.DeploymentStatus{ - Conditions: []appsv1.DeploymentCondition{ - { - Type: "Available", - Status: "False", - Reason: "MinimumReplicasUnavailable", - }, - { - Type: "Progressing", - Status: "True", - Reason: "NewReplicaSetAvailable", + &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-operator-controller-manager", + Namespace: "test-operator-system", + Labels: map[string]string{ + "control-plane": "controller-manager", }, }, - }, - }, - &appsv1.Deployment{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dummy-operator", - Namespace: "dummy-operator-system", - }, - Status: appsv1.DeploymentStatus{ - Conditions: []appsv1.DeploymentCondition{ - { - Type: "Available", - Status: "false", + Status: appsv1.DeploymentStatus{ + Conditions: []appsv1.DeploymentCondition{ + { + Type: "Available", + Status: "False", + }, + { + Type: "Progressing", + Status: "True", + }, }, }, }, - }, - ) - }) - Context("with a valid csv", func() { - It("should validate the csv successfully", func() { + ) key := types.NamespacedName{ Name: "test.operator", Namespace: "test-operator-system", } - csv := olmapiv1alpha1.ClusterServiceVersion{ - Spec: olmapiv1alpha1.ClusterServiceVersionSpec{ - DisplayName: "test-operator", - InstallStrategy: olmapiv1alpha1.NamedInstallStrategy{ - StrategySpec: olmapiv1alpha1.StrategyDetailsDeployment{ - DeploymentSpecs: []olmapiv1alpha1.StrategyDeploymentSpec{ - { - Name: "test-operator-controller-manager", - Spec: appsv1.DeploymentSpec{ - Selector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "control-plane": "controller-manager", - }, - }, + olmclient := Client{KubeClient: fakeClient} + err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) + Expect(err.Error()).To(ContainSubstring("back-off 5m0s restarting failed container")) + }) + + It("check error string for multiple pod failures", func() { + fakeClt := fake.NewFakeClient( + &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-operator-jjj", + Namespace: "test-operator-system", + Labels: map[string]string{ + "control-plane": "controller-manager", + }, + }, + Status: corev1.PodStatus{ + ContainerStatuses: []corev1.ContainerStatus{ + { + Ready: false, + State: corev1.ContainerState{ + Waiting: &corev1.ContainerStateWaiting{ + Message: "Restarting container", }, }, - { - Name: "dummy-operator", - Spec: appsv1.DeploymentSpec{ - Selector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "dummylabel": "dummyvalue", - }, - }, + }, + }, + }, + }, + &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-operator-kkk", + Namespace: "test-operator-system", + Labels: map[string]string{ + "control-plane": "controller-manager", + }, + }, + Status: corev1.PodStatus{ + ContainerStatuses: []corev1.ContainerStatus{ + { + Ready: false, + State: corev1.ContainerState{ + Waiting: &corev1.ContainerStateWaiting{ + Message: "ImageErrPull", }, }, }, }, }, }, - } - olmclient := Client{KubeClient: fakeClient} - err := olmclient.printDeploymentErrors(context.TODO(), key, csv) - Expect(err).To(BeNil()) - - }) - }) - Context("with csv key namespace NOT provided", func() { - It("should error out ", func() { + &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-operator-controller-manager", + Namespace: "test-operator-system", + Labels: map[string]string{ + "control-plane": "controller-manager", + }, + }, + Status: appsv1.DeploymentStatus{ + Conditions: []appsv1.DeploymentCondition{ + { + Type: "Available", + Status: "False", + }, + { + Type: "Progressing", + Status: "True", + }, + }, + }, + }, + ) key := types.NamespacedName{ - Name: "dummy.clusterserviceversion.yaml", + Name: "test.operator", + Namespace: "test-operator-system", } - csv := olmapiv1alpha1.ClusterServiceVersion{ - Spec: olmapiv1alpha1.ClusterServiceVersionSpec{ - DisplayName: "dummy-operator", - InstallStrategy: olmapiv1alpha1.NamedInstallStrategy{ - StrategySpec: olmapiv1alpha1.StrategyDetailsDeployment{ - DeploymentSpecs: []olmapiv1alpha1.StrategyDeploymentSpec{ - { - Name: "dummy-operator", - Spec: appsv1.DeploymentSpec{}, - }, + olmclient := Client{KubeClient: fakeClt} + err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) + Expect(err.Error()).To(ContainSubstring("ImageErrPull")) + Expect(err.Error()).To(ContainSubstring("Restarting container")) + }) + + It("check error string for deployment errors,when no pods exist", func() { + fakeClient = fake.NewFakeClient( + &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-operator-controller-manager", + Namespace: "test-operator-system", + Labels: map[string]string{ + "control-plane": "controller-manager", + }, + }, + Status: appsv1.DeploymentStatus{ + Conditions: []appsv1.DeploymentCondition{ + { + Type: "Available", + Status: "False", + Reason: "Pods not available", }, }, }, }, + ) + key := types.NamespacedName{ + Name: "test-operator", + Namespace: "test-operator-system", + } + olmclient := Client{KubeClient: fakeClient} + err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) + Expect(err.Error()).To(ContainSubstring("Pods not available")) + }) + + It("check error string,when no deployments exist for given CSV", func() { + fakeClient = fake.NewFakeClient() + olmclient := Client{KubeClient: fakeClient} + key := types.NamespacedName{ + Name: "test-operator", + Namespace: "test-operator-system", } + err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) + Expect(err.Error()).To(ContainSubstring("\"test-operator-controller-manager\" not found")) + Expect(err.Error()).To(ContainSubstring("\"dummy-operator\" not found")) + }) + It("check error string,when no namespace provided", func() { + fakeClient = fake.NewFakeClient() olmclient := Client{KubeClient: fakeClient} - err := olmclient.printDeploymentErrors(context.TODO(), key, csv) - Expect(err).ToNot(BeNil()) + key := types.NamespacedName{ + Name: "test-operator", + } + err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) + Expect(err.Error()).To(ContainSubstring("no namespace provided to get deployment failures")) }) }) }) From 8e004b2b5b5864f96508bcd2f97d368f1288e4cf Mon Sep 17 00:00:00 2001 From: bharathi-tenneti Date: Thu, 24 Sep 2020 18:20:38 -0400 Subject: [PATCH 2/8] Addressing PR comments --- .../cmd/operator-sdk/bundle/validate/cmd.go | 4 +- .../operator-sdk/bundle/validate/validate.go | 10 +-- .../bundle/validate/validate_test.go | 5 +- internal/olm/client/client.go | 84 ++++++++----------- internal/olm/client/client_test.go | 77 ++++++++++++++--- .../internal => util/resultutil}/logger.go | 2 +- .../internal => util/resultutil}/result.go | 2 +- .../resultutil}/result_test.go | 2 +- 8 files changed, 112 insertions(+), 74 deletions(-) rename internal/{cmd/operator-sdk/bundle/validate/internal => util/resultutil}/logger.go (97%) rename internal/{cmd/operator-sdk/bundle/validate/internal => util/resultutil}/result.go (99%) rename internal/{cmd/operator-sdk/bundle/validate/internal => util/resultutil}/result_test.go (99%) diff --git a/internal/cmd/operator-sdk/bundle/validate/cmd.go b/internal/cmd/operator-sdk/bundle/validate/cmd.go index 0f4dfd7f27..ff64eb81a9 100644 --- a/internal/cmd/operator-sdk/bundle/validate/cmd.go +++ b/internal/cmd/operator-sdk/bundle/validate/cmd.go @@ -23,8 +23,8 @@ import ( "github.com/spf13/viper" "k8s.io/apimachinery/pkg/labels" - "github.com/operator-framework/operator-sdk/internal/cmd/operator-sdk/bundle/validate/internal" "github.com/operator-framework/operator-sdk/internal/flags" + "github.com/operator-framework/operator-sdk/internal/util/resultutil" ) const ( @@ -118,7 +118,7 @@ func NewCmd() *cobra.Command { // createLogger creates a new logrus Entry that is optionally verbose. func createLogger(verbose bool) *log.Entry { - logger := log.NewEntry(internal.NewLoggerTo(os.Stderr)) + logger := log.NewEntry(resultutil.NewLoggerTo(os.Stderr)) if verbose { logger.Logger.SetLevel(log.DebugLevel) } diff --git a/internal/cmd/operator-sdk/bundle/validate/validate.go b/internal/cmd/operator-sdk/bundle/validate/validate.go index 56a2776ef5..7a6664aa7e 100644 --- a/internal/cmd/operator-sdk/bundle/validate/validate.go +++ b/internal/cmd/operator-sdk/bundle/validate/validate.go @@ -31,8 +31,8 @@ import ( "github.com/spf13/pflag" "k8s.io/apimachinery/pkg/labels" - "github.com/operator-framework/operator-sdk/internal/cmd/operator-sdk/bundle/validate/internal" internalregistry "github.com/operator-framework/operator-sdk/internal/registry" + "github.com/operator-framework/operator-sdk/internal/util/resultutil" ) type bundleValidateCmd struct { @@ -53,7 +53,7 @@ func (c bundleValidateCmd) validate(args []string) error { if len(args) != 1 { return errors.New("an image tag or directory is a required argument") } - if c.outputFormat != internal.JSONAlpha1 && c.outputFormat != internal.Text { + if c.outputFormat != resultutil.JSONAlpha1 && c.outputFormat != resultutil.Text { return fmt.Errorf("invalid value for output flag: %v", c.outputFormat) } @@ -79,7 +79,7 @@ func (c *bundleValidateCmd) addToFlagSet(fs *pflag.FlagSet) { fs.BoolVar(&c.listOptional, "list-optional", false, "List all optional validators available. When set, no validators will be run") - fs.StringVarP(&c.outputFormat, "output", "o", internal.Text, + fs.StringVarP(&c.outputFormat, "output", "o", resultutil.Text, "Result format for results. One of: [text, json-alpha1]") // It is hidden because it is an alpha option // The idea is the next versions of Operator Registry will return a List of errors @@ -88,7 +88,7 @@ func (c *bundleValidateCmd) addToFlagSet(fs *pflag.FlagSet) { } } -func (c bundleValidateCmd) run(logger *log.Entry, bundleRaw string) (res *internal.Result, err error) { +func (c bundleValidateCmd) run(logger *log.Entry, bundleRaw string) (res *resultutil.Result, err error) { // Create a registry to validate bundle files and optionally unpack the image with. reg, err := newImageRegistryForTool(logger, c.imageBuilder) if err != nil { @@ -130,7 +130,7 @@ func (c bundleValidateCmd) run(logger *log.Entry, bundleRaw string) (res *intern } // Create Result to be output. - res = internal.NewResult() + res = resultutil.NewResult() logger = logger.WithFields(log.Fields{ "bundle-dir": c.directory, diff --git a/internal/cmd/operator-sdk/bundle/validate/validate_test.go b/internal/cmd/operator-sdk/bundle/validate/validate_test.go index 8c58404e5f..ba5afa3159 100644 --- a/internal/cmd/operator-sdk/bundle/validate/validate_test.go +++ b/internal/cmd/operator-sdk/bundle/validate/validate_test.go @@ -17,10 +17,9 @@ package validate import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + "github.com/operator-framework/operator-sdk/internal/util/resultutil" log "github.com/sirupsen/logrus" "github.com/spf13/pflag" - - "github.com/operator-framework/operator-sdk/internal/cmd/operator-sdk/bundle/validate/internal" ) var _ = Describe("Running a bundle validate command", func() { @@ -47,7 +46,7 @@ var _ = Describe("Running a bundle validate command", func() { flag = cmd.Flags().Lookup("output") Expect(flag).NotTo(BeNil()) Expect(flag.Shorthand).To(Equal("o")) - Expect(flag.DefValue).To(Equal(internal.Text)) + Expect(flag.DefValue).To(Equal(resultutil.Text)) }) }) diff --git a/internal/olm/client/client.go b/internal/olm/client/client.go index 66b5e8426e..b22d9ad3cc 100644 --- a/internal/olm/client/client.go +++ b/internal/olm/client/client.go @@ -27,6 +27,7 @@ import ( "github.com/blang/semver" olmapiv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" + "github.com/operator-framework/operator-sdk/internal/util/resultutil" log "github.com/sirupsen/logrus" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -48,22 +49,6 @@ var ErrOLMNotInstalled = errors.New("no existing installation found") var Scheme = scheme.Scheme -// custom error struct to capture deployment errors -// while verifying CSV installs. -type deploymentError struct { - depName string - issue string -} -type deploymentVerifyError []deploymentError - -func (e deploymentVerifyError) Error() string { - var sb strings.Builder - for _, i := range e { - sb.WriteString(i.depName + " has errors: \n" + i.issue + "\n") - } - return sb.String() -} - func init() { if err := olmapiv1alpha1.AddToScheme(Scheme); err != nil { log.Fatalf("Failed to add OLM operator API v1alpha1 types to scheme: %v", err) @@ -241,9 +226,12 @@ func (c Client) DoCSVWait(ctx context.Context, key types.NamespacedName) error { } err := wait.PollImmediateUntil(time.Second, csvPhaseSucceeded, ctx.Done()) if err != nil && errors.Is(err, context.DeadlineExceeded) { - depCheckErr := c.checkDeploymentErrors(ctx, key, csv) - if _, ok := depCheckErr.(deploymentVerifyError); ok { - return depCheckErr + result, err := c.checkDeploymentErrors(ctx, key, csv) + if len(result.Outputs) > 0 { + result.PrintWithFormat("json-alpha1") + } + if err != nil { + return err } } return err @@ -251,10 +239,10 @@ func (c Client) DoCSVWait(ctx context.Context, key types.NamespacedName) error { // checkDeploymentErrors function loops through deployment specs of a given CSV, and returns reason // in case of failures, based on deployment condition. -func (c Client) checkDeploymentErrors(ctx context.Context, key types.NamespacedName, csv olmapiv1alpha1.ClusterServiceVersion) error { - depErr := deploymentVerifyError{} +func (c Client) checkDeploymentErrors(ctx context.Context, key types.NamespacedName, csv olmapiv1alpha1.ClusterServiceVersion) (resultutil.Result, error) { + result := resultutil.NewResult() if key.Namespace == "" { - return fmt.Errorf("no namespace provided to get deployment failures") + return *result, fmt.Errorf("no namespace provided to get deployment failures") } dep := &appsv1.Deployment{} for _, ds := range csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs { @@ -264,62 +252,58 @@ func (c Client) checkDeploymentErrors(ctx context.Context, key types.NamespacedN } depSelectors := ds.Spec.Selector if err := c.KubeClient.Get(ctx, depKey, dep); err != nil { - depErr = append(depErr, deploymentError{ - depName: ds.Name, - issue: err.Error(), - }) + result.AddError(fmt.Errorf("error getting operator deployment %s : %s", ds.Name, err.Error())) continue } for _, s := range dep.Status.Conditions { - if s.Type == appsv1.DeploymentAvailable && s.Status == corev1.ConditionFalse { - podError := c.checkPodErrors(ctx, depSelectors, key) - if podError.Error() == "" { - depErr = append(depErr, deploymentError{ - depName: ds.Name, - issue: s.Reason, - }) - return depErr + if s.Type == appsv1.DeploymentAvailable && s.Status != corev1.ConditionTrue { + podResult, err := c.checkPodErrors(ctx, depSelectors, key) + if len(podResult.Outputs) == 0 { + if err != nil { + result.AddError(fmt.Errorf("error getting operator deployment %s : %s", ds.Name, s.Reason+err.Error())) + } + result.AddError(fmt.Errorf("error getting operator deployment %s : %s", ds.Name, s.Reason)) + return *result, nil } - if _, ok := podError.(deploymentVerifyError); ok { - depErr = append(depErr, deploymentError{ - depName: ds.Name, - issue: podError.Error(), - }) - return depErr + var podErrors []string + for _, p := range podResult.Outputs { + podErrors = append(podErrors, p.Message) } + result.AddError(fmt.Errorf("error getting operator deployment %s : %s", ds.Name, podErrors)) + return *result, nil } } } - return depErr + return *result, nil } // checkPodErrors loops through pods, and returns pod errors if any. -func (c Client) checkPodErrors(ctx context.Context, depSelectors *metav1.LabelSelector, key types.NamespacedName) error { +func (c Client) checkPodErrors(ctx context.Context, depSelectors *metav1.LabelSelector, key types.NamespacedName) (resultutil.Result, error) { // loop through pods and return specific error message. - podErr := deploymentVerifyError{} + //podErr := deploymentVerifyError{} + result := resultutil.NewResult() podList := &corev1.PodList{} podLabelSelectors, err := metav1.LabelSelectorAsSelector(depSelectors) if err != nil { - return err + return *result, err } options := client.ListOptions{ LabelSelector: podLabelSelectors, Namespace: key.Namespace, } if err := c.KubeClient.List(ctx, podList, &options); err != nil { - return fmt.Errorf("error getting Pods: %v", err) + return *result, fmt.Errorf("error getting Pods: %v", err) } for _, p := range podList.Items { for _, cs := range p.Status.ContainerStatuses { if !cs.Ready { - podErr = append(podErr, deploymentError{ - depName: p.Name + cs.Name, - issue: cs.State.Waiting.Message, - }) + if cs.State.Waiting != nil { + result.AddError(fmt.Errorf("pod errors: %s, %s", p.Name+cs.Name, cs.State.Waiting.Message)) + } } } } - return podErr + return *result, nil } // GetInstalledVersion returns the OLM version installed in the namespace informed. diff --git a/internal/olm/client/client_test.go b/internal/olm/client/client_test.go index b45197a790..4133269454 100644 --- a/internal/olm/client/client_test.go +++ b/internal/olm/client/client_test.go @@ -123,8 +123,9 @@ var _ = Describe("Client", func() { Namespace: "test-operator-system", } olmclient := Client{KubeClient: fakeClient} - err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) - Expect(err.Error()).To(ContainSubstring("back-off 5m0s restarting failed container")) + result, err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) + Expect(err).To(BeNil()) + Expect(result.Outputs[0].Message).To(ContainSubstring("back-off 5m0s restarting failed container")) }) It("check error string for multiple pod failures", func() { @@ -198,9 +199,10 @@ var _ = Describe("Client", func() { Namespace: "test-operator-system", } olmclient := Client{KubeClient: fakeClt} - err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) - Expect(err.Error()).To(ContainSubstring("ImageErrPull")) - Expect(err.Error()).To(ContainSubstring("Restarting container")) + result, err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) + Expect(err).To(BeNil()) + Expect(result.Outputs[0].Message).To(ContainSubstring("ImageErrPull")) + Expect(result.Outputs[0].Message).To(ContainSubstring("Restarting container")) }) It("check error string for deployment errors,when no pods exist", func() { @@ -229,8 +231,10 @@ var _ = Describe("Client", func() { Namespace: "test-operator-system", } olmclient := Client{KubeClient: fakeClient} - err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) - Expect(err.Error()).To(ContainSubstring("Pods not available")) + result, err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) + Expect(err).To(BeNil()) + Expect(result.Outputs[0].Message).To(ContainSubstring("error getting operator deployment test-operator-controller-manager")) + }) It("check error string,when no deployments exist for given CSV", func() { @@ -240,9 +244,11 @@ var _ = Describe("Client", func() { Name: "test-operator", Namespace: "test-operator-system", } - err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) - Expect(err.Error()).To(ContainSubstring("\"test-operator-controller-manager\" not found")) - Expect(err.Error()).To(ContainSubstring("\"dummy-operator\" not found")) + result, err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) + Expect(err).To(BeNil()) + Expect(result.Outputs[0].Message).To(ContainSubstring("\"test-operator-controller-manager\" not found")) + Expect(result.Outputs[1].Message).To(ContainSubstring("\"dummy-operator\" not found")) + }) It("check error string,when no namespace provided", func() { fakeClient = fake.NewFakeClient() @@ -250,9 +256,58 @@ var _ = Describe("Client", func() { key := types.NamespacedName{ Name: "test-operator", } - err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) + result, err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) + Expect(result.Outputs).To(BeNil()) Expect(err.Error()).To(ContainSubstring("no namespace provided to get deployment failures")) }) + It("check error string for pod failure with no Message", func() { + fakeClt := fake.NewFakeClient( + &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-operator-jjj", + Namespace: "test-operator-system", + Labels: map[string]string{ + "control-plane": "controller-manager", + }, + }, + Status: corev1.PodStatus{ + ContainerStatuses: []corev1.ContainerStatus{ + { + Ready: false, + State: corev1.ContainerState{}, + }, + }, + }, + }, + &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-operator-controller-manager", + Namespace: "test-operator-system", + Labels: map[string]string{ + "control-plane": "controller-manager", + }, + }, + Status: appsv1.DeploymentStatus{ + Conditions: []appsv1.DeploymentCondition{ + { + Type: "Available", + Status: "Unknown", + }, + }, + }, + }) + + olmclient := Client{KubeClient: fakeClt} + key := types.NamespacedName{ + Name: "test-operator", + Namespace: "test-operator-system", + } + result, err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) + Expect(err).To(BeNil()) + Expect(result.Outputs[0].Message).To(ContainSubstring("error getting operator deployment test-operator-controller-manager : ")) + + }) + }) }) }) diff --git a/internal/cmd/operator-sdk/bundle/validate/internal/logger.go b/internal/util/resultutil/logger.go similarity index 97% rename from internal/cmd/operator-sdk/bundle/validate/internal/logger.go rename to internal/util/resultutil/logger.go index fcba00b250..4d3dc8ee27 100644 --- a/internal/cmd/operator-sdk/bundle/validate/internal/logger.go +++ b/internal/util/resultutil/logger.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package internal +package resultutil import ( "io" diff --git a/internal/cmd/operator-sdk/bundle/validate/internal/result.go b/internal/util/resultutil/result.go similarity index 99% rename from internal/cmd/operator-sdk/bundle/validate/internal/result.go rename to internal/util/resultutil/result.go index a653bf5230..f45594a128 100644 --- a/internal/cmd/operator-sdk/bundle/validate/internal/result.go +++ b/internal/util/resultutil/result.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package internal +package resultutil import ( "encoding/json" diff --git a/internal/cmd/operator-sdk/bundle/validate/internal/result_test.go b/internal/util/resultutil/result_test.go similarity index 99% rename from internal/cmd/operator-sdk/bundle/validate/internal/result_test.go rename to internal/util/resultutil/result_test.go index ad41f53375..966006587c 100644 --- a/internal/cmd/operator-sdk/bundle/validate/internal/result_test.go +++ b/internal/util/resultutil/result_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package internal +package resultutil import ( "encoding/json" From a2ed4930a66f2a9a6502896f96dbb6cc94525673 Mon Sep 17 00:00:00 2001 From: bharathi-tenneti Date: Mon, 28 Sep 2020 10:44:04 -0400 Subject: [PATCH 3/8] fix sanity error --- internal/olm/client/client.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/olm/client/client.go b/internal/olm/client/client.go index b22d9ad3cc..8225742c00 100644 --- a/internal/olm/client/client.go +++ b/internal/olm/client/client.go @@ -228,7 +228,9 @@ func (c Client) DoCSVWait(ctx context.Context, key types.NamespacedName) error { if err != nil && errors.Is(err, context.DeadlineExceeded) { result, err := c.checkDeploymentErrors(ctx, key, csv) if len(result.Outputs) > 0 { - result.PrintWithFormat("json-alpha1") + if err := result.PrintWithFormat("json-alpha1"); err != nil { + return err + } } if err != nil { return err @@ -280,7 +282,6 @@ func (c Client) checkDeploymentErrors(ctx context.Context, key types.NamespacedN // checkPodErrors loops through pods, and returns pod errors if any. func (c Client) checkPodErrors(ctx context.Context, depSelectors *metav1.LabelSelector, key types.NamespacedName) (resultutil.Result, error) { // loop through pods and return specific error message. - //podErr := deploymentVerifyError{} result := resultutil.NewResult() podList := &corev1.PodList{} podLabelSelectors, err := metav1.LabelSelectorAsSelector(depSelectors) From 2635edd296b977f1e67d5e97a6167eb72719d4e5 Mon Sep 17 00:00:00 2001 From: bharathi-tenneti Date: Fri, 2 Oct 2020 10:11:55 -0400 Subject: [PATCH 4/8] Reverting a2ed4 and 8e004 --- internal/olm/client/client.go | 57 +++--- internal/olm/client/client_test.go | 301 +++++++++++------------------ 2 files changed, 139 insertions(+), 219 deletions(-) diff --git a/internal/olm/client/client.go b/internal/olm/client/client.go index 8225742c00..60db14922d 100644 --- a/internal/olm/client/client.go +++ b/internal/olm/client/client.go @@ -27,7 +27,6 @@ import ( "github.com/blang/semver" olmapiv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" - "github.com/operator-framework/operator-sdk/internal/util/resultutil" log "github.com/sirupsen/logrus" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -224,25 +223,20 @@ func (c Client) DoCSVWait(ctx context.Context, key types.NamespacedName) error { return false, nil } } + err := wait.PollImmediateUntil(time.Second, csvPhaseSucceeded, ctx.Done()) if err != nil && errors.Is(err, context.DeadlineExceeded) { - result, err := c.checkDeploymentErrors(ctx, key, csv) - if len(result.Outputs) > 0 { - if err := result.PrintWithFormat("json-alpha1"); err != nil { - return err - } - } - if err != nil { - return err + if depCheckErr := c.printDeploymentErrors(ctx, key, csv); depCheckErr != nil { + return fmt.Errorf("error printing operator resource errors: %v %v", err, depCheckErr) } } return err } -// checkDeploymentErrors function loops through deployment specs of a given CSV, and returns reason +// TODO(btenneti) Refactor function to collect errors into customized error and return. +// printDeploymentErrors function loops through deployment specs of a given CSV, and prints reason // in case of failures, based on deployment condition. -func (c Client) checkDeploymentErrors(ctx context.Context, key types.NamespacedName, csv olmapiv1alpha1.ClusterServiceVersion) (resultutil.Result, error) { - result := resultutil.NewResult() +func (c Client) printDeploymentErrors(ctx context.Context, key types.NamespacedName, csv olmapiv1alpha1.ClusterServiceVersion) error { if key.Namespace == "" { return *result, fmt.Errorf("no namespace provided to get deployment failures") } @@ -254,35 +248,27 @@ func (c Client) checkDeploymentErrors(ctx context.Context, key types.NamespacedN } depSelectors := ds.Spec.Selector if err := c.KubeClient.Get(ctx, depKey, dep); err != nil { - result.AddError(fmt.Errorf("error getting operator deployment %s : %s", ds.Name, err.Error())) + log.Printf("error getting operator deployment %q: %v", ds.Name, err) continue } for _, s := range dep.Status.Conditions { - if s.Type == appsv1.DeploymentAvailable && s.Status != corev1.ConditionTrue { - podResult, err := c.checkPodErrors(ctx, depSelectors, key) - if len(podResult.Outputs) == 0 { - if err != nil { - result.AddError(fmt.Errorf("error getting operator deployment %s : %s", ds.Name, s.Reason+err.Error())) - } - result.AddError(fmt.Errorf("error getting operator deployment %s : %s", ds.Name, s.Reason)) - return *result, nil - } - var podErrors []string - for _, p := range podResult.Outputs { - podErrors = append(podErrors, p.Message) + if s.Type == appsv1.DeploymentAvailable && s.Status == corev1.ConditionFalse { + log.Printf("operator deployment %q not available: %s", ds.Name, s.Reason) + if err := c.printPodErrors(ctx, depSelectors, key); err != nil { + return err } result.AddError(fmt.Errorf("error getting operator deployment %s : %s", ds.Name, podErrors)) return *result, nil } } } - return *result, nil + return nil } -// checkPodErrors loops through pods, and returns pod errors if any. -func (c Client) checkPodErrors(ctx context.Context, depSelectors *metav1.LabelSelector, key types.NamespacedName) (resultutil.Result, error) { +// printPodErrors loops through pods, and prints pod errors if any. +func (c Client) printPodErrors(ctx context.Context, depSelectors *metav1.LabelSelector, key types.NamespacedName) error { // loop through pods and return specific error message. - result := resultutil.NewResult() + podErrors := make(map[string]string) podList := &corev1.PodList{} podLabelSelectors, err := metav1.LabelSelectorAsSelector(depSelectors) if err != nil { @@ -296,15 +282,18 @@ func (c Client) checkPodErrors(ctx context.Context, depSelectors *metav1.LabelSe return *result, fmt.Errorf("error getting Pods: %v", err) } for _, p := range podList.Items { - for _, cs := range p.Status.ContainerStatuses { - if !cs.Ready { - if cs.State.Waiting != nil { - result.AddError(fmt.Errorf("pod errors: %s, %s", p.Name+cs.Name, cs.State.Waiting.Message)) + if p.Status.Phase != corev1.PodSucceeded { + for _, cs := range p.Status.ContainerStatuses { + if !cs.Ready { + podErrors[p.Name] = cs.State.Waiting.Message } } } } - return *result, nil + if len(podErrors) > 0 { + log.Printf("pod errors: %v\n", podErrors) + } + return nil } // GetInstalledVersion returns the OLM version installed in the namespace informed. diff --git a/internal/olm/client/client_test.go b/internal/olm/client/client_test.go index 4133269454..be41f8a0b8 100644 --- a/internal/olm/client/client_test.go +++ b/internal/olm/client/client_test.go @@ -31,234 +31,165 @@ import ( ) var _ = Describe("Client", func() { - Describe("checkDeploymentErrors", func() { + Describe("printDeploymentErrors", func() { var ( fakeClient client.Client - csv olmapiv1alpha1.ClusterServiceVersion ) BeforeEach(func() { - csv = olmapiv1alpha1.ClusterServiceVersion{ - Spec: olmapiv1alpha1.ClusterServiceVersionSpec{ - DisplayName: "test-operator", - InstallStrategy: olmapiv1alpha1.NamedInstallStrategy{ - StrategySpec: olmapiv1alpha1.StrategyDetailsDeployment{ - DeploymentSpecs: []olmapiv1alpha1.StrategyDeploymentSpec{ - { - Name: "test-operator-controller-manager", - Spec: appsv1.DeploymentSpec{ - Selector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "control-plane": "controller-manager", - }, - }, - }, - }, - { - Name: "dummy-operator", - Spec: appsv1.DeploymentSpec{ - Selector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "dummylabel": "dummyvalue", - }, - }, - }, - }, - }, + fakeClient = fake.NewFakeClient( + &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-operator-controller-manager-8687c65f7d-kc44t", + Namespace: "test-operator-system", + Labels: map[string]string{ + "control-plane": "controller-manager", }, }, - }, - } - }) - Context("with a valid csv", func() { - It("check error string for pod errors", func() { - fakeClient = fake.NewFakeClient( - - &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-operator-kc44t", - Namespace: "test-operator-system", - Labels: map[string]string{ - "control-plane": "controller-manager", - }, - }, - Status: corev1.PodStatus{ - ContainerStatuses: []corev1.ContainerStatus{ - { - Ready: false, - State: corev1.ContainerState{ - Waiting: &corev1.ContainerStateWaiting{ - Message: "back-off 5m0s restarting failed container", - }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + ContainerStatuses: []corev1.ContainerStatus{ + { + Ready: false, + State: corev1.ContainerState{ + Waiting: &corev1.ContainerStateWaiting{ + Message: "back-off 5m0s restarting failed container)", + Reason: "CrashLoopBackOff", }, }, }, }, }, - &appsv1.Deployment{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-operator-controller-manager", - Namespace: "test-operator-system", - Labels: map[string]string{ - "control-plane": "controller-manager", - }, + }, + &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dummypod", + Namespace: "testns", + }, + }, + &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-operator-controller-manager-jjj", + Namespace: "test-operator-system", + Labels: map[string]string{ + "control-plane": "controller-manager", }, - Status: appsv1.DeploymentStatus{ - Conditions: []appsv1.DeploymentCondition{ - { - Type: "Available", - Status: "False", - }, - { - Type: "Progressing", - Status: "True", - }, + }, + Status: corev1.PodStatus{ + ContainerStatuses: []corev1.ContainerStatus{ + { + Ready: true, }, }, }, - ) - key := types.NamespacedName{ - Name: "test.operator", - Namespace: "test-operator-system", - } - olmclient := Client{KubeClient: fakeClient} - result, err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) - Expect(err).To(BeNil()) - Expect(result.Outputs[0].Message).To(ContainSubstring("back-off 5m0s restarting failed container")) - }) + }, - It("check error string for multiple pod failures", func() { - fakeClt := fake.NewFakeClient( - &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-operator-jjj", - Namespace: "test-operator-system", - Labels: map[string]string{ - "control-plane": "controller-manager", - }, - }, - Status: corev1.PodStatus{ - ContainerStatuses: []corev1.ContainerStatus{ - { - Ready: false, - State: corev1.ContainerState{ - Waiting: &corev1.ContainerStateWaiting{ - Message: "Restarting container", - }, - }, - }, - }, + &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-operator-controller-manager", + Namespace: "test-operator-system", + Labels: map[string]string{ + "control-plane": "controller-manager", }, }, - &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-operator-kkk", - Namespace: "test-operator-system", - Labels: map[string]string{ - "control-plane": "controller-manager", + Status: appsv1.DeploymentStatus{ + Conditions: []appsv1.DeploymentCondition{ + { + Type: "Available", + Status: "False", + Reason: "MinimumReplicasUnavailable", }, - }, - Status: corev1.PodStatus{ - ContainerStatuses: []corev1.ContainerStatus{ - { - Ready: false, - State: corev1.ContainerState{ - Waiting: &corev1.ContainerStateWaiting{ - Message: "ImageErrPull", - }, - }, - }, + { + Type: "Progressing", + Status: "True", + Reason: "NewReplicaSetAvailable", }, }, }, - &appsv1.Deployment{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-operator-controller-manager", - Namespace: "test-operator-system", - Labels: map[string]string{ - "control-plane": "controller-manager", - }, - }, - Status: appsv1.DeploymentStatus{ - Conditions: []appsv1.DeploymentCondition{ - { - Type: "Available", - Status: "False", - }, - { - Type: "Progressing", - Status: "True", - }, + }, + &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dummy-operator", + Namespace: "dummy-operator-system", + }, + Status: appsv1.DeploymentStatus{ + Conditions: []appsv1.DeploymentCondition{ + { + Type: "Available", + Status: "false", }, }, }, - ) + }, + ) + }) + Context("with a valid csv", func() { + It("should validate the csv successfully", func() { key := types.NamespacedName{ Name: "test.operator", Namespace: "test-operator-system", } - olmclient := Client{KubeClient: fakeClt} - result, err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) - Expect(err).To(BeNil()) - Expect(result.Outputs[0].Message).To(ContainSubstring("ImageErrPull")) - Expect(result.Outputs[0].Message).To(ContainSubstring("Restarting container")) - }) - - It("check error string for deployment errors,when no pods exist", func() { - fakeClient = fake.NewFakeClient( - &appsv1.Deployment{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-operator-controller-manager", - Namespace: "test-operator-system", - Labels: map[string]string{ - "control-plane": "controller-manager", - }, - }, - Status: appsv1.DeploymentStatus{ - Conditions: []appsv1.DeploymentCondition{ - { - Type: "Available", - Status: "False", - Reason: "Pods not available", + csv := olmapiv1alpha1.ClusterServiceVersion{ + Spec: olmapiv1alpha1.ClusterServiceVersionSpec{ + DisplayName: "test-operator", + InstallStrategy: olmapiv1alpha1.NamedInstallStrategy{ + StrategySpec: olmapiv1alpha1.StrategyDetailsDeployment{ + DeploymentSpecs: []olmapiv1alpha1.StrategyDeploymentSpec{ + { + Name: "test-operator-controller-manager", + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "control-plane": "controller-manager", + }, + }, + }, + }, + { + Name: "dummy-operator", + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "dummylabel": "dummyvalue", + }, + }, + }, + }, }, }, }, }, - ) - key := types.NamespacedName{ - Name: "test-operator", - Namespace: "test-operator-system", } olmclient := Client{KubeClient: fakeClient} - result, err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) + err := olmclient.printDeploymentErrors(context.TODO(), key, csv) Expect(err).To(BeNil()) - Expect(result.Outputs[0].Message).To(ContainSubstring("error getting operator deployment test-operator-controller-manager")) }) - - It("check error string,when no deployments exist for given CSV", func() { - fakeClient = fake.NewFakeClient() - olmclient := Client{KubeClient: fakeClient} + }) + Context("with csv key namespace NOT provided", func() { + It("should error out ", func() { key := types.NamespacedName{ - Name: "test-operator", - Namespace: "test-operator-system", + Name: "dummy.clusterserviceversion.yaml", } - result, err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) - Expect(err).To(BeNil()) - Expect(result.Outputs[0].Message).To(ContainSubstring("\"test-operator-controller-manager\" not found")) - Expect(result.Outputs[1].Message).To(ContainSubstring("\"dummy-operator\" not found")) - - }) - It("check error string,when no namespace provided", func() { - fakeClient = fake.NewFakeClient() - olmclient := Client{KubeClient: fakeClient} - key := types.NamespacedName{ - Name: "test-operator", + csv := olmapiv1alpha1.ClusterServiceVersion{ + Spec: olmapiv1alpha1.ClusterServiceVersionSpec{ + DisplayName: "dummy-operator", + InstallStrategy: olmapiv1alpha1.NamedInstallStrategy{ + StrategySpec: olmapiv1alpha1.StrategyDetailsDeployment{ + DeploymentSpecs: []olmapiv1alpha1.StrategyDeploymentSpec{ + { + Name: "dummy-operator", + Spec: appsv1.DeploymentSpec{}, + }, + }, + }, + }, + }, } - result, err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) - Expect(result.Outputs).To(BeNil()) - Expect(err.Error()).To(ContainSubstring("no namespace provided to get deployment failures")) + olmclient := Client{KubeClient: fakeClient} + err := olmclient.printDeploymentErrors(context.TODO(), key, csv) + Expect(err).ToNot(BeNil()) }) It("check error string for pod failure with no Message", func() { fakeClt := fake.NewFakeClient( From ff7db4f25a54479fe2f48b7d7164af247a101c61 Mon Sep 17 00:00:00 2001 From: bharathi-tenneti Date: Fri, 2 Oct 2020 10:14:04 -0400 Subject: [PATCH 5/8] Reverting a2ed4 and 8e004 --- .../cmd/operator-sdk/bundle/validate/cmd.go | 4 +- .../bundle/validate/internal}/logger.go | 2 +- .../bundle/validate/internal}/result.go | 2 +- .../bundle/validate/internal}/result_test.go | 2 +- .../operator-sdk/bundle/validate/validate.go | 10 +- .../bundle/validate/validate_test.go | 5 +- internal/olm/client/client.go | 78 +++++++---- internal/olm/client/client_test.go | 131 ++++++------------ 8 files changed, 106 insertions(+), 128 deletions(-) rename internal/{util/resultutil => cmd/operator-sdk/bundle/validate/internal}/logger.go (97%) rename internal/{util/resultutil => cmd/operator-sdk/bundle/validate/internal}/result.go (99%) rename internal/{util/resultutil => cmd/operator-sdk/bundle/validate/internal}/result_test.go (99%) diff --git a/internal/cmd/operator-sdk/bundle/validate/cmd.go b/internal/cmd/operator-sdk/bundle/validate/cmd.go index ff64eb81a9..0f4dfd7f27 100644 --- a/internal/cmd/operator-sdk/bundle/validate/cmd.go +++ b/internal/cmd/operator-sdk/bundle/validate/cmd.go @@ -23,8 +23,8 @@ import ( "github.com/spf13/viper" "k8s.io/apimachinery/pkg/labels" + "github.com/operator-framework/operator-sdk/internal/cmd/operator-sdk/bundle/validate/internal" "github.com/operator-framework/operator-sdk/internal/flags" - "github.com/operator-framework/operator-sdk/internal/util/resultutil" ) const ( @@ -118,7 +118,7 @@ func NewCmd() *cobra.Command { // createLogger creates a new logrus Entry that is optionally verbose. func createLogger(verbose bool) *log.Entry { - logger := log.NewEntry(resultutil.NewLoggerTo(os.Stderr)) + logger := log.NewEntry(internal.NewLoggerTo(os.Stderr)) if verbose { logger.Logger.SetLevel(log.DebugLevel) } diff --git a/internal/util/resultutil/logger.go b/internal/cmd/operator-sdk/bundle/validate/internal/logger.go similarity index 97% rename from internal/util/resultutil/logger.go rename to internal/cmd/operator-sdk/bundle/validate/internal/logger.go index 4d3dc8ee27..fcba00b250 100644 --- a/internal/util/resultutil/logger.go +++ b/internal/cmd/operator-sdk/bundle/validate/internal/logger.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package resultutil +package internal import ( "io" diff --git a/internal/util/resultutil/result.go b/internal/cmd/operator-sdk/bundle/validate/internal/result.go similarity index 99% rename from internal/util/resultutil/result.go rename to internal/cmd/operator-sdk/bundle/validate/internal/result.go index f45594a128..a653bf5230 100644 --- a/internal/util/resultutil/result.go +++ b/internal/cmd/operator-sdk/bundle/validate/internal/result.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package resultutil +package internal import ( "encoding/json" diff --git a/internal/util/resultutil/result_test.go b/internal/cmd/operator-sdk/bundle/validate/internal/result_test.go similarity index 99% rename from internal/util/resultutil/result_test.go rename to internal/cmd/operator-sdk/bundle/validate/internal/result_test.go index 966006587c..ad41f53375 100644 --- a/internal/util/resultutil/result_test.go +++ b/internal/cmd/operator-sdk/bundle/validate/internal/result_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package resultutil +package internal import ( "encoding/json" diff --git a/internal/cmd/operator-sdk/bundle/validate/validate.go b/internal/cmd/operator-sdk/bundle/validate/validate.go index 7a6664aa7e..56a2776ef5 100644 --- a/internal/cmd/operator-sdk/bundle/validate/validate.go +++ b/internal/cmd/operator-sdk/bundle/validate/validate.go @@ -31,8 +31,8 @@ import ( "github.com/spf13/pflag" "k8s.io/apimachinery/pkg/labels" + "github.com/operator-framework/operator-sdk/internal/cmd/operator-sdk/bundle/validate/internal" internalregistry "github.com/operator-framework/operator-sdk/internal/registry" - "github.com/operator-framework/operator-sdk/internal/util/resultutil" ) type bundleValidateCmd struct { @@ -53,7 +53,7 @@ func (c bundleValidateCmd) validate(args []string) error { if len(args) != 1 { return errors.New("an image tag or directory is a required argument") } - if c.outputFormat != resultutil.JSONAlpha1 && c.outputFormat != resultutil.Text { + if c.outputFormat != internal.JSONAlpha1 && c.outputFormat != internal.Text { return fmt.Errorf("invalid value for output flag: %v", c.outputFormat) } @@ -79,7 +79,7 @@ func (c *bundleValidateCmd) addToFlagSet(fs *pflag.FlagSet) { fs.BoolVar(&c.listOptional, "list-optional", false, "List all optional validators available. When set, no validators will be run") - fs.StringVarP(&c.outputFormat, "output", "o", resultutil.Text, + fs.StringVarP(&c.outputFormat, "output", "o", internal.Text, "Result format for results. One of: [text, json-alpha1]") // It is hidden because it is an alpha option // The idea is the next versions of Operator Registry will return a List of errors @@ -88,7 +88,7 @@ func (c *bundleValidateCmd) addToFlagSet(fs *pflag.FlagSet) { } } -func (c bundleValidateCmd) run(logger *log.Entry, bundleRaw string) (res *resultutil.Result, err error) { +func (c bundleValidateCmd) run(logger *log.Entry, bundleRaw string) (res *internal.Result, err error) { // Create a registry to validate bundle files and optionally unpack the image with. reg, err := newImageRegistryForTool(logger, c.imageBuilder) if err != nil { @@ -130,7 +130,7 @@ func (c bundleValidateCmd) run(logger *log.Entry, bundleRaw string) (res *result } // Create Result to be output. - res = resultutil.NewResult() + res = internal.NewResult() logger = logger.WithFields(log.Fields{ "bundle-dir": c.directory, diff --git a/internal/cmd/operator-sdk/bundle/validate/validate_test.go b/internal/cmd/operator-sdk/bundle/validate/validate_test.go index ba5afa3159..8c58404e5f 100644 --- a/internal/cmd/operator-sdk/bundle/validate/validate_test.go +++ b/internal/cmd/operator-sdk/bundle/validate/validate_test.go @@ -17,9 +17,10 @@ package validate import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" - "github.com/operator-framework/operator-sdk/internal/util/resultutil" log "github.com/sirupsen/logrus" "github.com/spf13/pflag" + + "github.com/operator-framework/operator-sdk/internal/cmd/operator-sdk/bundle/validate/internal" ) var _ = Describe("Running a bundle validate command", func() { @@ -46,7 +47,7 @@ var _ = Describe("Running a bundle validate command", func() { flag = cmd.Flags().Lookup("output") Expect(flag).NotTo(BeNil()) Expect(flag.Shorthand).To(Equal("o")) - Expect(flag.DefValue).To(Equal(resultutil.Text)) + Expect(flag.DefValue).To(Equal(internal.Text)) }) }) diff --git a/internal/olm/client/client.go b/internal/olm/client/client.go index 60db14922d..8e5ccdd6b4 100644 --- a/internal/olm/client/client.go +++ b/internal/olm/client/client.go @@ -48,6 +48,22 @@ var ErrOLMNotInstalled = errors.New("no existing installation found") var Scheme = scheme.Scheme +// custom error struct to capture deployment errors +// while verifying CSV installs. +type deploymentError struct { + depName string + issue string +} +type deploymentVerifyError []deploymentError + +func (e deploymentVerifyError) Error() string { + var sb strings.Builder + for _, i := range e { + sb.WriteString(i.depName + " has errors: \n" + i.issue + "\n") + } + return sb.String() +} + func init() { if err := olmapiv1alpha1.AddToScheme(Scheme); err != nil { log.Fatalf("Failed to add OLM operator API v1alpha1 types to scheme: %v", err) @@ -226,8 +242,9 @@ func (c Client) DoCSVWait(ctx context.Context, key types.NamespacedName) error { err := wait.PollImmediateUntil(time.Second, csvPhaseSucceeded, ctx.Done()) if err != nil && errors.Is(err, context.DeadlineExceeded) { - if depCheckErr := c.printDeploymentErrors(ctx, key, csv); depCheckErr != nil { - return fmt.Errorf("error printing operator resource errors: %v %v", err, depCheckErr) + depCheckErr := c.checkDeploymentErrors(ctx, key, csv) + if _, ok := depCheckErr.(deploymentVerifyError); ok { + return depCheckErr } } return err @@ -236,9 +253,10 @@ func (c Client) DoCSVWait(ctx context.Context, key types.NamespacedName) error { // TODO(btenneti) Refactor function to collect errors into customized error and return. // printDeploymentErrors function loops through deployment specs of a given CSV, and prints reason // in case of failures, based on deployment condition. -func (c Client) printDeploymentErrors(ctx context.Context, key types.NamespacedName, csv olmapiv1alpha1.ClusterServiceVersion) error { +func (c Client) checkDeploymentErrors(ctx context.Context, key types.NamespacedName, csv olmapiv1alpha1.ClusterServiceVersion) error { + depErr := deploymentVerifyError{} if key.Namespace == "" { - return *result, fmt.Errorf("no namespace provided to get deployment failures") + return fmt.Errorf("no namespace provided to get deployment failures") } dep := &appsv1.Deployment{} for _, ds := range csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs { @@ -248,52 +266,62 @@ func (c Client) printDeploymentErrors(ctx context.Context, key types.NamespacedN } depSelectors := ds.Spec.Selector if err := c.KubeClient.Get(ctx, depKey, dep); err != nil { - log.Printf("error getting operator deployment %q: %v", ds.Name, err) + depErr = append(depErr, deploymentError{ + depName: ds.Name, + issue: err.Error(), + }) continue } for _, s := range dep.Status.Conditions { if s.Type == appsv1.DeploymentAvailable && s.Status == corev1.ConditionFalse { - log.Printf("operator deployment %q not available: %s", ds.Name, s.Reason) - if err := c.printPodErrors(ctx, depSelectors, key); err != nil { - return err + podError := c.checkPodErrors(ctx, depSelectors, key) + if podError.Error() == "" { + depErr = append(depErr, deploymentError{ + depName: ds.Name, + issue: s.Reason, + }) + return depErr + } + if _, ok := podError.(deploymentVerifyError); ok { + depErr = append(depErr, deploymentError{ + depName: ds.Name, + issue: podError.Error(), + }) + return depErr } - result.AddError(fmt.Errorf("error getting operator deployment %s : %s", ds.Name, podErrors)) - return *result, nil } } } - return nil + return depErr } -// printPodErrors loops through pods, and prints pod errors if any. -func (c Client) printPodErrors(ctx context.Context, depSelectors *metav1.LabelSelector, key types.NamespacedName) error { +// checkPodErrors loops through pods, and returns pod errors if any. +func (c Client) checkPodErrors(ctx context.Context, depSelectors *metav1.LabelSelector, key types.NamespacedName) error { // loop through pods and return specific error message. - podErrors := make(map[string]string) + podErr := deploymentVerifyError{} podList := &corev1.PodList{} podLabelSelectors, err := metav1.LabelSelectorAsSelector(depSelectors) if err != nil { - return *result, err + return err } options := client.ListOptions{ LabelSelector: podLabelSelectors, Namespace: key.Namespace, } if err := c.KubeClient.List(ctx, podList, &options); err != nil { - return *result, fmt.Errorf("error getting Pods: %v", err) + return fmt.Errorf("error getting Pods: %v", err) } for _, p := range podList.Items { - if p.Status.Phase != corev1.PodSucceeded { - for _, cs := range p.Status.ContainerStatuses { - if !cs.Ready { - podErrors[p.Name] = cs.State.Waiting.Message - } + for _, cs := range p.Status.ContainerStatuses { + if !cs.Ready { + podErr = append(podErr, deploymentError{ + depName: p.Name + cs.Name, + issue: cs.State.Waiting.Message, + }) } } } - if len(podErrors) > 0 { - log.Printf("pod errors: %v\n", podErrors) - } - return nil + return podErr } // GetInstalledVersion returns the OLM version installed in the namespace informed. diff --git a/internal/olm/client/client_test.go b/internal/olm/client/client_test.go index be41f8a0b8..7f2e0b3359 100644 --- a/internal/olm/client/client_test.go +++ b/internal/olm/client/client_test.go @@ -83,7 +83,15 @@ var _ = Describe("Client", func() { }, }, }, - }, + ) + key := types.NamespacedName{ + Name: "test.operator", + Namespace: "test-operator-system", + } + olmclient := Client{KubeClient: fakeClient} + err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) + Expect(err.Error()).To(ContainSubstring("back-off 5m0s restarting failed container")) + }) &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ @@ -130,86 +138,14 @@ var _ = Describe("Client", func() { Name: "test.operator", Namespace: "test-operator-system", } - csv := olmapiv1alpha1.ClusterServiceVersion{ - Spec: olmapiv1alpha1.ClusterServiceVersionSpec{ - DisplayName: "test-operator", - InstallStrategy: olmapiv1alpha1.NamedInstallStrategy{ - StrategySpec: olmapiv1alpha1.StrategyDetailsDeployment{ - DeploymentSpecs: []olmapiv1alpha1.StrategyDeploymentSpec{ - { - Name: "test-operator-controller-manager", - Spec: appsv1.DeploymentSpec{ - Selector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "control-plane": "controller-manager", - }, - }, - }, - }, - { - Name: "dummy-operator", - Spec: appsv1.DeploymentSpec{ - Selector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "dummylabel": "dummyvalue", - }, - }, - }, - }, - }, - }, - }, - }, - } - olmclient := Client{KubeClient: fakeClient} - err := olmclient.printDeploymentErrors(context.TODO(), key, csv) - Expect(err).To(BeNil()) - - }) - }) - Context("with csv key namespace NOT provided", func() { - It("should error out ", func() { - key := types.NamespacedName{ - Name: "dummy.clusterserviceversion.yaml", - } - csv := olmapiv1alpha1.ClusterServiceVersion{ - Spec: olmapiv1alpha1.ClusterServiceVersionSpec{ - DisplayName: "dummy-operator", - InstallStrategy: olmapiv1alpha1.NamedInstallStrategy{ - StrategySpec: olmapiv1alpha1.StrategyDetailsDeployment{ - DeploymentSpecs: []olmapiv1alpha1.StrategyDeploymentSpec{ - { - Name: "dummy-operator", - Spec: appsv1.DeploymentSpec{}, - }, - }, - }, - }, - }, - } - olmclient := Client{KubeClient: fakeClient} - err := olmclient.printDeploymentErrors(context.TODO(), key, csv) - Expect(err).ToNot(BeNil()) + olmclient := Client{KubeClient: fakeClt} + err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) + Expect(err.Error()).To(ContainSubstring("ImageErrPull")) + Expect(err.Error()).To(ContainSubstring("Restarting container")) }) - It("check error string for pod failure with no Message", func() { - fakeClt := fake.NewFakeClient( - &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-operator-jjj", - Namespace: "test-operator-system", - Labels: map[string]string{ - "control-plane": "controller-manager", - }, - }, - Status: corev1.PodStatus{ - ContainerStatuses: []corev1.ContainerStatus{ - { - Ready: false, - State: corev1.ContainerState{}, - }, - }, - }, - }, + + It("check error string for deployment errors,when no pods exist", func() { + fakeClient = fake.NewFakeClient( &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ Name: "test-operator-controller-manager", @@ -222,23 +158,36 @@ var _ = Describe("Client", func() { Conditions: []appsv1.DeploymentCondition{ { Type: "Available", - Status: "Unknown", + Status: "False", + Reason: "Pods not available", }, }, }, - }) - - olmclient := Client{KubeClient: fakeClt} + }, + } + olmclient := Client{KubeClient: fakeClient} + err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) + Expect(err.Error()).To(ContainSubstring("Pods not available")) + }) + }) + Context("with csv key namespace NOT provided", func() { + It("should error out ", func() { key := types.NamespacedName{ - Name: "test-operator", - Namespace: "test-operator-system", + Name: "dummy.clusterserviceversion.yaml", } - result, err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) - Expect(err).To(BeNil()) - Expect(result.Outputs[0].Message).To(ContainSubstring("error getting operator deployment test-operator-controller-manager : ")) - + err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) + Expect(err.Error()).To(ContainSubstring("\"test-operator-controller-manager\" not found")) + Expect(err.Error()).To(ContainSubstring("\"dummy-operator\" not found")) + }) + It("check error string,when no namespace provided", func() { + fakeClient = fake.NewFakeClient() + olmclient := Client{KubeClient: fakeClient} + key := types.NamespacedName{ + Name: "test-operator", + } + err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) + Expect(err.Error()).To(ContainSubstring("no namespace provided to get deployment failures")) }) - }) }) }) From e78d048ab9b591246a5e71fc28a27589f5b0a2d7 Mon Sep 17 00:00:00 2001 From: bharathi-tenneti Date: Fri, 2 Oct 2020 10:34:01 -0400 Subject: [PATCH 6/8] Addressing PR comments --- internal/olm/client/client.go | 33 ++--- internal/olm/client/client_test.go | 207 +++++++++++++++++++---------- 2 files changed, 153 insertions(+), 87 deletions(-) diff --git a/internal/olm/client/client.go b/internal/olm/client/client.go index 8e5ccdd6b4..4d009e8003 100644 --- a/internal/olm/client/client.go +++ b/internal/olm/client/client.go @@ -51,15 +51,15 @@ var Scheme = scheme.Scheme // custom error struct to capture deployment errors // while verifying CSV installs. type deploymentError struct { - depName string - issue string + name string + issue string } type deploymentVerifyError []deploymentError func (e deploymentVerifyError) Error() string { var sb strings.Builder for _, i := range e { - sb.WriteString(i.depName + " has errors: \n" + i.issue + "\n") + sb.WriteString(i.name + " has errors: \n" + i.issue + "\n") } return sb.String() } @@ -250,8 +250,7 @@ func (c Client) DoCSVWait(ctx context.Context, key types.NamespacedName) error { return err } -// TODO(btenneti) Refactor function to collect errors into customized error and return. -// printDeploymentErrors function loops through deployment specs of a given CSV, and prints reason +// checkDeploymentErrors function loops through deployment specs of a given CSV, and prints reason // in case of failures, based on deployment condition. func (c Client) checkDeploymentErrors(ctx context.Context, key types.NamespacedName, csv olmapiv1alpha1.ClusterServiceVersion) error { depErr := deploymentVerifyError{} @@ -267,25 +266,25 @@ func (c Client) checkDeploymentErrors(ctx context.Context, key types.NamespacedN depSelectors := ds.Spec.Selector if err := c.KubeClient.Get(ctx, depKey, dep); err != nil { depErr = append(depErr, deploymentError{ - depName: ds.Name, - issue: err.Error(), + name: ds.Name, + issue: err.Error(), }) continue } for _, s := range dep.Status.Conditions { - if s.Type == appsv1.DeploymentAvailable && s.Status == corev1.ConditionFalse { + if s.Type == appsv1.DeploymentAvailable && s.Status != corev1.ConditionTrue { podError := c.checkPodErrors(ctx, depSelectors, key) if podError.Error() == "" { depErr = append(depErr, deploymentError{ - depName: ds.Name, - issue: s.Reason, + name: ds.Name, + issue: s.Reason, }) return depErr } if _, ok := podError.(deploymentVerifyError); ok { depErr = append(depErr, deploymentError{ - depName: ds.Name, - issue: podError.Error(), + name: ds.Name, + issue: podError.Error(), }) return depErr } @@ -314,10 +313,12 @@ func (c Client) checkPodErrors(ctx context.Context, depSelectors *metav1.LabelSe for _, p := range podList.Items { for _, cs := range p.Status.ContainerStatuses { if !cs.Ready { - podErr = append(podErr, deploymentError{ - depName: p.Name + cs.Name, - issue: cs.State.Waiting.Message, - }) + if cs.State.Waiting != nil { + podErr = append(podErr, deploymentError{ + name: p.Name + cs.Name, + issue: cs.State.Waiting.Message, + }) + } } } } diff --git a/internal/olm/client/client_test.go b/internal/olm/client/client_test.go index 7f2e0b3359..b45197a790 100644 --- a/internal/olm/client/client_test.go +++ b/internal/olm/client/client_test.go @@ -31,55 +31,89 @@ import ( ) var _ = Describe("Client", func() { - Describe("printDeploymentErrors", func() { + Describe("checkDeploymentErrors", func() { var ( fakeClient client.Client + csv olmapiv1alpha1.ClusterServiceVersion ) BeforeEach(func() { - fakeClient = fake.NewFakeClient( - &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-operator-controller-manager-8687c65f7d-kc44t", - Namespace: "test-operator-system", - Labels: map[string]string{ - "control-plane": "controller-manager", - }, - }, - Status: corev1.PodStatus{ - Phase: corev1.PodRunning, - ContainerStatuses: []corev1.ContainerStatus{ - { - Ready: false, - State: corev1.ContainerState{ - Waiting: &corev1.ContainerStateWaiting{ - Message: "back-off 5m0s restarting failed container)", - Reason: "CrashLoopBackOff", + csv = olmapiv1alpha1.ClusterServiceVersion{ + Spec: olmapiv1alpha1.ClusterServiceVersionSpec{ + DisplayName: "test-operator", + InstallStrategy: olmapiv1alpha1.NamedInstallStrategy{ + StrategySpec: olmapiv1alpha1.StrategyDetailsDeployment{ + DeploymentSpecs: []olmapiv1alpha1.StrategyDeploymentSpec{ + { + Name: "test-operator-controller-manager", + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "control-plane": "controller-manager", + }, + }, + }, + }, + { + Name: "dummy-operator", + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "dummylabel": "dummyvalue", + }, + }, }, }, }, }, }, }, - &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dummypod", - Namespace: "testns", - }, - }, - &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-operator-controller-manager-jjj", - Namespace: "test-operator-system", - Labels: map[string]string{ - "control-plane": "controller-manager", + } + }) + Context("with a valid csv", func() { + It("check error string for pod errors", func() { + fakeClient = fake.NewFakeClient( + + &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-operator-kc44t", + Namespace: "test-operator-system", + Labels: map[string]string{ + "control-plane": "controller-manager", + }, + }, + Status: corev1.PodStatus{ + ContainerStatuses: []corev1.ContainerStatus{ + { + Ready: false, + State: corev1.ContainerState{ + Waiting: &corev1.ContainerStateWaiting{ + Message: "back-off 5m0s restarting failed container", + }, + }, + }, + }, }, }, - Status: corev1.PodStatus{ - ContainerStatuses: []corev1.ContainerStatus{ - { - Ready: true, + &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-operator-controller-manager", + Namespace: "test-operator-system", + Labels: map[string]string{ + "control-plane": "controller-manager", + }, + }, + Status: appsv1.DeploymentStatus{ + Conditions: []appsv1.DeploymentCondition{ + { + Type: "Available", + Status: "False", + }, + { + Type: "Progressing", + Status: "True", + }, }, }, }, @@ -93,47 +127,72 @@ var _ = Describe("Client", func() { Expect(err.Error()).To(ContainSubstring("back-off 5m0s restarting failed container")) }) - &appsv1.Deployment{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-operator-controller-manager", - Namespace: "test-operator-system", - Labels: map[string]string{ - "control-plane": "controller-manager", + It("check error string for multiple pod failures", func() { + fakeClt := fake.NewFakeClient( + &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-operator-jjj", + Namespace: "test-operator-system", + Labels: map[string]string{ + "control-plane": "controller-manager", + }, + }, + Status: corev1.PodStatus{ + ContainerStatuses: []corev1.ContainerStatus{ + { + Ready: false, + State: corev1.ContainerState{ + Waiting: &corev1.ContainerStateWaiting{ + Message: "Restarting container", + }, + }, + }, + }, }, }, - Status: appsv1.DeploymentStatus{ - Conditions: []appsv1.DeploymentCondition{ - { - Type: "Available", - Status: "False", - Reason: "MinimumReplicasUnavailable", + &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-operator-kkk", + Namespace: "test-operator-system", + Labels: map[string]string{ + "control-plane": "controller-manager", }, - { - Type: "Progressing", - Status: "True", - Reason: "NewReplicaSetAvailable", + }, + Status: corev1.PodStatus{ + ContainerStatuses: []corev1.ContainerStatus{ + { + Ready: false, + State: corev1.ContainerState{ + Waiting: &corev1.ContainerStateWaiting{ + Message: "ImageErrPull", + }, + }, + }, }, }, }, - }, - &appsv1.Deployment{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dummy-operator", - Namespace: "dummy-operator-system", - }, - Status: appsv1.DeploymentStatus{ - Conditions: []appsv1.DeploymentCondition{ - { - Type: "Available", - Status: "false", + &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-operator-controller-manager", + Namespace: "test-operator-system", + Labels: map[string]string{ + "control-plane": "controller-manager", + }, + }, + Status: appsv1.DeploymentStatus{ + Conditions: []appsv1.DeploymentCondition{ + { + Type: "Available", + Status: "False", + }, + { + Type: "Progressing", + Status: "True", + }, }, }, }, - }, - ) - }) - Context("with a valid csv", func() { - It("should validate the csv successfully", func() { + ) key := types.NamespacedName{ Name: "test.operator", Namespace: "test-operator-system", @@ -164,16 +223,22 @@ var _ = Describe("Client", func() { }, }, }, + ) + key := types.NamespacedName{ + Name: "test-operator", + Namespace: "test-operator-system", } olmclient := Client{KubeClient: fakeClient} err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) Expect(err.Error()).To(ContainSubstring("Pods not available")) }) - }) - Context("with csv key namespace NOT provided", func() { - It("should error out ", func() { + + It("check error string,when no deployments exist for given CSV", func() { + fakeClient = fake.NewFakeClient() + olmclient := Client{KubeClient: fakeClient} key := types.NamespacedName{ - Name: "dummy.clusterserviceversion.yaml", + Name: "test-operator", + Namespace: "test-operator-system", } err := olmclient.checkDeploymentErrors(context.TODO(), key, csv) Expect(err.Error()).To(ContainSubstring("\"test-operator-controller-manager\" not found")) From 938758278ca790260200f3afd42cbbaf4829cf4b Mon Sep 17 00:00:00 2001 From: bharathi-tenneti Date: Fri, 2 Oct 2020 17:55:01 -0400 Subject: [PATCH 7/8] Added podError struct, and other PR comments --- internal/olm/client/client.go | 65 +++++++++++++++++++----------- internal/olm/client/client_test.go | 12 ++++-- 2 files changed, 49 insertions(+), 28 deletions(-) diff --git a/internal/olm/client/client.go b/internal/olm/client/client.go index 4d009e8003..b00d5d31e8 100644 --- a/internal/olm/client/client.go +++ b/internal/olm/client/client.go @@ -50,16 +50,32 @@ var Scheme = scheme.Scheme // custom error struct to capture deployment errors // while verifying CSV installs. -type deploymentError struct { +type resourceError struct { name string issue string } -type deploymentVerifyError []deploymentError +type podError struct { + resourceError +} +type deploymentError struct { + resourceError + podErrs podErrors +} +type deploymentErrors []deploymentError +type podErrors []podError + +func (e deploymentErrors) Error() string { + var sb strings.Builder + for _, i := range e { + sb.WriteString(fmt.Sprintf("deployment %s has error: %s\n%s", i.name, i.issue, i.podErrs.Error())) + } + return sb.String() +} -func (e deploymentVerifyError) Error() string { +func (e podErrors) Error() string { var sb strings.Builder for _, i := range e { - sb.WriteString(i.name + " has errors: \n" + i.issue + "\n") + sb.WriteString(fmt.Sprintf("\tpod %s has error: %s\n", i.name, i.issue)) } return sb.String() } @@ -243,7 +259,7 @@ func (c Client) DoCSVWait(ctx context.Context, key types.NamespacedName) error { err := wait.PollImmediateUntil(time.Second, csvPhaseSucceeded, ctx.Done()) if err != nil && errors.Is(err, context.DeadlineExceeded) { depCheckErr := c.checkDeploymentErrors(ctx, key, csv) - if _, ok := depCheckErr.(deploymentVerifyError); ok { + if depCheckErr != nil { return depCheckErr } } @@ -253,12 +269,13 @@ func (c Client) DoCSVWait(ctx context.Context, key types.NamespacedName) error { // checkDeploymentErrors function loops through deployment specs of a given CSV, and prints reason // in case of failures, based on deployment condition. func (c Client) checkDeploymentErrors(ctx context.Context, key types.NamespacedName, csv olmapiv1alpha1.ClusterServiceVersion) error { - depErr := deploymentVerifyError{} + depErr := deploymentErrors{} if key.Namespace == "" { return fmt.Errorf("no namespace provided to get deployment failures") } dep := &appsv1.Deployment{} for _, ds := range csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs { + podErr := podErrors{} depKey := types.NamespacedName{ Namespace: key.Namespace, Name: ds.Name, @@ -266,28 +283,25 @@ func (c Client) checkDeploymentErrors(ctx context.Context, key types.NamespacedN depSelectors := ds.Spec.Selector if err := c.KubeClient.Get(ctx, depKey, dep); err != nil { depErr = append(depErr, deploymentError{ - name: ds.Name, - issue: err.Error(), + resourceError{ + name: ds.Name, + issue: err.Error(), + }, + podErr, }) continue } for _, s := range dep.Status.Conditions { if s.Type == appsv1.DeploymentAvailable && s.Status != corev1.ConditionTrue { podError := c.checkPodErrors(ctx, depSelectors, key) - if podError.Error() == "" { - depErr = append(depErr, deploymentError{ + podErr = podError.(podErrors) + depErr = append(depErr, deploymentError{ + resourceError{ name: ds.Name, issue: s.Reason, - }) - return depErr - } - if _, ok := podError.(deploymentVerifyError); ok { - depErr = append(depErr, deploymentError{ - name: ds.Name, - issue: podError.Error(), - }) - return depErr - } + }, + podErr, + }) } } } @@ -297,7 +311,7 @@ func (c Client) checkDeploymentErrors(ctx context.Context, key types.NamespacedN // checkPodErrors loops through pods, and returns pod errors if any. func (c Client) checkPodErrors(ctx context.Context, depSelectors *metav1.LabelSelector, key types.NamespacedName) error { // loop through pods and return specific error message. - podErr := deploymentVerifyError{} + podErr := podErrors{} podList := &corev1.PodList{} podLabelSelectors, err := metav1.LabelSelectorAsSelector(depSelectors) if err != nil { @@ -314,9 +328,12 @@ func (c Client) checkPodErrors(ctx context.Context, depSelectors *metav1.LabelSe for _, cs := range p.Status.ContainerStatuses { if !cs.Ready { if cs.State.Waiting != nil { - podErr = append(podErr, deploymentError{ - name: p.Name + cs.Name, - issue: cs.State.Waiting.Message, + containerName := p.Name + ":" + cs.Name + podErr = append(podErr, podError{ + resourceError{ + name: containerName, + issue: cs.State.Waiting.Message, + }, }) } } diff --git a/internal/olm/client/client_test.go b/internal/olm/client/client_test.go index b45197a790..b5f0a170ed 100644 --- a/internal/olm/client/client_test.go +++ b/internal/olm/client/client_test.go @@ -46,21 +46,22 @@ var _ = Describe("Client", func() { StrategySpec: olmapiv1alpha1.StrategyDetailsDeployment{ DeploymentSpecs: []olmapiv1alpha1.StrategyDeploymentSpec{ { - Name: "test-operator-controller-manager", + Name: "dummy-operator", Spec: appsv1.DeploymentSpec{ Selector: &metav1.LabelSelector{ MatchLabels: map[string]string{ - "control-plane": "controller-manager", + "dummylabel": "dummyvalue", }, }, }, }, + { - Name: "dummy-operator", + Name: "test-operator-controller-manager", Spec: appsv1.DeploymentSpec{ Selector: &metav1.LabelSelector{ MatchLabels: map[string]string{ - "dummylabel": "dummyvalue", + "control-plane": "controller-manager", }, }, }, @@ -86,6 +87,7 @@ var _ = Describe("Client", func() { Status: corev1.PodStatus{ ContainerStatuses: []corev1.ContainerStatus{ { + Name: "container1", Ready: false, State: corev1.ContainerState{ Waiting: &corev1.ContainerStateWaiting{ @@ -140,6 +142,7 @@ var _ = Describe("Client", func() { Status: corev1.PodStatus{ ContainerStatuses: []corev1.ContainerStatus{ { + Name: "container1", Ready: false, State: corev1.ContainerState{ Waiting: &corev1.ContainerStateWaiting{ @@ -161,6 +164,7 @@ var _ = Describe("Client", func() { Status: corev1.PodStatus{ ContainerStatuses: []corev1.ContainerStatus{ { + Name: "container2", Ready: false, State: corev1.ContainerState{ Waiting: &corev1.ContainerStateWaiting{ From 81fcac2a15bd3784ea119b6ac6827c8d07568779 Mon Sep 17 00:00:00 2001 From: varshaprasad96 Date: Thu, 8 Oct 2020 13:03:55 -0700 Subject: [PATCH 8/8] Address review comments --- internal/olm/.DS_Store | Bin 0 -> 6148 bytes internal/olm/client/client.go | 27 +++++++++++++++------------ 2 files changed, 15 insertions(+), 12 deletions(-) create mode 100644 internal/olm/.DS_Store diff --git a/internal/olm/.DS_Store b/internal/olm/.DS_Store new file mode 100644 index 0000000000000000000000000000000000000000..ef82ecf21faacd9ab6b959ebf1f809df197588c8 GIT binary patch literal 6148 zcmeHKOKQU~5S>X)F?8c)mbyZ3AcA`WUm%c%5-={5HfyhPt{$y#J{#BVF4=@PFnZEx zo`l}Q;}H?veZ4-5bRyEi4dr6P+HBu^Wt)sB5RNk@`8*FF)A&BUt+M|P823>QGRjGg zzxcLCqXJZb3Qz$mKn1R=K-Smx>AJ`AG%7#^?mz+iJ`}iNO`HS$(}BTT0APc#8|L0i z0E-2HHE|9^1g1d+2351g(4Zq;GOs4ifk79|=0o#l%??HVcAQ^4U9<*rqyki6slY=l zJFEXM@PGRMC5bC4Kn3nf0d4xB-{DDFTUU>>T3g_IxaEAq%`kTg1~11zFUMF|Io^6w biGvE;K4|YXyD)bAA=5 literal 0 HcmV?d00001 diff --git a/internal/olm/client/client.go b/internal/olm/client/client.go index b00d5d31e8..b904f0ecb0 100644 --- a/internal/olm/client/client.go +++ b/internal/olm/client/client.go @@ -269,43 +269,46 @@ func (c Client) DoCSVWait(ctx context.Context, key types.NamespacedName) error { // checkDeploymentErrors function loops through deployment specs of a given CSV, and prints reason // in case of failures, based on deployment condition. func (c Client) checkDeploymentErrors(ctx context.Context, key types.NamespacedName, csv olmapiv1alpha1.ClusterServiceVersion) error { - depErr := deploymentErrors{} + depErrs := deploymentErrors{} if key.Namespace == "" { return fmt.Errorf("no namespace provided to get deployment failures") } dep := &appsv1.Deployment{} for _, ds := range csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs { - podErr := podErrors{} depKey := types.NamespacedName{ Namespace: key.Namespace, Name: ds.Name, } depSelectors := ds.Spec.Selector if err := c.KubeClient.Get(ctx, depKey, dep); err != nil { - depErr = append(depErr, deploymentError{ - resourceError{ + depErrs = append(depErrs, deploymentError{ + resourceError: resourceError{ name: ds.Name, issue: err.Error(), }, - podErr, }) continue } for _, s := range dep.Status.Conditions { if s.Type == appsv1.DeploymentAvailable && s.Status != corev1.ConditionTrue { - podError := c.checkPodErrors(ctx, depSelectors, key) - podErr = podError.(podErrors) - depErr = append(depErr, deploymentError{ - resourceError{ + depErr := deploymentError{ + resourceError: resourceError{ name: ds.Name, issue: s.Reason, }, - podErr, - }) + } + podErr := c.checkPodErrors(ctx, depSelectors, key) + podErrs := podErrors{} + if errors.As(podErr, &podErrs) { + depErr.podErrs = append(depErr.podErrs, podErrs...) + } else { + return podErr + } + depErrs = append(depErrs, depErr) } } } - return depErr + return depErrs } // checkPodErrors loops through pods, and returns pod errors if any.