Bug 1723818: CSV name change should not cause upgrade to fail #925

tkashem · 2019-06-26T01:40:50Z

If a head CSV has a different name than the CSV it replaces, upgrade
fails because olm fails to transfer ownership of the APIService from
the previous CSV to the head CSV.

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1723818

njhale

🎉 I think you're on the right track! A few thoughts on what's here:

Be careful of the additional adoptability checks during APIService install and installation checking. In the future, it may be better to consolidate these checks (maybe in an implementation of Operator validation library project proposal #924 extended to include APIServices).
I think we also want to make the latest version of the OLM CSV in ./manifests replaces the previous CSV.
@ecordell and I were talking about making OLM detect and apply CSV deployment spec changes at runtime, which may circumvent the issue packageserver is encountering that spurred this fix PR.

njhale · 2019-06-26T20:55:56Z

pkg/controller/operators/olm/operator.go

+				continue
+			}
+
+			if currentOwnerCSV.Status.Phase == v1alpha1.CSVPhaseReplacing ||


I think this will allow any CSV to adopt an APIService from a CSV being replaced or deleted. This is not necessarily what we want. For example: APIService-A is owned by CSV-A and is being replaced by CSV-B, which also provides APIService-A. Concurrently, there exists a CSV-C that provides APIService-A as well. In this scenario, OLM would falsely determine that APIService-A is adoptable by CSV-B - I may be missing something though.

CSV-C either currently owns APIService-A or is trying to own it. If CSV-C currently owns it and is not being replaced or being deleted then the function will not allow CSV-B to take ownership. In the latter case where CSV-C is also trying to own it then it's a race to the ownership.

njhale · 2019-06-26T21:03:58Z

pkg/controller/operators/olm/operator.go

+		}
+
+		// Is it owned by this csv or the one it replaces.
+		if ownerutil.AdoptableLabels(apiService.GetLabels(), true, owners...) {


If you check the owner for existence before this, and add it to the owners slice if it's not found, then you should be able to drop the rest of this function.

tkashem · 2019-06-27T21:02:22Z

/retest

tkashem · 2019-06-28T04:30:05Z

/retest

If a head CSV has a different name than the CSV it replaces, upgrade fails because olm fails to transfer ownership of the APIService from the previous CSV to the head CSV. BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1723818

openshift-ci-robot · 2019-06-28T12:23:32Z

@tkashem: This pull request references a valid Bugzilla bug. The bug has been moved to the POST state.

In response to this:

Bug 1723818: CSV name change should not cause upgrade to fail

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.

ecordell · 2019-06-28T12:29:47Z

/retest

tkashem · 2019-06-28T14:20:27Z

/retest

ecordell

/lgtm

I left some comments that we should follow up on, but this looks good in general. I believe this will fix the linked bug and we won't have to merge the PR to remove the ClusterOperator for packageserver.

ecordell · 2019-06-28T12:26:53Z

pkg/controller/operators/olm/apiservices.go

+		return
+	}
+
+	if err := ownerutil.InferGroupVersionKind(target); err != nil {


We know the type coming in and don't need the rest of the meta (just the kind string) - I would just import the ClusterServiceVersionKind string from the api package and compare

if ownerKind != v1alpha1.ClusterServiceVersionKind { ... }

ecordell · 2019-06-28T17:15:43Z

pkg/controller/operators/olm/apiservices.go

+		owners = append(owners, replacing)
+	}
+	if currentOwnerCSV != nil && (
+		currentOwnerCSV.Status.Phase == v1alpha1.CSVPhaseReplacing || currentOwnerCSV.Status.Phase == v1alpha1.CSVPhaseDeleting) {


I believe there's a set of phases that include these that you can check for membership

openshift-ci-robot · 2019-06-28T17:17:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ecordell, tkashem

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ecordell]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

smarterclayton · 2019-06-29T05:22:09Z

Looks like it’s still failing:

