Skip to content

Commit 7fc883c

Browse files
benluddybenluddy
committed
Preserve original .status.lastUpdateTime in copied CSVs.
Currently, CSVs are mirrored into the namespaces that are watched by the operator they represent. At the moment a copied CSV is updated, its .status.lastUpdateTime field is set to the current time, rather than using the same timestamp that appears on the original CSV's status. As the size of a cluster grows, it becomes increasingly unlikely that these two timestamps will have the same value. This means that there's almost always a non-empty diff between the original and any given copy, thereby triggering an update to the copy even if nothing else has changed in the original.
1 parent cfda6a3 commit 7fc883c

File tree

3 files changed

+286
-2
lines changed

3 files changed

+286
-2
lines changed

‎pkg/controller/operators/olm/operatorgroup.go

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -734,7 +734,6 @@ func (a *Operator) copyToNamespace(csv *v1alpha1.ClusterServiceVersion, namespac
734734
logger.Debug("updating status")
735735
// Must use fetchedCSV because UpdateStatus(...) checks resource UID.
736736
fetchedCSV.Status = newCSV.Status
737-
fetchedCSV.Status.LastUpdateTime = a.now()
738737
if fetchedCSV, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(namespace).UpdateStatus(context.TODO(), fetchedCSV, metav1.UpdateOptions{}); err != nil {
739738
logger.WithError(err).Error("status update for target CSV failed")
740739
return nil, err
@@ -756,7 +755,6 @@ func (a *Operator) copyToNamespace(csv *v1alpha1.ClusterServiceVersion, namespac
756755
}
757756
createdCSV.Status.Reason = v1alpha1.CSVReasonCopied
758757
createdCSV.Status.Message = fmt.Sprintf("The operator is running in %s but is managing this namespace", csv.GetNamespace())
759-
createdCSV.Status.LastUpdateTime = a.now()
760758
if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(namespace).UpdateStatus(context.TODO(), createdCSV, metav1.UpdateOptions{}); err != nil {
761759
a.logger.Errorf("Status update for CSV failed: %v", err)
762760
return nil, err

‎pkg/controller/operators/olm/operatorgroup_test.go

Lines changed: 199 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,199 @@
1+
package olm
2+
3+
import (
4+
"io/ioutil"
5+
"testing"
6+
"time"
7+
8+
"github.com/sirupsen/logrus"
9+
"github.com/stretchr/testify/require"
10+
11+
"k8s.io/apimachinery/pkg/api/errors"
12+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
13+
"k8s.io/apimachinery/pkg/labels"
14+
ktesting "k8s.io/client-go/testing"
15+
16+
"github.com/operator-framework/api/pkg/operators/v1alpha1"
17+
"github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned/fake"
18+
listersv1alpha1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/listers/operators/v1alpha1"
19+
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister/operatorlisterfakes"
20+
)
21+
22+
func TestCopyToNamespace(t *testing.T) {
23+
gvr := v1alpha1.SchemeGroupVersion.WithResource("clusterserviceversions")
24+
25+
for _, tc := range []struct {
26+
Name string
27+
Namespace string
28+
Original *v1alpha1.ClusterServiceVersion
29+
ExistingCopy *v1alpha1.ClusterServiceVersion
30+
ExpectedResult *v1alpha1.ClusterServiceVersion
31+
ExpectedError string
32+
ExpectedActions []ktesting.Action
33+
}{
34+
{
35+
Name: "copy to original namespace returns error",
36+
Namespace: "foo",
37+
Original: &v1alpha1.ClusterServiceVersion{
38+
ObjectMeta: metav1.ObjectMeta{
39+
Namespace: "foo",
40+
},
41+
},
42+
ExpectedError: "bug: can not copy to active namespace foo",
43+
},
44+
{
45+
Name: "status updated if meaningfully changed",
46+
Namespace: "bar",
47+
Original: &v1alpha1.ClusterServiceVersion{
48+
ObjectMeta: metav1.ObjectMeta{
49+
Name: "name",
50+
Namespace: "foo",
51+
},
52+
Status: v1alpha1.ClusterServiceVersionStatus{
53+
LastUpdateTime: &metav1.Time{Time: time.Unix(2, 0)},
54+
},
55+
},
56+
ExistingCopy: &v1alpha1.ClusterServiceVersion{
57+
ObjectMeta: metav1.ObjectMeta{
58+
Name: "name",
59+
Namespace: "bar",
60+
},
61+
Status: v1alpha1.ClusterServiceVersionStatus{
62+
LastUpdateTime: &metav1.Time{Time: time.Unix(1, 0)},
63+
},
64+
},
65+
ExpectedResult: &v1alpha1.ClusterServiceVersion{
66+
ObjectMeta: metav1.ObjectMeta{
67+
Name: "name",
68+
Namespace: "bar",
69+
},
70+
Status: v1alpha1.ClusterServiceVersionStatus{
71+
LastUpdateTime: &metav1.Time{Time: time.Unix(2, 0)},
72+
Message: "The operator is running in foo but is managing this namespace",
73+
Reason: v1alpha1.CSVReasonCopied,
74+
},
75+
},
76+
ExpectedActions: []ktesting.Action{
77+
ktesting.NewUpdateSubresourceAction(gvr, "status", "bar", &v1alpha1.ClusterServiceVersion{
78+
ObjectMeta: metav1.ObjectMeta{
79+
Name: "name",
80+
Namespace: "bar",
81+
},
82+
Status: v1alpha1.ClusterServiceVersionStatus{
83+
LastUpdateTime: &metav1.Time{Time: time.Unix(2, 0)},
84+
Message: "The operator is running in foo but is managing this namespace",
85+
Reason: v1alpha1.CSVReasonCopied,
86+
},
87+
}),
88+
},
89+
},
90+
{
91+
Name: "status not updated if not meaningfully changed",
92+
Namespace: "bar",
93+
Original: &v1alpha1.ClusterServiceVersion{
94+
ObjectMeta: metav1.ObjectMeta{
95+
Name: "name",
96+
Namespace: "foo",
97+
},
98+
Status: v1alpha1.ClusterServiceVersionStatus{
99+
LastUpdateTime: &metav1.Time{Time: time.Unix(2, 0)},
100+
},
101+
},
102+
ExistingCopy: &v1alpha1.ClusterServiceVersion{
103+
ObjectMeta: metav1.ObjectMeta{
104+
Name: "name",
105+
Namespace: "bar",
106+
},
107+
Status: v1alpha1.ClusterServiceVersionStatus{
108+
LastUpdateTime: &metav1.Time{Time: time.Unix(2, 0)},
109+
Message: "The operator is running in foo but is managing this namespace",
110+
Reason: v1alpha1.CSVReasonCopied},
111+
},
112+
ExpectedResult: &v1alpha1.ClusterServiceVersion{
113+
ObjectMeta: metav1.ObjectMeta{
114+
Name: "name",
115+
Namespace: "bar",
116+
},
117+
Status: v1alpha1.ClusterServiceVersionStatus{
118+
LastUpdateTime: &metav1.Time{Time: time.Unix(2, 0)},
119+
Message: "The operator is running in foo but is managing this namespace",
120+
Reason: v1alpha1.CSVReasonCopied,
121+
},
122+
},
123+
},
124+
} {
125+
t.Run(tc.Name, func(t *testing.T) {
126+
lister := &operatorlisterfakes.FakeOperatorLister{}
127+
v1alpha1lister := &operatorlisterfakes.FakeOperatorsV1alpha1Lister{}
128+
lister.OperatorsV1alpha1Returns(v1alpha1lister)
129+
130+
client := fake.NewSimpleClientset()
131+
if tc.ExistingCopy != nil {
132+
client = fake.NewSimpleClientset(tc.ExistingCopy)
133+
v1alpha1lister.ClusterServiceVersionListerReturns(FakeClusterServiceVersionLister{tc.ExistingCopy})
134+
}
135+
136+
logger := logrus.New()
137+
logger.SetOutput(ioutil.Discard)
138+
139+
o := &Operator{
140+
lister: lister,
141+
client: client,
142+
logger: logger,
143+
}
144+
result, err := o.copyToNamespace(tc.Original, tc.Namespace)
145+
146+
require := require.New(t)
147+
if tc.ExpectedError == "" {
148+
require.NoError(err)
149+
} else {
150+
require.EqualError(err, tc.ExpectedError)
151+
}
152+
require.Equal(tc.ExpectedResult, result)
153+
154+
actions := client.Actions()
155+
if len(actions) == 0 {
156+
actions = nil
157+
}
158+
require.Equal(tc.ExpectedActions, actions)
159+
})
160+
}
161+
}
162+
163+
type FakeClusterServiceVersionLister []*v1alpha1.ClusterServiceVersion
164+
165+
func (l FakeClusterServiceVersionLister) List(selector labels.Selector) ([]*v1alpha1.ClusterServiceVersion, error) {
166+
var result []*v1alpha1.ClusterServiceVersion
167+
for _, csv := range l {
168+
if !selector.Matches(labels.Set(csv.GetLabels())) {
169+
continue
170+
}
171+
result = append(result, csv)
172+
}
173+
return result, nil
174+
}
175+
176+
func (l FakeClusterServiceVersionLister) ClusterServiceVersions(namespace string) listersv1alpha1.ClusterServiceVersionNamespaceLister {
177+
var filtered []*v1alpha1.ClusterServiceVersion
178+
for _, csv := range l {
179+
if csv.GetNamespace() != namespace {
180+
continue
181+
}
182+
filtered = append(filtered, csv)
183+
}
184+
return FakeClusterServiceVersionLister(filtered)
185+
}
186+
187+
func (l FakeClusterServiceVersionLister) Get(name string) (*v1alpha1.ClusterServiceVersion, error) {
188+
for _, csv := range l {
189+
if csv.GetName() == name {
190+
return csv, nil
191+
}
192+
}
193+
return nil, errors.NewNotFound(v1alpha1.Resource("clusterserviceversion"), name)
194+
}
195+
196+
var (
197+
_ listersv1alpha1.ClusterServiceVersionLister = FakeClusterServiceVersionLister{}
198+
_ listersv1alpha1.ClusterServiceVersionNamespaceLister = FakeClusterServiceVersionLister{}
199+
)

‎test/e2e/csv_e2e_test.go

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"k8s.io/apimachinery/pkg/util/wait"
2424
"k8s.io/apimachinery/pkg/watch"
2525
apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1"
26+
"sigs.k8s.io/controller-runtime/pkg/client"
2627

2728
v1 "github.com/operator-framework/api/pkg/operators/v1"
2829
"github.com/operator-framework/api/pkg/operators/v1alpha1"
@@ -47,6 +48,92 @@ var _ = Describe("ClusterServiceVersion", func() {
4748
TearDown(testNamespace)
4849
})
4950

51+
When("a copied csv exists", func() {
52+
var (
53+
target corev1.Namespace
54+
original v1alpha1.ClusterServiceVersion
55+
copy v1alpha1.ClusterServiceVersion
56+
)
57+
58+
BeforeEach(func() {
59+
target = corev1.Namespace{
60+
ObjectMeta: metav1.ObjectMeta{
61+
GenerateName: "watched-",
62+
},
63+
}
64+
Expect(ctx.Ctx().Client().Create(context.Background(), &target)).To(Succeed())
65+
66+
original = v1alpha1.ClusterServiceVersion{
67+
TypeMeta: metav1.TypeMeta{
68+
Kind: v1alpha1.ClusterServiceVersionKind,
69+
APIVersion: v1alpha1.ClusterServiceVersionAPIVersion,
70+
},
71+
ObjectMeta: metav1.ObjectMeta{
72+
GenerateName: "csv-",
73+
Namespace: testNamespace,
74+
},
75+
Spec: v1alpha1.ClusterServiceVersionSpec{
76+
InstallStrategy: newNginxInstallStrategy(genName("csv-"), nil, nil),
77+
InstallModes: []v1alpha1.InstallMode{
78+
{
79+
Type: v1alpha1.InstallModeTypeAllNamespaces,
80+
Supported: true,
81+
},
82+
},
83+
},
84+
}
85+
Expect(ctx.Ctx().Client().Create(context.Background(), &original)).To(Succeed())
86+
87+
Eventually(func() error {
88+
key, err := client.ObjectKeyFromObject(&original)
89+
Expect(err).ToNot(HaveOccurred())
90+
key.Namespace = target.GetName()
91+
return ctx.Ctx().Client().Get(context.Background(), key, &copy)
92+
}).Should(Succeed())
93+
})
94+
95+
AfterEach(func() {
96+
if target.GetName() != "" {
97+
Expect(ctx.Ctx().Client().Delete(context.Background(), &target)).To(Succeed())
98+
}
99+
})
100+
101+
It("is synchronized with the original csv", func() {
102+
Eventually(func() error {
103+
key, err := client.ObjectKeyFromObject(&copy)
104+
if err != nil {
105+
return err
106+
}
107+
108+
key.Namespace = target.Name
109+
if err := ctx.Ctx().Client().Get(context.Background(), key, &copy); err != nil {
110+
return err
111+
}
112+
113+
copy.Status.LastUpdateTime = &metav1.Time{Time: time.Unix(1, 0)}
114+
return ctx.Ctx().Client().Status().Update(context.Background(), &copy)
115+
}).Should(Succeed())
116+
117+
Eventually(func() (bool, error) {
118+
key, err := client.ObjectKeyFromObject(&original)
119+
if err != nil {
120+
return false, err
121+
}
122+
123+
if err := ctx.Ctx().Client().Get(context.Background(), key, &original); err != nil {
124+
return false, err
125+
}
126+
127+
key.Namespace = target.Name
128+
if err := ctx.Ctx().Client().Get(context.Background(), key, &copy); err != nil {
129+
return false, err
130+
}
131+
132+
return original.Status.LastUpdateTime.Equal(copy.Status.LastUpdateTime), nil
133+
}).Should(BeTrue(), "Change to status of copy should have been reverted")
134+
})
135+
})
136+
50137
It("create with unmet requirements min kube version", func() {
51138

52139
depName := genName("dep-")

0 commit comments

Comments
 (0)