Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 71 additions & 2 deletions internal/olm/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import (
olmapiv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
log "github.com/sirupsen/logrus"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"

apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -194,8 +196,8 @@ func (c Client) DoCSVWait(ctx context.Context, key types.NamespacedName) error {
)
once := sync.Once{}

csv := olmapiv1alpha1.ClusterServiceVersion{}
csvPhaseSucceeded := func() (bool, error) {
csv := olmapiv1alpha1.ClusterServiceVersion{}
err := c.KubeClient.Get(ctx, key, &csv)
if err != nil {
if apierrors.IsNotFound(err) {
Expand All @@ -222,7 +224,74 @@ func (c Client) DoCSVWait(ctx context.Context, key types.NamespacedName) error {
}
}

return wait.PollImmediateUntil(time.Second, csvPhaseSucceeded, ctx.Done())
err := wait.PollImmediateUntil(time.Second, csvPhaseSucceeded, ctx.Done())
if err != nil && errors.Is(err, context.DeadlineExceeded) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Coming back to this from a prior review: why check deployments/pods only when a timeout occurs? Shouldn't we print deployment/pod status when any error occurs? We can ignore "not found" errors in case something happened before the operator deployment was created.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@joelanford WDYT? I know you have mentioned to check only for deadline exceeded situation.

if depCheckErr := c.printDeploymentErrors(ctx, key, csv); depCheckErr != nil {
return fmt.Errorf("error printing operator resource errors: %v %v", err, 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
// in case of failures, based on deployment condition.
func (c Client) printDeploymentErrors(ctx context.Context, key types.NamespacedName, csv olmapiv1alpha1.ClusterServiceVersion) error {
Comment thread
bharathi-tenneti marked this conversation as resolved.
if key.Namespace == "" {
return fmt.Errorf("no namespace provided to get deployment failures")
}
dep := &appsv1.Deployment{}
for _, ds := range csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs {
depKey := types.NamespacedName{
Namespace: key.Namespace,
Name: ds.Name,
}
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)
continue
}
for _, s := range dep.Status.Conditions {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ideally you shouldn't interleave network calls and print statements; instead all errors should be collected, then printed together. Doing so is ok for now, but can you add a TODO to separate these two actions?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, so modify these two functions to return errors back to DoCSVWait() instead of printing.?

Copy link
Copy Markdown
Member

@estroz estroz Sep 9, 2020

Choose a reason for hiding this comment

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

Not necessarily. printDeploymentErrors() should look something like (in pseudocode):

depErrs := make(map[string]string)
podErrsForDep := make(map[string]map[string]string)
for dep in deployements {
	for cond in dep.Status.Conditions {
		if isStatusNotAvailable(dep) {
			depErrs[dep.Name] = dep.Status.Reason
			podErrsForDep[dep.Name] = getPodErrs(dep)
			break
		}
	}
}

for depName, depReason in depErrs {
	printf("dep %q error: %s\n", depName, depReason)
	for podName, podErr in podErrsForDep[depName] {
		printf("\tpod %q error: %s\n", podName, podErr)
	}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added TODO comments , and addressed rest of the suggestions.

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
}
}
}
}
return nil
}

// 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.
podErrors := make(map[string]string)
podList := &corev1.PodList{}
podLabelSelectors, err := metav1.LabelSelectorAsSelector(depSelectors)
if err != nil {
return err
}
options := client.ListOptions{
LabelSelector: podLabelSelectors,
Namespace: key.Namespace,
}
if err := c.KubeClient.List(ctx, podList, &options); err != nil {
Comment thread
bharathi-tenneti marked this conversation as resolved.
return fmt.Errorf("error getting Pods: %v", err)
}
for _, p := range podList.Items {
if p.Status.Phase != corev1.PodSucceeded {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Succeeded means the Pod finished and exited. For the operator pods, they should be running all the time to monitor the CRs, so, the expected status should be Running, not the Succeeded.

for _, cs := range p.Status.ContainerStatuses {
if !cs.Ready {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sometimes, there are multi containers in a Pod.

podErrors[p.Name] = cs.State.Waiting.Message
}
}
}
}
if len(podErrors) > 0 {
log.Printf("pod errors: %v\n", podErrors)
}
return nil
}

// GetInstalledVersion returns the OLM version installed in the namespace informed.
Expand Down
27 changes: 27 additions & 0 deletions internal/olm/client/client_suite_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// Copyright 2020 The Operator-SDK Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package client
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This file needs the license header that's why it's failing travis.


import (
"testing"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
)

func TestClient(t *testing.T) {
RegisterFailHandler(Fail)
RunSpecs(t, "check depErrors")
}
196 changes: 196 additions & 0 deletions internal/olm/client/client_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
// Copyright 2020 The Operator-SDK Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package client

import (
"context"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
olmapiv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"k8s.io/apimachinery/pkg/types"

"sigs.k8s.io/controller-runtime/pkg/client"
fake "sigs.k8s.io/controller-runtime/pkg/client/fake"
)

var _ = Describe("Client", func() {
Describe("printDeploymentErrors", func() {

var (
fakeClient client.Client
)

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",
},
},
},
},
},
},
&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,
},
},
},
},

&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: "MinimumReplicasUnavailable",
},
{
Type: "Progressing",
Status: "True",
Reason: "NewReplicaSetAvailable",
},
},
},
},
&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",
}
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())
})
})
})
})