diff --git a/pkg/api/apis/clusterserviceversion/v1alpha1/types.go b/pkg/api/apis/clusterserviceversion/v1alpha1/types.go index 4aacd29aac..01787cc7b8 100644 --- a/pkg/api/apis/clusterserviceversion/v1alpha1/types.go +++ b/pkg/api/apis/clusterserviceversion/v1alpha1/types.go @@ -6,8 +6,8 @@ import ( "encoding/json" "sort" - "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis" "github.com/coreos/go-semver/semver" + "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -153,6 +153,7 @@ const ( CSVReasonRequirementsUnknown ConditionReason = "RequirementsUnknown" CSVReasonRequirementsNotMet ConditionReason = "RequirementsNotMet" CSVReasonRequirementsMet ConditionReason = "AllRequirementsMet" + CSVReasonOwnerConflict ConditionReason = "OwnerConflict" CSVReasonComponentFailed ConditionReason = "InstallComponentFailed" CSVReasonInvalidStrategy ConditionReason = "InvalidInstallStrategy" CSVReasonWaiting ConditionReason = "InstallWaiting" diff --git a/pkg/controller/operators/alm/operator.go b/pkg/controller/operators/alm/operator.go index d76aa414fb..8e2671ea1d 100644 --- a/pkg/controller/operators/alm/operator.go +++ b/pkg/controller/operators/alm/operator.go @@ -155,18 +155,23 @@ func (a *ALMOperator) transitionCSVState(csv *v1alpha1.ClusterServiceVersion) (s csv.SetPhase(v1alpha1.CSVPhasePending, v1alpha1.CSVReasonRequirementsUnknown, "requirements not yet checked") case v1alpha1.CSVPhasePending: met, statuses := a.requirementStatus(csv) + csv.SetRequirementStatus(statuses) if !met { log.Info("requirements were not met") csv.SetPhase(v1alpha1.CSVPhasePending, v1alpha1.CSVReasonRequirementsNotMet, "one or more requirements couldn't be found") - csv.SetRequirementStatus(statuses) syncError = ErrRequirementsNotMet return } + // check for CRD ownership conflicts + if syncError = a.crdOwnerConflicts(csv, a.csvsInNamespace(csv.GetNamespace())); syncError != nil { + csv.SetPhase(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonOwnerConflict, fmt.Sprintf("owner conflict: %s", syncError)) + return + } + log.Infof("scheduling ClusterServiceVersion for install: %s", csv.SelfLink) csv.SetPhase(v1alpha1.CSVPhaseInstallReady, v1alpha1.CSVReasonRequirementsMet, "all requirements found, attempting install") - csv.SetRequirementStatus(statuses) case v1alpha1.CSVPhaseInstallReady: installer, strategy, _ := a.parseStrategiesAndUpdateStatus(csv) if strategy == nil { @@ -174,13 +179,14 @@ func (a *ALMOperator) transitionCSVState(csv *v1alpha1.ClusterServiceVersion) (s return } - syncError = installer.Install(strategy) - if syncError == nil { - csv.SetPhase(v1alpha1.CSVPhaseInstalling, v1alpha1.CSVReasonInstallSuccessful, "waiting for install components to report healthy") - a.requeueCSV(csv) + if syncError = installer.Install(strategy); syncError != nil { + csv.SetPhase(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonComponentFailed, fmt.Sprintf("install strategy failed: %s", syncError)) return } - csv.SetPhase(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonComponentFailed, fmt.Sprintf("install strategy failed: %s", syncError)) + + csv.SetPhase(v1alpha1.CSVPhaseInstalling, v1alpha1.CSVReasonInstallSuccessful, "waiting for install components to report healthy") + a.requeueCSV(csv) + return case v1alpha1.CSVPhaseInstalling: installer, strategy, _ := a.parseStrategiesAndUpdateStatus(csv) if strategy == nil { @@ -366,6 +372,34 @@ func (a *ALMOperator) requirementStatus(csv *v1alpha1.ClusterServiceVersion) (me return } +func (a *ALMOperator) crdOwnerConflicts(in *v1alpha1.ClusterServiceVersion, csvsInNamespace []*v1alpha1.ClusterServiceVersion) error { + for _, crd := range in.Spec.CustomResourceDefinitions.Owned { + for _, csv := range csvsInNamespace { + if csv.OwnsCRD(crd.Name) { + // two csvs own the same CRD, only valid if there's a replacing chain between them + // TODO: this and the other replacement checking should just load the replacement chain DAG into memory + current := csv + for { + if in.Spec.Replaces == current.GetName() { + return nil + } + next := a.isBeingReplaced(current, csvsInNamespace) + if next != nil { + current = next + continue + } + if in.Name == csv.Name { + return nil + } + // couldn't find a chain between the two csvs + return fmt.Errorf("%s and %s both own %s, but there is no replacement chain linking them", in.Name, csv.Name, crd.Name) + } + } + } + } + return nil +} + // annotateNamespace is the method that gets called when we see a namespace event in the cluster func (a *ALMOperator) annotateNamespace(obj interface{}) (syncError error) { namespace, ok := obj.(*corev1.Namespace) diff --git a/pkg/controller/operators/alm/operator_test.go b/pkg/controller/operators/alm/operator_test.go index fb7d783ca9..371d9437de 100644 --- a/pkg/controller/operators/alm/operator_test.go +++ b/pkg/controller/operators/alm/operator_test.go @@ -296,12 +296,12 @@ func TestCSVStateTransitionsFromNone(t *testing.T) { func TestCSVStateTransitionsFromPending(t *testing.T) { type clusterState struct { - crdDescriptons []*v1alpha1.CRDDescription + csvs []*v1alpha1.ClusterServiceVersion } tests := []struct { in *v1alpha1.ClusterServiceVersion out *v1alpha1.ClusterServiceVersion - state clusterState + state *clusterState err error description string }{ @@ -414,6 +414,9 @@ func TestCSVStateTransitionsFromPending(t *testing.T) { Message: "all requirements found, attempting install", Reason: v1alpha1.CSVReasonRequirementsMet, }), + state: &clusterState{ + csvs: []*v1alpha1.ClusterServiceVersion{}, + }, description: "RequirementsMet/OwnedAndRequiredFound", }, { @@ -431,6 +434,9 @@ func TestCSVStateTransitionsFromPending(t *testing.T) { Message: "all requirements found, attempting install", Reason: v1alpha1.CSVReasonRequirementsMet, }), + state: &clusterState{ + csvs: []*v1alpha1.ClusterServiceVersion{}, + }, description: "RequirementsMet/OwnedFound", }, { @@ -448,27 +454,144 @@ func TestCSVStateTransitionsFromPending(t *testing.T) { Message: "all requirements found, attempting install", Reason: v1alpha1.CSVReasonRequirementsMet, }), + state: &clusterState{ + csvs: []*v1alpha1.ClusterServiceVersion{}, + }, description: "RequirementsMet/RequiredFound", }, + { + in: withStatus(withSpec(testCSV(""), + &v1alpha1.ClusterServiceVersionSpec{ + CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{ + Owned: makeCRDDescriptions("found1", "found2"), + Required: makeCRDDescriptions("found3", "found4"), + }, + }), + &v1alpha1.ClusterServiceVersionStatus{ + Phase: v1alpha1.CSVPhasePending, + }), + out: withStatus(testCSV(""), &v1alpha1.ClusterServiceVersionStatus{ + Phase: v1alpha1.CSVPhaseFailed, + Message: "owner conflict: test-csv and existing-owner both own found1, but there is no replacement chain linking them", + Reason: v1alpha1.CSVReasonOwnerConflict, + }), + state: &clusterState{ + csvs: []*v1alpha1.ClusterServiceVersion{withSpec(testCSV("existing-owner"), + &v1alpha1.ClusterServiceVersionSpec{ + CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{ + Owned: makeCRDDescriptions("found1"), + }, + })}, + }, + description: "RequirementsMet/OwnedAndRequiredFound/CRDAlreadyOwnedNoReplacementChain", + err: fmt.Errorf("test-csv and existing-owner both own found1, but there is no replacement chain linking them"), + }, + { + in: withStatus(withSpec(testCSV(""), + &v1alpha1.ClusterServiceVersionSpec{ + CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{ + Owned: makeCRDDescriptions("found1", "found2"), + Required: makeCRDDescriptions("found3", "found4"), + }, + Replaces: "existing-owner-2", + }), + &v1alpha1.ClusterServiceVersionStatus{ + Phase: v1alpha1.CSVPhasePending, + }), + out: withStatus(testCSV(""), &v1alpha1.ClusterServiceVersionStatus{ + Phase: v1alpha1.CSVPhaseInstallReady, + Message: "all requirements found, attempting install", + Reason: v1alpha1.CSVReasonRequirementsMet, + }), + state: &clusterState{ + csvs: []*v1alpha1.ClusterServiceVersion{ + withSpec(testCSV("existing-owner-1"), + &v1alpha1.ClusterServiceVersionSpec{ + CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{ + Owned: makeCRDDescriptions("found1"), + }, + }), + withSpec(testCSV("existing-owner-2"), + &v1alpha1.ClusterServiceVersionSpec{ + CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{ + Owned: makeCRDDescriptions("found1", "found2"), + }, + Replaces: "existing-owner-1", + }), + }, + }, + description: "RequirementsMet/OwnedAndRequiredFound/CRDOwnedInReplacementChain", + }, + { + in: withStatus(withSpec(testCSV(""), + &v1alpha1.ClusterServiceVersionSpec{ + CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{ + Owned: makeCRDDescriptions("found1", "found2"), + Required: makeCRDDescriptions("found3", "found4"), + }, + Replaces: "existing-owner-2", + }), + &v1alpha1.ClusterServiceVersionStatus{ + Phase: v1alpha1.CSVPhasePending, + }), + out: withStatus(testCSV(""), &v1alpha1.ClusterServiceVersionStatus{ + Phase: v1alpha1.CSVPhaseInstallReady, + Message: "all requirements found, attempting install", + Reason: v1alpha1.CSVReasonRequirementsMet, + }), + state: &clusterState{ + csvs: []*v1alpha1.ClusterServiceVersion{ + withSpec(testCSV("existing-owner-1"), + &v1alpha1.ClusterServiceVersionSpec{ + CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{ + Owned: makeCRDDescriptions("found1"), + }, + Replaces: "existing-owner-3", + }), + withSpec(testCSV("existing-owner-2"), + &v1alpha1.ClusterServiceVersionSpec{ + CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{ + Owned: makeCRDDescriptions("found1", "found2"), + }, + Replaces: "existing-owner-1", + }), + withSpec(testCSV("existing-owner-3"), + &v1alpha1.ClusterServiceVersionSpec{ + CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{ + Owned: makeCRDDescriptions("found1", "found2"), + }, + Replaces: "existing-owner-2", + }), + }, + }, + description: "RequirementsMet/OwnedAndRequiredFound/CRDOwnedInReplacementChainLoop", + }, } for _, tt := range tests { - ctrl := gomock.NewController(t) - mockOp := NewMockALMOperator(ctrl) - - mockCRDExistence(*mockOp.MockQueueOperator.MockClient, tt.in.Spec.CustomResourceDefinitions.Owned) - mockCRDExistence(*mockOp.MockQueueOperator.MockClient, tt.in.Spec.CustomResourceDefinitions.Required) - mockOp.MockOpClient.EXPECT().ListCustomResource(apis.GroupName, v1alpha1.GroupVersion, tt.in.GetNamespace(), v1alpha1.ClusterServiceVersionKind).Return(&operatorclient.CustomResourceList{}, nil) - // Test the transition t.Run(tt.description, func(t *testing.T) { + ctrl := gomock.NewController(t) + mockOp := NewMockALMOperator(ctrl) + + mockCRDExistence(*mockOp.MockQueueOperator.MockClient, tt.in.Spec.CustomResourceDefinitions.Owned) + mockCRDExistence(*mockOp.MockQueueOperator.MockClient, tt.in.Spec.CustomResourceDefinitions.Required) + + // mock for call to short-circuit when replacing + mockCSVsInNamespace(t, mockOp.MockQueueOperator.MockClient, tt.in.Namespace, nil, nil) + + // mock for pending, check that no other CSV owns the CRDs (unless being replaced) + if tt.state != nil { + mockCSVsInNamespace(t, mockOp.MockQueueOperator.MockClient, tt.in.Namespace, tt.state.csvs, nil) + } + err := mockOp.transitionCSVState(tt.in) require.EqualValues(t, tt.err, err) require.EqualValues(t, tt.out.Status.Phase, tt.in.Status.Phase) require.EqualValues(t, tt.out.Status.Message, tt.in.Status.Message) require.EqualValues(t, tt.out.Status.Reason, tt.in.Status.Reason) + ctrl.Finish() }) - ctrl.Finish() } }