Jun 29 02:57:43.223: INFO: cluster upgrade is Progressing: Unable to apply 4.2.0-0.ci-2019-06-28-191017: the cluster operator operator-lifecycle-manager-packageserver has not yet successfully rolled out

https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/logs/release-openshift-origin-installer-e2e-aws-upgrade-4.1-to-4.2/131

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 Jun 26, 2019

openshift-ci-robot requested review from alecmerdler and ecordell June 26, 2019 01:41

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 26, 2019

tkashem force-pushed the bz-1723818 branch 2 times, most recently from 6f0d5b6 to 22ce783 Compare June 26, 2019 18:02

njhale reviewed Jun 26, 2019

View reviewed changes

tkashem force-pushed the bz-1723818 branch from 22ce783 to 1d53fcc Compare June 27, 2019 17:12

tkashem changed the title ~~[WIP] Upgrade should not fail if the csv name changes~~ (bug) CSV name change should not cause upgrade to fail Jun 27, 2019

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 Jun 27, 2019

tkashem force-pushed the bz-1723818 branch from 1d53fcc to fda6c09 Compare June 27, 2019 23:07

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 27, 2019

(bug) CSV name change causes upgrade to fail

d3c2b32

If a head CSV has a different name than the CSV it replaces, upgrade fails because olm fails to transfer ownership of the APIService from the previous CSV to the head CSV. BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1723818

tkashem force-pushed the bz-1723818 branch from fda6c09 to d3c2b32 Compare June 28, 2019 05:01

ecordell changed the title ~~(bug) CSV name change should not cause upgrade to fail~~ Bug 1723818: CSV name change should not cause upgrade to fail Jun 28, 2019

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jun 28, 2019

ecordell approved these changes Jun 28, 2019

View reviewed changes

openshift-ci-robot assigned ecordell Jun 28, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 28, 2019

openshift-merge-robot merged commit 1b20b3e into operator-framework:master Jun 28, 2019

ecordell mentioned this pull request Jun 28, 2019

Disable clusteroperator for package server #932

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1723818: CSV name change should not cause upgrade to fail #925

Bug 1723818: CSV name change should not cause upgrade to fail #925

tkashem commented Jun 26, 2019 •

edited

Loading

njhale left a comment

njhale Jun 26, 2019

tkashem Jun 27, 2019

njhale Jun 26, 2019 •

edited

Loading

tkashem commented Jun 27, 2019

tkashem commented Jun 28, 2019

openshift-ci-robot commented Jun 28, 2019

ecordell commented Jun 28, 2019

tkashem commented Jun 28, 2019

ecordell left a comment

ecordell Jun 28, 2019

ecordell Jun 28, 2019

openshift-ci-robot commented Jun 28, 2019

smarterclayton commented Jun 29, 2019

Bug 1723818: CSV name change should not cause upgrade to fail #925

Bug 1723818: CSV name change should not cause upgrade to fail #925

Conversation

tkashem commented Jun 26, 2019 • edited Loading

njhale left a comment

Choose a reason for hiding this comment

njhale Jun 26, 2019

Choose a reason for hiding this comment

tkashem Jun 27, 2019

Choose a reason for hiding this comment

njhale Jun 26, 2019 • edited Loading

Choose a reason for hiding this comment

tkashem commented Jun 27, 2019

tkashem commented Jun 28, 2019

openshift-ci-robot commented Jun 28, 2019

ecordell commented Jun 28, 2019

tkashem commented Jun 28, 2019

ecordell left a comment

Choose a reason for hiding this comment

ecordell Jun 28, 2019

Choose a reason for hiding this comment

ecordell Jun 28, 2019

Choose a reason for hiding this comment

openshift-ci-robot commented Jun 28, 2019

smarterclayton commented Jun 29, 2019

tkashem commented Jun 26, 2019 •

edited

Loading

njhale Jun 26, 2019 •

edited

Loading