Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit de74420

Browse files
njhalenjhale
committedMay 18, 2021
fix: elide channels with deprecated bundle heads
Signed-off-by: Nick Hale <[email protected]>
1 parent 7e5150e commit de74420

File tree

4 files changed

+136
-102
lines changed

4 files changed

+136
-102
lines changed
 

‎pkg/registry/populator_test.go

+23-29
Original file line numberDiff line numberDiff line change
@@ -741,7 +741,7 @@ func TestDeprecateBundle(t *testing.T) {
741741
expected expected
742742
}{
743743
{
744-
description: "BundleDeprecated/IgnoreIfNotInIndex",
744+
description: "IgnoreIfNotInIndex",
745745
args: args{
746746
bundles: []string{
747747
"quay.io/test/etcd.0.6.0",
@@ -776,7 +776,7 @@ func TestDeprecateBundle(t *testing.T) {
776776
},
777777
},
778778
{
779-
description: "BundleDeprecated/SingleChannel",
779+
description: "ChannelRemoved",
780780
args: args{
781781
bundles: []string{
782782
"quay.io/test/prometheus.0.15.0",
@@ -790,13 +790,11 @@ func TestDeprecateBundle(t *testing.T) {
790790
"quay.io/test/etcd.0.9.0/stable",
791791
"quay.io/test/etcd.0.9.2/stable",
792792
"quay.io/test/etcd.0.9.2/alpha",
793-
"quay.io/test/prometheus.0.15.0/preview",
794-
"quay.io/test/prometheus.0.15.0/stable",
795793
"quay.io/test/prometheus.0.22.2/preview",
794+
"quay.io/test/prometheus.0.15.0/preview",
796795
},
797796
deprecatedBundles: []string{
798797
"quay.io/test/prometheus.0.15.0/preview",
799-
"quay.io/test/prometheus.0.15.0/stable",
800798
},
801799
remainingPkgChannels: pkgChannel{
802800
"etcd": []string{
@@ -806,35 +804,35 @@ func TestDeprecateBundle(t *testing.T) {
806804
},
807805
"prometheus": []string{
808806
"preview",
809-
"stable",
810807
},
811808
},
812809
},
813810
},
814811
{
815-
description: "BundleDeprecated/ChannelRemoved",
812+
description: "ChannelRemoved/ErrorOnDefault",
816813
args: args{
817814
bundles: []string{
818-
"quay.io/test/etcd.0.9.2",
815+
"quay.io/test/prometheus.0.22.2",
819816
},
820817
},
821818
expected: expected{
822-
err: nil,
819+
err: errors.NewAggregate([]error{fmt.Errorf("error deprecating bundle quay.io/test/prometheus.0.22.2: %s", registry.ErrRemovingDefaultChannelDuringDeprecation)}),
823820
remainingBundles: []string{
824-
"quay.io/test/etcd.0.9.2/alpha",
821+
"quay.io/test/etcd.0.9.0/alpha",
822+
"quay.io/test/etcd.0.9.0/beta",
823+
"quay.io/test/etcd.0.9.0/stable",
825824
"quay.io/test/etcd.0.9.2/stable",
825+
"quay.io/test/etcd.0.9.2/alpha",
826826
"quay.io/test/prometheus.0.22.2/preview",
827-
"quay.io/test/prometheus.0.14.0/preview",
828-
"quay.io/test/prometheus.0.14.0/stable",
829827
"quay.io/test/prometheus.0.15.0/preview",
830828
"quay.io/test/prometheus.0.15.0/stable",
829+
"quay.io/test/prometheus.0.14.0/preview",
830+
"quay.io/test/prometheus.0.14.0/stable",
831831
},
832-
deprecatedBundles: []string{
833-
"quay.io/test/etcd.0.9.2/alpha",
834-
"quay.io/test/etcd.0.9.2/stable",
835-
},
832+
deprecatedBundles: []string{},
836833
remainingPkgChannels: pkgChannel{
837834
"etcd": []string{
835+
"beta",
838836
"alpha",
839837
"stable",
840838
},
@@ -860,9 +858,7 @@ func TestDeprecateBundle(t *testing.T) {
860858
require.NoError(t, err)
861859

862860
deprecator := sqlite.NewSQLDeprecatorForBundles(store, tt.args.bundles)
863-
err = deprecator.Deprecate()
864-
fmt.Printf("error: %s\n", err)
865-
require.Equal(t, tt.expected.err, err)
861+
require.Equal(t, tt.expected.err, deprecator.Deprecate())
866862

867863
// Ensure remaining bundlePaths in db match
868864
bundles, err := querier.ListBundles(context.Background())
@@ -938,7 +934,7 @@ func TestAddAfterDeprecate(t *testing.T) {
938934
"prometheus.0.15.0",
939935
},
940936
deprecate: []string{
941-
"quay.io/test/prometheus.0.15.0",
937+
"quay.io/test/prometheus.0.14.0",
942938
},
943939
add: []string{
944940
"prometheus.0.22.2",
@@ -947,13 +943,15 @@ func TestAddAfterDeprecate(t *testing.T) {
947943
expected: expected{
948944
err: nil,
949945
remaining: []string{
946+
"quay.io/test/prometheus.0.22.2/preview",
950947
"quay.io/test/prometheus.0.15.0/preview",
948+
"quay.io/test/prometheus.0.14.0/preview",
951949
"quay.io/test/prometheus.0.15.0/stable",
952-
"quay.io/test/prometheus.0.22.2/preview",
950+
"quay.io/test/prometheus.0.14.0/stable",
953951
},
954952
deprecated: []string{
955-
"quay.io/test/prometheus.0.15.0/preview",
956-
"quay.io/test/prometheus.0.15.0/stable",
953+
"quay.io/test/prometheus.0.14.0/preview",
954+
"quay.io/test/prometheus.0.14.0/stable",
957955
},
958956
pkgChannels: pkgChannel{
959957
"prometheus": []string{
@@ -984,18 +982,15 @@ func TestAddAfterDeprecate(t *testing.T) {
984982
expected: expected{
985983
err: nil,
986984
remaining: []string{
987-
"quay.io/test/prometheus.0.15.0/preview",
988-
"quay.io/test/prometheus.0.15.0/stable",
989985
"quay.io/test/prometheus.0.22.2/preview",
986+
"quay.io/test/prometheus.0.15.0/preview",
990987
},
991988
deprecated: []string{
992989
"quay.io/test/prometheus.0.15.0/preview",
993-
"quay.io/test/prometheus.0.15.0/stable",
994990
},
995991
pkgChannels: pkgChannel{
996992
"prometheus": []string{
997993
"preview",
998-
"stable",
999994
},
1000995
},
1001996
},
@@ -1045,8 +1040,7 @@ func TestAddAfterDeprecate(t *testing.T) {
10451040
require.NoError(t, populate(tt.args.existing, nil))
10461041

10471042
deprecator := sqlite.NewSQLDeprecatorForBundles(load, tt.args.deprecate)
1048-
err = deprecator.Deprecate()
1049-
require.Equal(t, tt.expected.err, err)
1043+
require.Equal(t, tt.expected.err, deprecator.Deprecate())
10501044

10511045
require.NoError(t, populate(tt.args.add, tt.args.overwrite))
10521046

‎pkg/sqlite/deprecate.go

+2-4
Original file line numberDiff line numberDiff line change
@@ -31,17 +31,15 @@ func NewSQLDeprecatorForBundles(store registry.Load, bundles []string) *BundleDe
3131

3232
func (d *BundleDeprecator) Deprecate() error {
3333
log := logrus.WithField("bundles", d.bundles)
34-
3534
log.Info("deprecating bundles")
3635

3736
var errs []error
38-
3937
for _, bundlePath := range d.bundles {
4038
if err := d.store.DeprecateBundle(bundlePath); err != nil {
39+
errs = append(errs, fmt.Errorf("error deprecating bundle %s: %s", bundlePath, err))
4140
if !errors.Is(err, registry.ErrBundleImageNotInDatabase) && !errors.Is(err, registry.ErrRemovingDefaultChannelDuringDeprecation) {
42-
return utilerrors.NewAggregate(append(errs, fmt.Errorf("error deprecating bundle %s: %s", bundlePath, err)))
41+
break
4342
}
44-
errs = append(errs, fmt.Errorf("error deprecating bundle %s: %s", bundlePath, err))
4543
}
4644
}
4745

‎pkg/sqlite/load.go

+67-58
Original file line numberDiff line numberDiff line change
@@ -576,9 +576,16 @@ func (s *sqlLoader) addPackageChannels(tx *sql.Tx, manifest registry.PackageMani
576576
return fmt.Errorf("failed to add package %q: %s", manifest.PackageName, err.Error())
577577
}
578578

579-
var errs []error
580-
hasDefault := false
579+
var (
580+
errs []error
581+
channels []registry.PackageChannel
582+
hasDefault bool
583+
)
581584
for _, c := range manifest.Channels {
585+
if deprecated, err := s.deprecated(tx, c.CurrentCSVName); err != nil || deprecated {
586+
// Elide channels that start with a deprecated bundle
587+
continue
588+
}
582589
if _, err := addChannel.Exec(c.Name, manifest.PackageName, c.CurrentCSVName); err != nil {
583590
errs = append(errs, fmt.Errorf("failed to add channel %q in package %q: %s", c.Name, manifest.PackageName, err.Error()))
584591
continue
@@ -590,12 +597,13 @@ func (s *sqlLoader) addPackageChannels(tx *sql.Tx, manifest registry.PackageMani
590597
continue
591598
}
592599
}
600+
channels = append(channels, c)
593601
}
594602
if !hasDefault {
595603
errs = append(errs, fmt.Errorf("no default channel specified for %s", manifest.PackageName))
596604
}
597605

598-
for _, c := range manifest.Channels {
606+
for _, c := range channels {
599607
res, err := addChannelEntry.Exec(c.Name, manifest.PackageName, c.CurrentCSVName, 0)
600608
if err != nil {
601609
errs = append(errs, fmt.Errorf("failed to add channel %q in package %q: %s", c.Name, manifest.PackageName, err.Error()))
@@ -1268,19 +1276,18 @@ func (s *sqlLoader) addBundleProperties(tx *sql.Tx, bundle *registry.Bundle) err
12681276
}
12691277

12701278
func (s *sqlLoader) rmChannelEntry(tx *sql.Tx, csvName string) error {
1271-
getEntryID := `SELECT entry_id FROM channel_entry WHERE operatorbundle_name=?`
1272-
rows, err := tx.QueryContext(context.TODO(), getEntryID, csvName)
1279+
rows, err := tx.Query(`SELECT entry_id FROM channel_entry WHERE operatorbundle_name=?`, csvName)
12731280
if err != nil {
12741281
return err
12751282
}
1283+
12761284
var entryIDs []int64
12771285
for rows.Next() {
12781286
var entryID sql.NullInt64
12791287
rows.Scan(&entryID)
12801288
entryIDs = append(entryIDs, entryID.Int64)
12811289
}
1282-
err = rows.Close()
1283-
if err != nil {
1290+
if err := rows.Close(); err != nil {
12841291
return err
12851292
}
12861293

@@ -1299,35 +1306,50 @@ func (s *sqlLoader) rmChannelEntry(tx *sql.Tx, csvName string) error {
12991306
return err
13001307
}
13011308

1302-
stmt, err := tx.Prepare("DELETE FROM channel_entry WHERE operatorbundle_name=?")
1309+
_, err = tx.Exec(`DELETE FROM channel WHERE head_operatorbundle_name=?`, csvName)
13031310
if err != nil {
13041311
return err
13051312
}
1306-
defer stmt.Close()
1307-
1308-
if _, err := stmt.Exec(csvName); err != nil {
1309-
return err
1310-
}
13111313

13121314
return nil
13131315
}
13141316

1315-
func getTailFromBundle(tx *sql.Tx, name string) (bundles []string, err error) {
1317+
func getTailFromBundle(tx *sql.Tx, head string) (bundles []string, err error) {
13161318
getReplacesSkips := `SELECT replaces, skips FROM operatorbundle WHERE name=?`
13171319
isDefaultChannelHead := `SELECT head_operatorbundle_name FROM channel
13181320
INNER JOIN package ON channel.name = package.default_channel
13191321
WHERE channel.head_operatorbundle_name = ?`
13201322

1321-
tail := make(map[string]struct{})
1322-
next := name
1323+
visited := map[string]struct{}{}
1324+
next := []string{head}
1325+
1326+
for len(next) > 0 {
1327+
// Pop the next bundle off of the queue
1328+
bundle := next[0]
1329+
next = next[1:] // Potentially inefficient queue implementation, but this function is only used when deprecate is called
13231330

1324-
for next != "" {
1325-
rows, err := tx.QueryContext(context.TODO(), getReplacesSkips, next)
1331+
// Check if next is the head of the defaultChannel
1332+
// If it is, the defaultChannel would be removed -- this is not allowed because we cannot know which channel to promote as the new default
1333+
var err error
1334+
if row := tx.QueryRow(isDefaultChannelHead, bundle); row != nil {
1335+
err = row.Scan(&sql.NullString{})
1336+
}
1337+
if err == nil {
1338+
// A nil error indicates that next is the default channel head
1339+
return nil, registry.ErrRemovingDefaultChannelDuringDeprecation
1340+
} else if err != sql.ErrNoRows {
1341+
return nil, err
1342+
}
1343+
1344+
rows, err := tx.QueryContext(context.TODO(), getReplacesSkips, bundle)
13261345
if err != nil {
13271346
return nil, err
13281347
}
1329-
var replaces sql.NullString
1330-
var skips sql.NullString
1348+
1349+
var (
1350+
replaces sql.NullString
1351+
skips sql.NullString
1352+
)
13311353
if rows.Next() {
13321354
if err := rows.Scan(&replaces, &skips); err != nil {
13331355
if nerr := rows.Close(); nerr != nil {
@@ -1341,49 +1363,32 @@ func getTailFromBundle(tx *sql.Tx, name string) (bundles []string, err error) {
13411363
}
13421364
if skips.Valid && skips.String != "" {
13431365
for _, skip := range strings.Split(skips.String, ",") {
1344-
tail[skip] = struct{}{}
1366+
if _, ok := visited[skip]; ok {
1367+
// We've already visited this bundle's subgraph
1368+
continue
1369+
}
1370+
visited[skip] = struct{}{}
1371+
next = append(next, skip)
13451372
}
13461373
}
13471374
if replaces.Valid && replaces.String != "" {
1348-
// check if replaces is the head of the defaultChannel
1349-
// if it is, the defaultChannel will be removed
1350-
// this is not allowed because we cannot know which channel to promote as the new default
1351-
rows, err := tx.QueryContext(context.TODO(), isDefaultChannelHead, replaces.String)
1352-
if err != nil {
1353-
return nil, err
1354-
}
1355-
if rows.Next() {
1356-
var defaultChannelHead sql.NullString
1357-
err := rows.Scan(&defaultChannelHead)
1358-
if err != nil {
1359-
if nerr := rows.Close(); nerr != nil {
1360-
return nil, nerr
1361-
}
1362-
return nil, err
1363-
}
1364-
if defaultChannelHead.Valid {
1365-
if nerr := rows.Close(); nerr != nil {
1366-
return nil, nerr
1367-
}
1368-
return nil, registry.ErrRemovingDefaultChannelDuringDeprecation
1369-
}
1370-
}
1371-
if err := rows.Close(); err != nil {
1372-
return nil, err
1375+
r := replaces.String
1376+
if _, ok := visited[r]; ok {
1377+
// We've already visited this bundle's subgraph
1378+
continue
13731379
}
1374-
next = replaces.String
1375-
tail[replaces.String] = struct{}{}
1376-
} else {
1377-
next = ""
1380+
visited[r] = struct{}{}
1381+
next = append(next, r)
13781382
}
13791383
}
1380-
var allTails []string
13811384

1382-
for k := range tail {
1383-
allTails = append(allTails, k)
1385+
// The tail is exactly the set of bundles we visited while traversing the graph from head
1386+
var tail []string
1387+
for v := range visited {
1388+
tail = append(tail, v)
13841389
}
13851390

1386-
return allTails, nil
1391+
return tail, nil
13871392

13881393
}
13891394

@@ -1427,16 +1432,20 @@ func (s *sqlLoader) DeprecateBundle(path string) error {
14271432
}
14281433

14291434
for _, bundle := range tailBundles {
1430-
err = s.rmChannelEntry(tx, bundle)
1431-
if err != nil {
1435+
if err := s.rmChannelEntry(tx, bundle); err != nil {
14321436
return err
14331437
}
1434-
err := s.rmBundle(tx, bundle)
1435-
if err != nil {
1438+
if err := s.rmBundle(tx, bundle); err != nil {
14361439
return err
14371440
}
14381441
}
14391442

1443+
// Remove any channels that start with the deprecated bundle
1444+
_, err = tx.Exec(fmt.Sprintf(`DELETE FROM channel WHERE head_operatorbundle_name="%s"`, name))
1445+
if err != nil {
1446+
return err
1447+
}
1448+
14401449
deprecatedValue, err := json.Marshal(registry.DeprecatedProperty{})
14411450
if err != nil {
14421451
return err

‎pkg/sqlite/load_test.go

+44-11
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ func newUnstructuredCSV(t *testing.T, name, replaces string) *unstructured.Unstr
224224
return &unstructured.Unstructured{Object: out}
225225
}
226226

227-
func newUnstructuredCSVwithSkips(t *testing.T, name, replaces string, skips ...string) *unstructured.Unstructured {
227+
func newUnstructuredCSVWithSkips(t *testing.T, name, replaces string, skips ...string) *unstructured.Unstructured {
228228
csv := &registry.ClusterServiceVersion{}
229229
csv.TypeMeta.Kind = "ClusterServiceVersion"
230230
csv.SetName(name)
@@ -271,7 +271,7 @@ func TestGetTailFromBundle(t *testing.T) {
271271
expected expected
272272
}{
273273
{
274-
description: "GetTailFromBundle/RemoveDefaultChannelForbidden",
274+
description: "ContainsDefaultChannel",
275275
fields: fields{
276276
bundles: []*registry.Bundle{
277277
newBundle(t, "csv-a", "pkg-0", []string{"alpha"}, newUnstructuredCSV(t, "csv-a", "csv-b")),
@@ -304,7 +304,7 @@ func TestGetTailFromBundle(t *testing.T) {
304304
},
305305
},
306306
{
307-
description: "GetTailFromBundle/RemovingNonDefaultChannel",
307+
description: "ContainsNoDefaultChannel",
308308
fields: fields{
309309
bundles: []*registry.Bundle{
310310
newBundle(t, "csv-a", "pkg-0", []string{"alpha"}, newUnstructuredCSV(t, "csv-a", "csv-b")),
@@ -329,23 +329,23 @@ func TestGetTailFromBundle(t *testing.T) {
329329
},
330330
},
331331
args: args{
332-
bundle: "csv-a",
332+
bundle: "csv-b",
333333
},
334334
expected: expected{
335335
err: nil,
336336
tail: []string{
337-
"csv-b",
338337
"csv-c",
339338
},
340339
},
341340
},
342341
{
343-
description: "GetTailFromBundle/HandlesSkips",
342+
description: "ContainsSkips",
344343
fields: fields{
345344
bundles: []*registry.Bundle{
346-
newBundle(t, "csv-a", "pkg-0", []string{"alpha"}, newUnstructuredCSVwithSkips(t, "csv-a", "csv-b", "csv-d", "csv-e", "csv-f")),
347-
newBundle(t, "csv-b", "pkg-0", []string{"alpha", "stable"}, newUnstructuredCSV(t, "csv-b", "csv-c")),
348-
newBundle(t, "csv-c", "pkg-0", []string{"alpha", "stable"}, newUnstructuredCSV(t, "csv-c", "")),
345+
newBundle(t, "csv-a", "pkg-0", []string{"alpha"}, newUnstructuredCSV(t, "csv-a", "csv-b")),
346+
newBundle(t, "csv-b", "pkg-0", []string{"alpha"}, newUnstructuredCSVWithSkips(t, "csv-b", "csv-c", "csv-d", "csv-e", "csv-f")),
347+
newBundle(t, "csv-c", "pkg-0", []string{"alpha", "stable"}, newUnstructuredCSV(t, "csv-c", "csv-d")),
348+
newBundle(t, "csv-d", "pkg-0", []string{"alpha", "stable"}, newUnstructuredCSV(t, "csv-d", "")),
349349
},
350350
pkgs: []registry.PackageManifest{
351351
{
@@ -365,19 +365,52 @@ func TestGetTailFromBundle(t *testing.T) {
365365
},
366366
},
367367
args: args{
368-
bundle: "csv-a",
368+
bundle: "csv-b",
369369
},
370370
expected: expected{
371371
err: nil,
372372
tail: []string{
373-
"csv-b",
374373
"csv-c",
375374
"csv-d",
376375
"csv-e",
377376
"csv-f",
378377
},
379378
},
380379
},
380+
{
381+
description: "ContainsDefaultChannelFromSkips",
382+
fields: fields{
383+
bundles: []*registry.Bundle{
384+
newBundle(t, "csv-a", "pkg-0", []string{"alpha"}, newUnstructuredCSV(t, "csv-a", "csv-b")),
385+
newBundle(t, "csv-b", "pkg-0", []string{"alpha"}, newUnstructuredCSVWithSkips(t, "csv-b", "csv-d", "csv-c")),
386+
newBundle(t, "csv-c", "pkg-0", []string{"alpha", "stable"}, newUnstructuredCSV(t, "csv-c", "csv-d")),
387+
newBundle(t, "csv-d", "pkg-0", []string{"alpha", "stable"}, newUnstructuredCSV(t, "csv-d", "")),
388+
},
389+
pkgs: []registry.PackageManifest{
390+
{
391+
PackageName: "pkg-0",
392+
Channels: []registry.PackageChannel{
393+
{
394+
Name: "alpha",
395+
CurrentCSVName: "csv-a",
396+
},
397+
{
398+
Name: "stable",
399+
CurrentCSVName: "csv-c",
400+
},
401+
},
402+
DefaultChannelName: "stable",
403+
},
404+
},
405+
},
406+
args: args{
407+
bundle: "csv-b",
408+
},
409+
expected: expected{
410+
err: registry.ErrRemovingDefaultChannelDuringDeprecation,
411+
tail: nil,
412+
},
413+
},
381414
}
382415

383416
for _, tt := range tests {

0 commit comments

Comments
 (0)
Please sign in to comment.