Skip to content

Commit 7dab655

Browse files
author
Eric Stroczynski
authored
internal/olm: make CLI output text more consistent (#1902)
* internal/olm: check resource errors and align log statement content * internal/olm/client: return bool and error from HasInstalledResources; check if CR errors are "no kind match" if their CRD is "not found" * internal/olm/client: DoCreate and DoDelete handle "not found" errors correctly
1 parent 052560a commit 7dab655

File tree

3 files changed

+99
-33
lines changed

3 files changed

+99
-33
lines changed

‎internal/olm/client.go

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,11 @@ func (c Client) InstallVersion(ctx context.Context, version string) (*olmresourc
7272
objs := toObjects(resources...)
7373

7474
status := c.GetObjectsStatus(ctx, objs...)
75-
if status.HasExistingResources() {
75+
installed, err := status.HasInstalledResources()
76+
if installed {
7677
return nil, errors.New("detected existing OLM resources: OLM must be completely uninstalled before installation")
78+
} else if err != nil {
79+
return nil, errors.New("detected errored OLM resources, see resource statuses for more details")
7780
}
7881

7982
log.Print("Creating CRDs and resources")
@@ -129,7 +132,8 @@ func (c Client) UninstallVersion(ctx context.Context, version string) error {
129132
objs := toObjects(resources...)
130133

131134
status := c.GetObjectsStatus(ctx, objs...)
132-
if !status.HasExistingResources() {
135+
installed, err := status.HasInstalledResources()
136+
if !installed && err == nil {
133137
return olmresourceclient.ErrOLMNotInstalled
134138
}
135139

@@ -148,7 +152,8 @@ func (c Client) GetStatus(ctx context.Context, version string) (*olmresourceclie
148152
objs := toObjects(resources...)
149153

150154
status := c.GetObjectsStatus(ctx, objs...)
151-
if !status.HasExistingResources() {
155+
installed, err := status.HasInstalledResources()
156+
if !installed && err == nil {
152157
return nil, olmresourceclient.ErrOLMNotInstalled
153158
}
154159
return &status, nil

‎internal/olm/client/client.go

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -89,11 +89,10 @@ func (c Client) DoCreate(ctx context.Context, objs ...runtime.Object) error {
8989
log.Infof(" Creating %s %q", kind, getName(a.GetNamespace(), a.GetName()))
9090
err = c.KubeClient.Create(ctx, obj)
9191
if err != nil {
92-
if apierrors.IsAlreadyExists(err) {
93-
log.Infof(" %s %q already exists", kind, getName(a.GetNamespace(), a.GetName()))
94-
return nil
92+
if !apierrors.IsAlreadyExists(err) {
93+
return err
9594
}
96-
return err
95+
log.Infof(" %s %q already exists", kind, getName(a.GetNamespace(), a.GetName()))
9796
}
9897
}
9998
return nil
@@ -109,19 +108,19 @@ func (c Client) DoDelete(ctx context.Context, objs ...runtime.Object) error {
109108
log.Infof(" Deleting %s %q", kind, getName(a.GetNamespace(), a.GetName()))
110109
err = c.KubeClient.Delete(ctx, obj, client.PropagationPolicy(metav1.DeletePropagationForeground))
111110
if err != nil {
112-
if apierrors.IsNotFound(err) {
113-
log.Infof(" %s %q does not exist", kind, getName(a.GetNamespace(), a.GetName()))
114-
continue
111+
if !apierrors.IsNotFound(err) {
112+
return err
115113
}
116-
return err
114+
log.Infof(" %s %q does not exist", kind, getName(a.GetNamespace(), a.GetName()))
117115
}
118116
}
119117

120118
log.Infof(" Waiting for deleted resources to disappear")
121119

122120
return wait.PollImmediateUntil(time.Second, func() (bool, error) {
123121
s := c.GetObjectsStatus(ctx, objs...)
124-
return !s.HasExistingResources(), nil
122+
installed, err := s.HasInstalledResources()
123+
return !installed, err
125124
}, ctx.Done())
126125
}
127126

@@ -151,27 +150,27 @@ func (c Client) DoRolloutWait(ctx context.Context, key types.NamespacedName) err
151150
}
152151
if deployment.Spec.Replicas != nil && deployment.Status.UpdatedReplicas < *deployment.Spec.Replicas {
153152
onceReplicasUpdated.Do(func() {
154-
log.Printf(" Waiting for deployment %q to rollout: %d out of %d new replicas have been updated", deployment.Name, deployment.Status.UpdatedReplicas, *deployment.Spec.Replicas)
153+
log.Printf(" Waiting for Deployment %q to rollout: %d out of %d new replicas have been updated", key, deployment.Status.UpdatedReplicas, *deployment.Spec.Replicas)
155154
})
156155
return false, nil
157156
}
158157
if deployment.Status.Replicas > deployment.Status.UpdatedReplicas {
159158
oncePendingTermination.Do(func() {
160-
log.Printf(" Waiting for deployment %q to rollout: %d old replicas are pending termination", deployment.Name, deployment.Status.Replicas-deployment.Status.UpdatedReplicas)
159+
log.Printf(" Waiting for Deployment %q to rollout: %d old replicas are pending termination", key, deployment.Status.Replicas-deployment.Status.UpdatedReplicas)
161160
})
162161
return false, nil
163162
}
164163
if deployment.Status.AvailableReplicas < deployment.Status.UpdatedReplicas {
165164
onceNotAvailable.Do(func() {
166-
log.Printf(" Waiting for deployment %q to rollout: %d of %d updated replicas are available", deployment.Name, deployment.Status.AvailableReplicas, deployment.Status.UpdatedReplicas)
165+
log.Printf(" Waiting for Deployment %q to rollout: %d of %d updated replicas are available", key, deployment.Status.AvailableReplicas, deployment.Status.UpdatedReplicas)
167166
})
168167
return false, nil
169168
}
170-
log.Printf(" Deployment %q successfully rolled out", deployment.Name)
169+
log.Printf(" Deployment %q successfully rolled out", key)
171170
return true, nil
172171
}
173172
onceSpecUpdate.Do(func() {
174-
log.Printf(" Waiting for deployment %q to rollout: waiting for deployment spec update to be observed", deployment.Name)
173+
log.Printf(" Waiting for Deployment %q to rollout: waiting for deployment spec update to be observed", key)
175174
})
176175
return false, nil
177176
}
@@ -191,7 +190,7 @@ func (c Client) DoCSVWait(ctx context.Context, key types.NamespacedName) error {
191190
if err != nil {
192191
if apierrors.IsNotFound(err) {
193192
once.Do(func() {
194-
log.Printf(" Waiting for clusterserviceversion %q to appear", key.Name)
193+
log.Printf(" Waiting for ClusterServiceVersion %q to appear", key)
195194
})
196195
return false, nil
197196
}
@@ -200,7 +199,7 @@ func (c Client) DoCSVWait(ctx context.Context, key types.NamespacedName) error {
200199
newPhase = csv.Status.Phase
201200
if newPhase != curPhase {
202201
curPhase = newPhase
203-
log.Printf(" Found clusterserviceversion %q phase: %s", key.Name, curPhase)
202+
log.Printf(" Found ClusterServiceVersion %q phase: %s", key, curPhase)
204203
}
205204
return curPhase == olmapiv1alpha1.CSVPhaseSucceeded, nil
206205
}

‎internal/olm/client/status.go

Lines changed: 76 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -20,15 +20,20 @@ package olm
2020
import (
2121
"bytes"
2222
"context"
23+
"errors"
2324
"fmt"
25+
"sort"
2426
"text/tabwriter"
2527

2628
log "github.com/sirupsen/logrus"
29+
"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
30+
apierrors "k8s.io/apimachinery/pkg/api/errors"
2731
"k8s.io/apimachinery/pkg/api/meta"
2832
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
2933
"k8s.io/apimachinery/pkg/runtime"
3034
"k8s.io/apimachinery/pkg/runtime/schema"
3135
"k8s.io/apimachinery/pkg/types"
36+
"k8s.io/apimachinery/pkg/util/sets"
3237
)
3338

3439
type Status struct {
@@ -40,29 +45,31 @@ type ResourceStatus struct {
4045
Resource *unstructured.Unstructured
4146
GVK schema.GroupVersionKind
4247
Error error
48+
49+
requestObject runtime.Object // Needed for context on errors from requests on an object.
4350
}
4451

4552
func (c Client) GetObjectsStatus(ctx context.Context, objs ...runtime.Object) Status {
4653
var rss []ResourceStatus
4754
for _, obj := range objs {
55+
gvk := obj.GetObjectKind().GroupVersionKind()
4856
a, aerr := meta.Accessor(obj)
4957
if aerr != nil {
50-
log.Fatalf("Object %s: %v", obj.GetObjectKind().GroupVersionKind(), aerr)
58+
log.Fatalf("Object %s: %v", gvk, aerr)
5159
}
5260
nn := types.NamespacedName{
5361
Namespace: a.GetNamespace(),
5462
Name: a.GetName(),
5563
}
56-
u := unstructured.Unstructured{}
57-
u.SetGroupVersionKind(obj.GetObjectKind().GroupVersionKind())
58-
err := c.KubeClient.Get(ctx, nn, &u)
5964
rs := ResourceStatus{
6065
NamespacedName: nn,
61-
GVK: obj.GetObjectKind().GroupVersionKind(),
66+
GVK: gvk,
67+
requestObject: obj,
6268
}
63-
if err != nil {
64-
rs.Error = err
65-
} else {
69+
u := unstructured.Unstructured{}
70+
u.SetGroupVersionKind(gvk)
71+
rs.Error = c.KubeClient.Get(ctx, nn, &u)
72+
if rs.Error == nil {
6673
rs.Resource = &u
6774
}
6875
rss = append(rss, rs)
@@ -71,13 +78,68 @@ func (c Client) GetObjectsStatus(ctx context.Context, objs ...runtime.Object) St
7178
return Status{Resources: rss}
7279
}
7380

74-
func (s Status) HasExistingResources() bool {
81+
// HasInstalledResources only returns true if at least one resource in s
82+
// was returned successfully by the API server. A resource error status
83+
// containing any error except "not found", or "no kind match" errors
84+
// for Custom Resources, will result in HasInstalledResources returning
85+
// false and the error.
86+
func (s Status) HasInstalledResources() (bool, error) {
87+
crdKindSet, err := s.getCRDKindSet()
88+
if err != nil {
89+
return false, fmt.Errorf("error getting set of CRD kinds in resources: %w", err)
90+
}
91+
// Sort resources by whether they're installed or not to get consistent
92+
// return values.
93+
sort.Slice(s.Resources, func(i int, j int) bool {
94+
return s.Resources[i].Resource != nil
95+
})
7596
for _, r := range s.Resources {
7697
if r.Resource != nil {
77-
return true
98+
return true, nil
99+
} else if r.Error != nil && !apierrors.IsNotFound(r.Error) {
100+
// We know the error is not a "resource not found" error at this point.
101+
// It still may be the equivalent for a CR, "no kind match", if its
102+
// corresponding CRD has been deleted already. We want to make sure
103+
// we're only allowing "no kind match" errors to be skipped for CR's
104+
// since we do not know if a kind is a CR kind, hence checking
105+
// crdKindSet for existence of a resource's kind.
106+
nkmerr := &meta.NoKindMatchError{}
107+
if !errors.As(r.Error, &nkmerr) || !crdKindSet.Has(r.GVK.Kind) {
108+
return false, r.Error
109+
}
78110
}
79111
}
80-
return false
112+
return false, nil
113+
}
114+
115+
// getCRDKindSet returns the set of all kinds specified by all CRDs in s.
116+
func (s Status) getCRDKindSet() (sets.String, error) {
117+
crdKindSet := sets.NewString()
118+
for _, r := range s.Resources {
119+
if r.GVK.Kind == "CustomResourceDefinition" {
120+
u := &unstructured.Unstructured{}
121+
switch v := r.requestObject.(type) {
122+
case *unstructured.Unstructured:
123+
u = v
124+
default:
125+
uObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&v)
126+
if err != nil {
127+
return nil, err
128+
}
129+
// Other fields are not important, just the CRD kind.
130+
u.Object = uObj
131+
}
132+
// Use unversioned CustomResourceDefinition to avoid implementing cast
133+
// for all versions.
134+
crd := &apiextensions.CustomResourceDefinition{}
135+
err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &crd)
136+
if err != nil {
137+
return nil, err
138+
}
139+
crdKindSet.Insert(crd.Spec.Names.Kind)
140+
}
141+
}
142+
return crdKindSet, nil
81143
}
82144

83145
func (s Status) String() string {
@@ -88,10 +150,10 @@ func (s Status) String() string {
88150
nn := r.NamespacedName
89151
kind := r.GVK.Kind
90152
var status string
91-
if r.Resource != nil {
92-
status = "Installed"
93-
} else if r.Error != nil {
153+
if r.Error != nil {
94154
status = r.Error.Error()
155+
} else if r.Resource != nil {
156+
status = "Installed"
95157
} else {
96158
status = "Unknown"
97159
}

0 commit comments

Comments
 (0)