Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(catalog): Add catalog weighting to dependency resolver #1682

Closed

Conversation

harishsurf
Copy link
Contributor

@harishsurf harishsurf commented Jul 27, 2020

Signed-off-by: Harish hgovinda@redhat.com

Description of the change:
This PR includes Priority field (on the catalog source) to sort catalog source based on priority which is later consumed by the dependency resolver.

  • Higher priority value is preferred over lower priority
  • The default priority for custom catsrc is 0.
  • Sorting precedence for catsrc is namespace > priority > name

Motivation for the change:

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Docs updated or added to /docs
  • Commit messages sensible and descriptive

@Bowenislandsong
Copy link
Member

This PR failed tests for 1 times with 2 individual failed tests and 4 skipped tests. A test is considered flaky if failed on multiple commits.

totaltestcount: 1
flaketestcount: 2
skippedtestcount: 4
flaketests:

  • classname: End-to-end
    name: 'Subscription creation in case of transferring providedAPIs'
    counts: 3
    details:
    • count: 3
      error: |4-

      /home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go:1379
      
      	Error Trace:	subscription_e2e_test.go:1517
      	            				runner.go:113
      	            				runner.go:64
      	            				it_node.go:26
      	            				spec.go:215
      	            				spec.go:138
      	            				spec_runner.go:200
      	            				spec_runner.go:170
      	            				spec_runner.go:66
      	            				suite.go:62
      	            				ginkgo_dsl.go:226
      	            				ginkgo_dsl.go:214
      	            				e2e_test.go:54
      	Error:      	Received unexpected error:
      	            	timed out waiting for the condition
      
      /home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/vendor/github.com/stretchr/testify/require/require.go:1005
      
    meandurationsec: 385.68525033333333
  • classname: End-to-end
    name: 'Garbage collection for dependent resources when a bundle with a new configmap
    is installed when the subscription is updated to a later CSV with a configmap
    with a new name should have removed the old configmap and put the new configmap
    in place
    '
    counts: 1
    details:
    • count: 1
      error: |4-

      /home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/gc_e2e_test.go:507
      could not get installplan at complete phase
      Unexpected error:
          <*errors.errorString | 0xc0001f82a0>: {
              s: "timed out waiting for the condition",
          }
          timed out waiting for the condition
      occurred
      /home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/gc_e2e_test.go:547
      
    meandurationsec: 312.100609
    skippedtests:
  • classname: End-to-end
    name: 'Subscriptions create required objects from Catalogs Given a Namespace
    when a CatalogSource is created with a bundle that contains prometheus objects
    creating a subscription using the CatalogSource should install the operator
    successfully
    '
    counts: 1
    details: []
    meandurationsec: 20.075543
  • classname: End-to-end
    name: 'Subscriptions create required objects from Catalogs Given a Namespace
    when a CatalogSource is created with a bundle that contains prometheus objects
    creating a subscription using the CatalogSource should have created the expected
    prometheus objects
    '
    counts: 1
    details: []
    meandurationsec: 15.079386
  • classname: End-to-end
    name: 'Catalog image update'
    counts: 1
    details: []
    meandurationsec: 0.02003
  • classname: End-to-end
    name: 'Subscription updates existing install plan'
    counts: 1
    details: []
    meandurationsec: 0.151669

@harishsurf harishsurf changed the title Add catalog weighting to dependency resolver [WIP] Add catalog weighting to dependency resolver Jul 27, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 27, 2020
@ecordell ecordell force-pushed the new-resolver branch 2 times, most recently from 46de607 to 64fc053 Compare July 27, 2020 16:13
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2020
@harishsurf harishsurf force-pushed the test-resolver branch 3 times, most recently from eb3d9f8 to 5a64b6a Compare July 28, 2020 17:14
pkg/controller/registry/resolver/cache.go Outdated Show resolved Hide resolved
pkg/controller/registry/resolver/cache.go Outdated Show resolved Hide resolved
pkg/controller/registry/resolver/cache.go Outdated Show resolved Hide resolved
@dinhxuanvu dinhxuanvu added the area/dependency Issues or PRs related to dependency changes label Jul 29, 2020
@Bowenislandsong
Copy link
Member

This PR failed tests for 1 times with 2 individual failed tests and 4 skipped tests. A test is considered flaky if failed on multiple commits.

