diff --git a/pkg/controller/install/deployment.go b/pkg/controller/install/deployment.go index fc419bacb28..c71014de17c 100644 --- a/pkg/controller/install/deployment.go +++ b/pkg/controller/install/deployment.go @@ -6,6 +6,8 @@ import ( log "github.com/sirupsen/logrus" appsv1 "k8s.io/api/apps/v1" rbac "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/util/diff" "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1" "github.com/operator-framework/operator-lifecycle-manager/pkg/api/wrappers" @@ -61,25 +63,7 @@ func NewStrategyDeploymentInstaller(strategyClient wrappers.InstallStrategyDeplo func (i *StrategyDeploymentInstaller) installDeployments(deps []StrategyDeploymentSpec) error { for _, d := range deps { - dep := &appsv1.Deployment{Spec: d.Spec} - dep.SetName(d.Name) - dep.SetNamespace(i.owner.GetNamespace()) - - // Merge annotations (to avoid losing info from pod template) - annotations := map[string]string{} - for k, v := range i.templateAnnotations { - annotations[k] = v - } - for k, v := range dep.Spec.Template.GetAnnotations() { - annotations[k] = v - } - dep.Spec.Template.SetAnnotations(annotations) - - ownerutil.AddNonBlockingOwner(dep, i.owner) - if err := ownerutil.AddOwnerLabels(dep, i.owner); err != nil { - return err - } - if _, err := i.strategyClient.CreateOrUpdateDeployment(dep); err != nil { + if _, err := i.strategyClient.CreateOrUpdateDeployment(i.deploymentForSpec(d.Name, d.Spec)); err != nil { return err } } @@ -87,6 +71,26 @@ func (i *StrategyDeploymentInstaller) installDeployments(deps []StrategyDeployme return nil } +func (i *StrategyDeploymentInstaller) deploymentForSpec(name string, spec appsv1.DeploymentSpec) *appsv1.Deployment { + dep := &appsv1.Deployment{Spec: spec} + dep.SetName(name) + dep.SetNamespace(i.owner.GetNamespace()) + + // Merge annotations (to avoid losing info from pod template) + annotations := map[string]string{} + for k, v := range i.templateAnnotations { + annotations[k] = v + } + for k, v := range dep.Spec.Template.GetAnnotations() { + annotations[k] = v + } + dep.Spec.Template.SetAnnotations(annotations) + + ownerutil.AddNonBlockingOwner(dep, i.owner) + ownerutil.AddOwnerLabelsForKind(dep, i.owner, v1alpha1.ClusterServiceVersionKind) + return dep +} + func (i *StrategyDeploymentInstaller) cleanupPrevious(current *StrategyDetailsDeployment, previous *StrategyDetailsDeployment) error { previousDeploymentsMap := map[string]struct{}{} for _, d := range previous.DeploymentSpecs { @@ -179,10 +183,44 @@ func (i *StrategyDeploymentInstaller) checkForDeployments(deploymentSpecs []Stra return StrategyError{Reason: StrategyErrReasonAnnotationsMissing, Message: fmt.Sprintf("annotations on deployment don't match. couldn't find %s: %s", key, value)} } } + + // check equality + calculated := i.deploymentForSpec(spec.Name, spec.Spec) + if !i.equalDeployments(&calculated.Spec, &dep.Spec) { + return StrategyError{Reason: StrategyErrDeploymentUpdated, Message: fmt.Sprintf("deployment changed, rolling update with patch: %s\n%#v\n%#v", diff.ObjectDiff(dep.Spec.Template.Spec, calculated.Spec.Template.Spec), calculated.Spec.Template.Spec, dep.Spec.Template.Spec)} + } } return nil } +func (i *StrategyDeploymentInstaller) equalDeployments(calculated, onCluster *appsv1.DeploymentSpec) bool { + // ignore template annotations, OLM injects these elsewhere + calculated.Template.Annotations = nil + + // DeepDerivative doesn't treat `0` ints as unset. Stripping them here means we miss changes to these values, + // but we don't end up getting bitten by the defaulter for deployments. + for i, c := range onCluster.Template.Spec.Containers { + o := calculated.Template.Spec.Containers[i] + if o.ReadinessProbe != nil { + o.ReadinessProbe.InitialDelaySeconds = c.ReadinessProbe.InitialDelaySeconds + o.ReadinessProbe.TimeoutSeconds = c.ReadinessProbe.TimeoutSeconds + o.ReadinessProbe.PeriodSeconds = c.ReadinessProbe.PeriodSeconds + o.ReadinessProbe.SuccessThreshold = c.ReadinessProbe.SuccessThreshold + o.ReadinessProbe.FailureThreshold = c.ReadinessProbe.FailureThreshold + } + if o.LivenessProbe != nil { + o.LivenessProbe.InitialDelaySeconds = c.LivenessProbe.InitialDelaySeconds + o.LivenessProbe.TimeoutSeconds = c.LivenessProbe.TimeoutSeconds + o.LivenessProbe.PeriodSeconds = c.LivenessProbe.PeriodSeconds + o.LivenessProbe.SuccessThreshold = c.LivenessProbe.SuccessThreshold + o.LivenessProbe.FailureThreshold = c.LivenessProbe.FailureThreshold + } + } + + // DeepDerivative ensures that, for any non-nil, non-empty value in A, the corresponding value is set in B + return equality.Semantic.DeepDerivative(calculated, onCluster) +} + // Clean up orphaned deployments after reinstalling deployments process func (i *StrategyDeploymentInstaller) cleanupOrphanedDeployments(deploymentSpecs []StrategyDeploymentSpec) error { // Map of deployments diff --git a/pkg/controller/install/errors.go b/pkg/controller/install/errors.go index 47a0ae4411e..4b06c555c9c 100644 --- a/pkg/controller/install/errors.go +++ b/pkg/controller/install/errors.go @@ -9,12 +9,15 @@ const ( StrategyErrReasonInvalidStrategy = "InvalidStrategy" StrategyErrReasonTimeout = "Timeout" StrategyErrReasonUnknown = "Unknown" + StrategyErrBadPatch = "PatchUnsuccessful" + StrategyErrDeploymentUpdated = "DeploymentUpdated" ) // unrecoverableErrors are the set of errors that mean we can't recover an install strategy var unrecoverableErrors = map[string]struct{}{ StrategyErrReasonInvalidStrategy: {}, StrategyErrReasonTimeout: {}, + StrategyErrBadPatch: {}, } // StrategyError is used to represent error types for install strategies diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index 48142093ca3..2f523ef3208 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -1256,6 +1256,10 @@ func (a *Operator) updateInstallStatus(csv *v1alpha1.ClusterServiceVersion, inst strategyInstalled, strategyErr := installer.CheckInstalled(strategy) now := a.now() + if strategyErr != nil { + a.logger.WithError(strategyErr).Debug("operator not installed") + } + if strategyInstalled && apiServicesInstalled { // if there's no error, we're successfully running csv.SetPhaseWithEventIfChanged(v1alpha1.CSVPhaseSucceeded, v1alpha1.CSVReasonInstallSuccessful, "install strategy completed with no errors", now, a.recorder) diff --git a/pkg/lib/ownerutil/util.go b/pkg/lib/ownerutil/util.go index 0f8ee561020..0348be004d6 100644 --- a/pkg/lib/ownerutil/util.go +++ b/pkg/lib/ownerutil/util.go @@ -195,21 +195,26 @@ func OwnerLabel(owner Owner, kind string) map[string]string { } } -// AddOwnerLabels adds ownerref-like labels to an object +// AddOwnerLabels adds ownerref-like labels to an object by inferring the owner kind func AddOwnerLabels(object metav1.Object, owner Owner) error { err := InferGroupVersionKind(owner) if err != nil { return err } + AddOwnerLabelsForKind(object, owner, owner.GetObjectKind().GroupVersionKind().Kind) + return nil +} + +// AddOwnerLabels adds ownerref-like labels to an object, with no inference +func AddOwnerLabelsForKind(object metav1.Object, owner Owner, kind string) { labels := object.GetLabels() if labels == nil { labels = map[string]string{} } - for key, val := range OwnerLabel(owner, owner.GetObjectKind().GroupVersionKind().Kind) { + for key, val := range OwnerLabel(owner, kind) { labels[key] = val } object.SetLabels(labels) - return nil } // IsOwnedByKindLabel returns whether or not a label exists on the object pointing to an owner of a particular kind diff --git a/scripts/build_local.sh b/scripts/build_local.sh index f4f0b7984bb..62bfe4a67b4 100755 --- a/scripts/build_local.sh +++ b/scripts/build_local.sh @@ -13,7 +13,7 @@ fi docker build -f local.Dockerfile -t quay.io/operator-framework/olm:local -t quay.io/operator-framework/olm-e2e:local ./bin -if [ -x "$(command -v kind)" ] && [ "kubectl config current-context" -eq "kind" ]; then +if [ -x "$(command -v kind)" ] && [ "$(kubectl config current-context)" = "kind" ]; then kind load docker-image quay.io/operator-framework/olm:local kind load docker-image quay.io/operator-framework/olm-e2e:local fi diff --git a/test/e2e/csv_e2e_test.go b/test/e2e/csv_e2e_test.go index b0113647dea..58cc10632d0 100644 --- a/test/e2e/csv_e2e_test.go +++ b/test/e2e/csv_e2e_test.go @@ -2393,6 +2393,146 @@ func TestUpdateCSVMultipleIntermediates(t *testing.T) { require.NoError(t, err) } +func TestUpdateCSVInPlace(t *testing.T) { + defer cleaner.NotifyTestComplete(t, true) + + c := newKubeClient(t) + crc := newCRClient(t) + + // Create dependency first (CRD) + crdPlural := genName("ins") + crdName := crdPlural + ".cluster.com" + cleanupCRD, err := createCRD(c, apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: crdName, + }, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Group: "cluster.com", + Version: "v1alpha1", + Names: apiextensions.CustomResourceDefinitionNames{ + Plural: crdPlural, + Singular: crdPlural, + Kind: crdPlural, + ListKind: "list" + crdPlural, + }, + Scope: "Namespaced", + }, + }) + + // Create "current" CSV + nginxName := genName("nginx-") + strategy := install.StrategyDetailsDeployment{ + DeploymentSpecs: []install.StrategyDeploymentSpec{ + { + Name: genName("dep-"), + Spec: newNginxDeployment(nginxName), + }, + }, + } + strategyRaw, err := json.Marshal(strategy) + require.NoError(t, err) + + require.NoError(t, err) + defer cleanupCRD() + csv := v1alpha1.ClusterServiceVersion{ + TypeMeta: metav1.TypeMeta{ + Kind: v1alpha1.ClusterServiceVersionKind, + APIVersion: v1alpha1.ClusterServiceVersionAPIVersion, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: genName("csv"), + }, + Spec: v1alpha1.ClusterServiceVersionSpec{ + MinKubeVersion: "0.0.0", + InstallModes: []v1alpha1.InstallMode{ + { + Type: v1alpha1.InstallModeTypeOwnNamespace, + Supported: true, + }, + { + Type: v1alpha1.InstallModeTypeSingleNamespace, + Supported: true, + }, + { + Type: v1alpha1.InstallModeTypeMultiNamespace, + Supported: true, + }, + { + Type: v1alpha1.InstallModeTypeAllNamespaces, + Supported: true, + }, + }, + InstallStrategy: v1alpha1.NamedInstallStrategy{ + StrategyName: install.InstallStrategyNameDeployment, + StrategySpecRaw: strategyRaw, + }, + CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{ + Owned: []v1alpha1.CRDDescription{ + { + Name: crdName, + Version: "v1alpha1", + Kind: crdPlural, + DisplayName: crdName, + Description: "In the cluster", + }, + }, + }, + }, + } + + // Don't need to cleanup this CSV, it will be deleted by the upgrade process + _, err = createCSV(t, c, crc, csv, testNamespace, false, false) + require.NoError(t, err) + + // Wait for current CSV to succeed + fetchedCSV, err := fetchCSV(t, crc, csv.Name, testNamespace, csvSucceededChecker) + require.NoError(t, err) + + // Should have created deployment + dep, err := c.GetDeployment(testNamespace, strategy.DeploymentSpecs[0].Name) + require.NoError(t, err) + require.NotNil(t, dep) + + // Create "updated" CSV + strategyNew := install.StrategyDetailsDeployment{ + DeploymentSpecs: []install.StrategyDeploymentSpec{ + { + // Same name + Name: strategy.DeploymentSpecs[0].Name, + // Different spec + Spec: newNginxDeployment(nginxName), + }, + }, + } + strategyNewRaw, err := json.Marshal(strategyNew) + require.NoError(t, err) + + fetchedCSV.Spec.InstallStrategy.StrategySpecRaw = strategyNewRaw + + // Update CSV directly + _, err = crc.OperatorsV1alpha1().ClusterServiceVersions(testNamespace).Update(fetchedCSV) + require.NoError(t, err) + + // Wait for updated CSV to succeed + updatedCSV, err := fetchCSV(t, crc, csv.Name, testNamespace, csvSucceededChecker) + require.NoError(t, err) + + // Should have updated existing deployment + depUpdated, err := c.GetDeployment(testNamespace, strategyNew.DeploymentSpecs[0].Name) + require.NoError(t, err) + require.NotNil(t, depUpdated) + require.Equal(t, depUpdated.Spec.Template.Spec.Containers[0].Name, strategyNew.DeploymentSpecs[0].Spec.Template.Spec.Containers[0].Name) + + // Should eventually GC the CSV + err = waitForCSVToDelete(t, crc, csv.Name) + require.NoError(t, err) + + // Fetch cluster service version again to check for unnecessary control loops + sameCSV, err := fetchCSV(t, crc, csv.Name, testNamespace, csvSucceededChecker) + require.NoError(t, err) + compareResources(t, updatedCSV, sameCSV) +} + func TestUpdateCSVMultipleVersionCRD(t *testing.T) { defer cleaner.NotifyTestComplete(t, true)