totaltestcount: 1
flaketestcount: 2
skippedtestcount: 4
flaketests:

  • classname: End-to-end
    name: 'Subscription puppeting CatalogSource health status when missing target
    catalog should surface the missing catalog
    '
    counts: 3
    details:
    • count: 3
      error: |4-

      /home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go:588
      Unexpected error:
          <*errors.errorString | 0xc0012dfe70>: {
              s: "E2E bug detected: configmaps \"mock-ocs\" already exists",
          }
          E2E bug detected: configmaps "mock-ocs" already exists
      occurred
      /home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go:590
      
    meandurationsec: 0.03879866666666667
  • classname: End-to-end
    name: 'Subscription with dependencies from different catsrc with priorities'
    counts: 2
    details:
    • count: 2
      error: |4-

      /home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go:1369
      
      	Error Trace:	subscription_e2e_test.go:1521
      	            				runner.go:113
      	            				runner.go:64
      	            				it_node.go:26
      	            				spec.go:215
      	            				spec.go:138
      	            				spec_runner.go:200
      	            				spec_runner.go:170
      	            				spec_runner.go:66
      	            				suite.go:62
      	            				ginkgo_dsl.go:226
      	            				ginkgo_dsl.go:214
      	            				e2e_test.go:54
      	Error:      	Received unexpected error:
      	            	clusterserviceversions.operators.coreos.com "csv-main" not found
      
      /home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/vendor/github.com/stretchr/testify/require/require.go:1005
      
    meandurationsec: 26.818526
    skippedtests:
  • classname: End-to-end
    name: 'Subscriptions create required objects from Catalogs Given a Namespace
    when a CatalogSource is created with a bundle that contains prometheus objects
    creating a subscription using the CatalogSource should have created the expected
    prometheus objects
    '
    counts: 1
    details: []
    meandurationsec: 23.258269
  • classname: End-to-end
    name: 'Subscription updates existing install plan'
    counts: 1
    details: []
    meandurationsec: 0.071045
  • classname: End-to-end
    name: 'Subscriptions create required objects from Catalogs Given a Namespace
    when a CatalogSource is created with a bundle that contains prometheus objects
    creating a subscription using the CatalogSource should install the operator
    successfully
    '
    counts: 1
    details: []
    meandurationsec: 10.226736
  • classname: End-to-end
    name: 'Catalog image update'
    counts: 1
    details: []
    meandurationsec: 0.553019

Copy link
Member

@Bowenislandsong Bowenislandsong left a comment

Choose a reason for hiding this comment

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

2 nits, the rest lgtm.
This PR should be opened against master now since we are not merging onto new-resolver anymore. The amount of retest is going too wild.

pkg/controller/registry/resolver/cache.go Outdated Show resolved Hide resolved
pkg/controller/registry/resolver/cache.go Show resolved Hide resolved
@dinhxuanvu dinhxuanvu changed the title [WIP] Add catalog weighting to dependency resolver feat(catalog): Add catalog weighting to dependency resolver Jul 30, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 30, 2020
@dinhxuanvu
Copy link
Member

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 30, 2020
test/e2e/subscription_e2e_test.go Outdated Show resolved Hide resolved
test/e2e/subscription_e2e_test.go Outdated Show resolved Hide resolved
@dinhxuanvu dinhxuanvu changed the base branch from new-resolver to master July 30, 2020 15:42
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@dinhxuanvu
Copy link
Member

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 3, 2020
@benluddy
Copy link
Contributor

benluddy commented Aug 3, 2020

/hold cancel
/retest

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 3, 2020
@Bowenislandsong
Copy link
Member

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

18 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot
Copy link
Collaborator

@harishsurf: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/verify bcba3c2 link /test verify
ci/prow/images bcba3c2 link /test images
ci/prow/unit bcba3c2 link /test unit
ci/prow/e2e-aws-olm bcba3c2 link /test e2e-aws-olm
ci/prow/e2e-aws-console-olm bcba3c2 link /test e2e-aws-console-olm
ci/prow/e2e-gcp bcba3c2 link /test e2e-gcp
ci/prow/e2e-gcp-upgrade bcba3c2 link /test e2e-gcp-upgrade

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@dinhxuanvu
Copy link
Member

Replaced by #1705

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/dependency Issues or PRs related to dependency changes lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants