From 07ce935a1355433c6f2b233da1caaf4576ab6dbc Mon Sep 17 00:00:00 2001 From: santiago-ventura <160760719+santiago-ventura@users.noreply.github.com> Date: Fri, 10 May 2024 12:58:16 +0200 Subject: [PATCH] Improve recreation of instances (#48) * add linting support * fix linting issue * add documentation for annotations * Implement exponential back-off for recreation * add generated files * use suite_test to support several integrations tests * Update controll tool version and update CRDs * Add the tests for Service instance controller * Update internal/controllers/suite_test.go Co-authored-by: uwefreidank <49634966+uwefreidank@users.noreply.github.com> * Adopted the suggession for a test * Add generated file api/v1alpha1/zz_generated.deepcopy.go * reverting back the changes to original code for create instance fails test * Make infinite retries as the default behavior * Added test for infinite retry * Adding comment for use of the reconcileTimeout * Update documentation of the annotations --------- Co-authored-by: RalfHammer <119853077+RalfHammer@users.noreply.github.com> Co-authored-by: shilparamasamyreddy <164521358+shilparamasamyreddy@users.noreply.github.com> Co-authored-by: uwefreidank <49634966+uwefreidank@users.noreply.github.com> --- .vscode/launch.json | 1 + Makefile | 25 +- api/v1alpha1/constants.go | 7 + api/v1alpha1/serviceinstance_types.go | 10 + api/v1alpha1/zz_generated.deepcopy.go | 1 - .../bases/cf.cs.sap.com_clusterspaces.yaml | 55 +-- .../bases/cf.cs.sap.com_servicebindings.yaml | 69 ++-- .../bases/cf.cs.sap.com_serviceinstances.yaml | 89 +++-- config/crd/bases/cf.cs.sap.com_spaces.yaml | 55 +-- config/rbac/role.yaml | 1 - config/webhook/manifests.yaml | 2 - crds/cf.cs.sap.com_clusterspaces.yaml | 55 +-- crds/cf.cs.sap.com_servicebindings.yaml | 69 ++-- crds/cf.cs.sap.com_serviceinstances.yaml | 89 +++-- crds/cf.cs.sap.com_spaces.yaml | 55 +-- internal/cf/instance.go | 1 + .../controllers/servicebinding_controller.go | 6 +- .../controllers/serviceinstance_controller.go | 120 +++++- ...iceinstance_controller_integration_test.go | 345 ++++++++++++++++++ .../space_controller_integration_test.go | 228 ++---------- internal/controllers/suite_test.go | 342 +++++++++++++++++ website/content/en/docs/usage/_index.md | 1 - .../content/en/docs/usage/serviceinstance.md | 86 ++++- 23 files changed, 1267 insertions(+), 445 deletions(-) create mode 100644 internal/controllers/serviceinstance_controller_integration_test.go create mode 100644 internal/controllers/suite_test.go diff --git a/.vscode/launch.json b/.vscode/launch.json index e522454..1b8db53 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -27,6 +27,7 @@ "console": "internalConsole", "env": { "KUBEBUILDER_ASSETS": "${workspaceFolder}/bin/k8s/current", + "TEST_TIMEOUT": "1200", }, "args": [], "showLog": true diff --git a/Makefile b/Makefile index 9a62deb..b7e5620 100644 --- a/Makefile +++ b/Makefile @@ -6,9 +6,9 @@ ENVTEST_K8S_VERSION = 1.25.0 # Get the currently used golang install path (in GOPATH/bin, unless GOBIN is set) ifeq (,$(shell go env GOBIN)) -GOBIN=$(shell go env GOPATH)/bin + GOBIN=$(shell go env GOPATH)/bin else -GOBIN=$(shell go env GOBIN) + GOBIN=$(shell go env GOBIN) endif # Setting SHELL to bash allows bash commands to be executed by recipes. @@ -57,11 +57,16 @@ fmt: ## Run go fmt against code. vet: ## Run go vet against code. go vet ./... +# Run the linter against the code +.PHONY: lint +lint: golangci-lint ## Run linter against the code + $(GOLINT) run ./... + .PHONY: test test: manifests generate fmt vet envtest ## Run tests. KUBEBUILDER_ASSETS="$(LOCALBIN)/k8s/current" go test ./... -coverprofile cover.out -.PHONY: test-fast ## Run tests without build. +.PHONY: manifests test-fast ## Run tests without build. test-fast: envtest ## Run tests. KUBEBUILDER_ASSETS="$(LOCALBIN)/k8s/current" go test ./... -coverprofile cover.out -ginkgo.v @@ -133,10 +138,15 @@ LOCALBIN ?= $(shell pwd)/bin $(LOCALBIN): mkdir -p $(LOCALBIN) +.PHONY: clean +clean: + rm -rf $(LOCALBIN) + ## Tool Binaries KUSTOMIZE ?= $(LOCALBIN)/kustomize CONTROLLER_GEN ?= $(LOCALBIN)/controller-gen COUNTERFEITER_GEN ?= $(LOCALBIN)/counterfeiter-gen +GOLINT ?= $(LOCALBIN)/golangci-lint ENVTEST ?= $(LOCALBIN)/setup-envtest CLIENT_GEN ?= $(shell pwd)/bin/client-gen INFORMER_GEN ?= $(shell pwd)/bin/informer-gen @@ -144,9 +154,10 @@ LISTER_GEN ?= $(shell pwd)/bin/lister-gen ## Tool Versions KUSTOMIZE_VERSION ?= v3.8.7 -CONTROLLER_TOOLS_VERSION ?= v0.9.2 +CONTROLLER_TOOLS_VERSION ?= v0.14.0 CODE_GENERATOR_VERSION ?= v0.23.4 COUNTERFEITER_VERSION ?= v6.8.1 +GOLINT_VERSION ?= v1.57.1 KUSTOMIZE_INSTALL_SCRIPT ?= "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh" .PHONY: kustomize @@ -164,6 +175,12 @@ counterfeiter-gen: $(COUNTERFEITER_GEN) ## Download controller-gen locally if ne $(COUNTERFEITER_GEN): $(LOCALBIN) test -s $(COUNTERFEITER_GEN) || GOBIN=$(LOCALBIN) go install github.com/maxbrunsfeld/counterfeiter/v6@$(COUNTERFEITER_VERSION) +# find or download golangci-lint +.PHONY: golangci-lint +golangci-lint: $(GOLINT) +$(GOLINT): $(LOCALBIN) + test -s $(GOLINT) || GOBIN=$(LOCALBIN) go install github.com/golangci/golangci-lint/cmd/golangci-lint@$(GOLINT_VERSION) + .PHONY: envtest envtest: $(ENVTEST) ## Download envtest-setup locally if necessary. $(ENVTEST): $(LOCALBIN) diff --git a/api/v1alpha1/constants.go b/api/v1alpha1/constants.go index 166d643..5e4d199 100644 --- a/api/v1alpha1/constants.go +++ b/api/v1alpha1/constants.go @@ -10,4 +10,11 @@ const ( LabelKeyClusterSpace = "service-operator.cf.cs.sap.com/cluster-space" LabelKeyServiceInstance = "service-operator.cf.cs.sap.com/service-instance" LabelKeyServiceBinding = "service-operator.cf.cs.sap.com/service-binding" + + // annotation on custom resources + AnnotationRecreate = "service-operator.cf.cs.sap.com/recreate-on-creation-failure" + // annotation max number of retries for a failed operation on a service instance + AnnotationMaxRetries = "service-operator.cf.cs.sap.com/max-retries" + // annotation to hold the reconciliation timeout value + AnnotationReconcileTimeout = "service-operator.cf.cs.sap.com/timeout-on-reconcile" ) diff --git a/api/v1alpha1/serviceinstance_types.go b/api/v1alpha1/serviceinstance_types.go index 2e4fd86..050400b 100644 --- a/api/v1alpha1/serviceinstance_types.go +++ b/api/v1alpha1/serviceinstance_types.go @@ -120,6 +120,16 @@ type ServiceInstanceStatus struct { // +optional ServiceInstanceDigest string `json:"serviceInstanceDigest,omitempty"` + // Counts the number of retries that have been attempted for the reconciliation of this service instance. + // This counter can be used to fail the instance if too many retries occur. + // +optional + RetryCounter int `json:"retryCounter,omitempty"` + + // This is the maximum number of retries that are allowed for the reconciliation of this service instance. + // If the retry counter exceeds this value, the service instance will be marked as failed. + // +optional + MaxRetries int `json:"maxRetries,omitempty"` + // List of status conditions to indicate the status of a ServiceInstance. // Known condition types are `Ready`. // +optional diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index ead09f9..c0d47b7 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -1,5 +1,4 @@ //go:build !ignore_autogenerated -// +build !ignore_autogenerated /* SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and cf-service-operator contributors diff --git a/config/crd/bases/cf.cs.sap.com_clusterspaces.yaml b/config/crd/bases/cf.cs.sap.com_clusterspaces.yaml index 3f6cc6a..31aef0b 100644 --- a/config/crd/bases/cf.cs.sap.com_clusterspaces.yaml +++ b/config/crd/bases/cf.cs.sap.com_clusterspaces.yaml @@ -3,8 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.9.2 - creationTimestamp: null + controller-gen.kubebuilder.io/version: v0.14.0 name: clusterspaces.cf.cs.sap.com spec: group: cf.cs.sap.com @@ -28,14 +27,19 @@ spec: description: ClusterSpace is the Schema for the clusterspaces API properties: apiVersion: - description: 'APIVersion defines the versioned schema of this representation - of an object. Servers should convert recognized schemas to the latest - internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources type: string kind: - description: 'Kind is a string value representing the REST resource this - object represents. Servers may infer this from the endpoint the client - submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds type: string metadata: type: object @@ -48,18 +52,21 @@ spec: minLength: 1 type: string guid: - description: Space GUID. Must not be specified if Name or OrganizationName - is present. + description: |- + Space GUID. + Must not be specified if Name or OrganizationName is present. minLength: 1 type: string name: - description: Space name. Must not be specified if Guid is present; - defauls to metadata.name otherwise. + description: |- + Space name. + Must not be specified if Guid is present; defauls to metadata.name otherwise. minLength: 1 type: string organizationName: - description: Organization name. Must not be specified if Guid is present; - required otherwise. + description: |- + Organization name. + Must not be specified if Guid is present; required otherwise. minLength: 1 type: string required: @@ -71,24 +78,28 @@ spec: description: SpaceStatus defines the observed state of Space. properties: conditions: - description: List of status conditions to indicate the status of a - Space. Known condition types are `Ready`. + description: |- + List of status conditions to indicate the status of a Space. + Known condition types are `Ready`. items: description: SpaceCondition contains condition information for a Space. properties: lastTransitionTime: - description: LastTransitionTime is the timestamp corresponding - to the last status change of this condition. + description: |- + LastTransitionTime is the timestamp corresponding to the last status + change of this condition. format: date-time type: string message: - description: Message is a human readable description of the - details of the last transition, complementing reason. + description: |- + Message is a human readable description of the details of the last + transition, complementing reason. type: string reason: - description: Reason is a brief machine readable explanation - for the condition's last transition. + description: |- + Reason is a brief machine readable explanation for the condition's last + transition. type: string status: description: Status of the condition, one of ('True', 'False', diff --git a/config/crd/bases/cf.cs.sap.com_servicebindings.yaml b/config/crd/bases/cf.cs.sap.com_servicebindings.yaml index 8cbd942..529f519 100644 --- a/config/crd/bases/cf.cs.sap.com_servicebindings.yaml +++ b/config/crd/bases/cf.cs.sap.com_servicebindings.yaml @@ -3,8 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.9.2 - creationTimestamp: null + controller-gen.kubebuilder.io/version: v0.14.0 name: servicebindings.cf.cs.sap.com spec: group: cf.cs.sap.com @@ -28,14 +27,19 @@ spec: description: ServiceBinding is the Schema for the servicebindings API properties: apiVersion: - description: 'APIVersion defines the versioned schema of this representation - of an object. Servers should convert recognized schemas to the latest - internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources type: string kind: - description: 'Kind is a string value representing the REST resource this - object represents. Servers may infer this from the endpoint the client - submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds type: string metadata: type: object @@ -48,13 +52,14 @@ spec: minLength: 1 type: string parameters: - description: Binding parameters. Do not provide any sensitve data - here; instead use ParametersFrom for such data. + description: |- + Binding parameters. + Do not provide any sensitve data here; instead use ParametersFrom for such data. x-kubernetes-preserve-unknown-fields: true parametersFrom: - description: References to secrets containing binding parameters. - Top level keys must occur only once across Parameters and the secrest - listed here. + description: |- + References to secrets containing binding parameters. + Top level keys must occur only once across Parameters and the secrest listed here. items: description: ParametersFromSource represents the source of a set of Parameters @@ -77,21 +82,21 @@ spec: type: object type: array secretKey: - description: Secret key (referring to SecretName) where the binding - credentials will be stored. If unspecified, the top level keys of - the binding credentials will become the secret keys. + description: |- + Secret key (referring to SecretName) where the binding credentials will be stored. + If unspecified, the top level keys of the binding credentials will become the secret keys. minLength: 1 type: string secretName: - description: Secret name where the binding credentials shall be stored - (in the same namespace where the binding exists). If unspecified, - metadata.name will be used. + description: |- + Secret name where the binding credentials shall be stored (in the same namespace where the binding exists). + If unspecified, metadata.name will be used. minLength: 1 type: string serviceInstanceName: - description: Name of a ServiceInstance resource in the same namespace, - identifying the Cloud Foundry service instance this binding refers - to. + description: |- + Name of a ServiceInstance resource in the same namespace, + identifying the Cloud Foundry service instance this binding refers to. minLength: 1 type: string required: @@ -103,24 +108,28 @@ spec: description: ServiceBindingStatus defines the observed state of ServiceBinding properties: conditions: - description: List of status conditions to indicate the status of a - ServiceBinding. Known condition types are `Ready`. + description: |- + List of status conditions to indicate the status of a ServiceBinding. + Known condition types are `Ready`. items: description: ServiceBindingCondition contains condition information for a ServiceBinding. properties: lastTransitionTime: - description: LastTransitionTime is the timestamp corresponding - to the last status change of this condition. + description: |- + LastTransitionTime is the timestamp corresponding to the last status + change of this condition. format: date-time type: string message: - description: Message is a human readable description of the - details of the last transition, complementing reason. + description: |- + Message is a human readable description of the details of the last + transition, complementing reason. type: string reason: - description: Reason is a brief machine readable explanation - for the condition's last transition. + description: |- + Reason is a brief machine readable explanation for the condition's last + transition. type: string status: description: Status of the condition, one of ('True', 'False', diff --git a/config/crd/bases/cf.cs.sap.com_serviceinstances.yaml b/config/crd/bases/cf.cs.sap.com_serviceinstances.yaml index b88c531..01590ca 100644 --- a/config/crd/bases/cf.cs.sap.com_serviceinstances.yaml +++ b/config/crd/bases/cf.cs.sap.com_serviceinstances.yaml @@ -3,8 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.9.2 - creationTimestamp: null + controller-gen.kubebuilder.io/version: v0.14.0 name: serviceinstances.cf.cs.sap.com spec: group: cf.cs.sap.com @@ -28,14 +27,19 @@ spec: description: ServiceInstance is the Schema for the serviceinstances API properties: apiVersion: - description: 'APIVersion defines the versioned schema of this representation - of an object. Servers should convert recognized schemas to the latest - internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources type: string kind: - description: 'Kind is a string value representing the REST resource this - object represents. Servers may infer this from the endpoint the client - submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds type: string metadata: type: object @@ -43,9 +47,10 @@ spec: description: ServiceInstanceSpec defines the desired state of ServiceInstance properties: clusterSpaceName: - description: Name of a ClusterSpace resource, identifying the Cloud - Foundry space where the instance will be provisioned. Exactly one - of SpaceName and ClusterSpaceName have to be specified. + description: |- + Name of a ClusterSpace resource, + identifying the Cloud Foundry space where the instance will be provisioned. + Exactly one of SpaceName and ClusterSpaceName have to be specified. minLength: 1 type: string name: @@ -54,13 +59,14 @@ spec: minLength: 1 type: string parameters: - description: Instance parameters. Do not provide any sensitve data - here; instead use ParametersFrom for such data. + description: |- + Instance parameters. + Do not provide any sensitve data here; instead use ParametersFrom for such data. x-kubernetes-preserve-unknown-fields: true parametersFrom: - description: References to secrets containing instance parameters. - Top level keys must occur only once across Parameters and the secrest - listed here. + description: |- + References to secrets containing instance parameters. + Top level keys must occur only once across Parameters and the secrest listed here. items: description: ParametersFromSource represents the source of a set of Parameters @@ -83,24 +89,27 @@ spec: type: object type: array serviceOfferingName: - description: Name of the service offering in Cloud Foundry. Either - ServiceOfferingName and ServicePlanName, or ServicePlanGuid must - be specified. + description: |- + Name of the service offering in Cloud Foundry. + Either ServiceOfferingName and ServicePlanName, or ServicePlanGuid must be specified. minLength: 1 type: string servicePlanGuid: - description: GUID of the service plan in Cloud Foundry. Either ServiceOfferingName - and ServicePlanName, or ServicePlanGuid must be specified. + description: |- + GUID of the service plan in Cloud Foundry. + Either ServiceOfferingName and ServicePlanName, or ServicePlanGuid must be specified. minLength: 1 type: string servicePlanName: - description: Name of the service plan in Cloud Foundry. Either ServiceOfferingName - and ServicePlanName, or ServicePlanGuid must be specified. + description: |- + Name of the service plan in Cloud Foundry. + Either ServiceOfferingName and ServicePlanName, or ServicePlanGuid must be specified. minLength: 1 type: string spaceName: - description: Name of a Space resource in the same namespace, identifying - the Cloud Foundry space where the instance will be provisioned. + description: |- + Name of a Space resource in the same namespace, + identifying the Cloud Foundry space where the instance will be provisioned. Exactly one of SpaceName and ClusterSpaceName have to be specified. minLength: 1 type: string @@ -116,24 +125,28 @@ spec: description: ServiceInstanceStatus defines the observed state of ServiceInstance properties: conditions: - description: List of status conditions to indicate the status of a - ServiceInstance. Known condition types are `Ready`. + description: |- + List of status conditions to indicate the status of a ServiceInstance. + Known condition types are `Ready`. items: description: ServiceInstanceCondition contains condition information for a ServiceInstance. properties: lastTransitionTime: - description: LastTransitionTime is the timestamp corresponding - to the last status change of this condition. + description: |- + LastTransitionTime is the timestamp corresponding to the last status + change of this condition. format: date-time type: string message: - description: Message is a human readable description of the - details of the last transition, complementing reason. + description: |- + Message is a human readable description of the details of the last + transition, complementing reason. type: string reason: - description: Reason is a brief machine readable explanation - for the condition's last transition. + description: |- + Reason is a brief machine readable explanation for the condition's last + transition. type: string status: description: Status of the condition, one of ('True', 'False', @@ -160,10 +173,20 @@ spec: description: Last reconciliation timestamp format: date-time type: string + maxRetries: + description: |- + This is the maximum number of retries that are allowed for the reconciliation of this service instance. + If the retry counter exceeds this value, the service instance will be marked as failed. + type: integer observedGeneration: description: Observed generation format: int64 type: integer + retryCounter: + description: |- + Counts the number of retries that have been attempted for the reconciliation of this service instance. + This counter can be used to fail the instance if too many retries occur. + type: integer serviceInstanceDigest: description: Digest identifying the current target state of the service instance (including praameters) diff --git a/config/crd/bases/cf.cs.sap.com_spaces.yaml b/config/crd/bases/cf.cs.sap.com_spaces.yaml index 5286712..6a4324f 100644 --- a/config/crd/bases/cf.cs.sap.com_spaces.yaml +++ b/config/crd/bases/cf.cs.sap.com_spaces.yaml @@ -3,8 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.9.2 - creationTimestamp: null + controller-gen.kubebuilder.io/version: v0.14.0 name: spaces.cf.cs.sap.com spec: group: cf.cs.sap.com @@ -28,14 +27,19 @@ spec: description: Space is the Schema for the spaces API properties: apiVersion: - description: 'APIVersion defines the versioned schema of this representation - of an object. Servers should convert recognized schemas to the latest - internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources type: string kind: - description: 'Kind is a string value representing the REST resource this - object represents. Servers may infer this from the endpoint the client - submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds type: string metadata: type: object @@ -48,18 +52,21 @@ spec: minLength: 1 type: string guid: - description: Space GUID. Must not be specified if Name or OrganizationName - is present. + description: |- + Space GUID. + Must not be specified if Name or OrganizationName is present. minLength: 1 type: string name: - description: Space name. Must not be specified if Guid is present; - defauls to metadata.name otherwise. + description: |- + Space name. + Must not be specified if Guid is present; defauls to metadata.name otherwise. minLength: 1 type: string organizationName: - description: Organization name. Must not be specified if Guid is present; - required otherwise. + description: |- + Organization name. + Must not be specified if Guid is present; required otherwise. minLength: 1 type: string required: @@ -71,24 +78,28 @@ spec: description: SpaceStatus defines the observed state of Space. properties: conditions: - description: List of status conditions to indicate the status of a - Space. Known condition types are `Ready`. + description: |- + List of status conditions to indicate the status of a Space. + Known condition types are `Ready`. items: description: SpaceCondition contains condition information for a Space. properties: lastTransitionTime: - description: LastTransitionTime is the timestamp corresponding - to the last status change of this condition. + description: |- + LastTransitionTime is the timestamp corresponding to the last status + change of this condition. format: date-time type: string message: - description: Message is a human readable description of the - details of the last transition, complementing reason. + description: |- + Message is a human readable description of the details of the last + transition, complementing reason. type: string reason: - description: Reason is a brief machine readable explanation - for the condition's last transition. + description: |- + Reason is a brief machine readable explanation for the condition's last + transition. type: string status: description: Status of the condition, one of ('True', 'False', diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 15cd3d8..90f6514 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -2,7 +2,6 @@ apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: - creationTimestamp: null name: manager-role rules: - apiGroups: diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index 77c5362..372562c 100644 --- a/config/webhook/manifests.yaml +++ b/config/webhook/manifests.yaml @@ -2,7 +2,6 @@ apiVersion: admissionregistration.k8s.io/v1 kind: MutatingWebhookConfiguration metadata: - creationTimestamp: null name: mutating-webhook-configuration webhooks: - admissionReviewVersions: @@ -89,7 +88,6 @@ webhooks: apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingWebhookConfiguration metadata: - creationTimestamp: null name: validating-webhook-configuration webhooks: - admissionReviewVersions: diff --git a/crds/cf.cs.sap.com_clusterspaces.yaml b/crds/cf.cs.sap.com_clusterspaces.yaml index 3f6cc6a..31aef0b 100644 --- a/crds/cf.cs.sap.com_clusterspaces.yaml +++ b/crds/cf.cs.sap.com_clusterspaces.yaml @@ -3,8 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.9.2 - creationTimestamp: null + controller-gen.kubebuilder.io/version: v0.14.0 name: clusterspaces.cf.cs.sap.com spec: group: cf.cs.sap.com @@ -28,14 +27,19 @@ spec: description: ClusterSpace is the Schema for the clusterspaces API properties: apiVersion: - description: 'APIVersion defines the versioned schema of this representation - of an object. Servers should convert recognized schemas to the latest - internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources type: string kind: - description: 'Kind is a string value representing the REST resource this - object represents. Servers may infer this from the endpoint the client - submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds type: string metadata: type: object @@ -48,18 +52,21 @@ spec: minLength: 1 type: string guid: - description: Space GUID. Must not be specified if Name or OrganizationName - is present. + description: |- + Space GUID. + Must not be specified if Name or OrganizationName is present. minLength: 1 type: string name: - description: Space name. Must not be specified if Guid is present; - defauls to metadata.name otherwise. + description: |- + Space name. + Must not be specified if Guid is present; defauls to metadata.name otherwise. minLength: 1 type: string organizationName: - description: Organization name. Must not be specified if Guid is present; - required otherwise. + description: |- + Organization name. + Must not be specified if Guid is present; required otherwise. minLength: 1 type: string required: @@ -71,24 +78,28 @@ spec: description: SpaceStatus defines the observed state of Space. properties: conditions: - description: List of status conditions to indicate the status of a - Space. Known condition types are `Ready`. + description: |- + List of status conditions to indicate the status of a Space. + Known condition types are `Ready`. items: description: SpaceCondition contains condition information for a Space. properties: lastTransitionTime: - description: LastTransitionTime is the timestamp corresponding - to the last status change of this condition. + description: |- + LastTransitionTime is the timestamp corresponding to the last status + change of this condition. format: date-time type: string message: - description: Message is a human readable description of the - details of the last transition, complementing reason. + description: |- + Message is a human readable description of the details of the last + transition, complementing reason. type: string reason: - description: Reason is a brief machine readable explanation - for the condition's last transition. + description: |- + Reason is a brief machine readable explanation for the condition's last + transition. type: string status: description: Status of the condition, one of ('True', 'False', diff --git a/crds/cf.cs.sap.com_servicebindings.yaml b/crds/cf.cs.sap.com_servicebindings.yaml index 8cbd942..529f519 100644 --- a/crds/cf.cs.sap.com_servicebindings.yaml +++ b/crds/cf.cs.sap.com_servicebindings.yaml @@ -3,8 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.9.2 - creationTimestamp: null + controller-gen.kubebuilder.io/version: v0.14.0 name: servicebindings.cf.cs.sap.com spec: group: cf.cs.sap.com @@ -28,14 +27,19 @@ spec: description: ServiceBinding is the Schema for the servicebindings API properties: apiVersion: - description: 'APIVersion defines the versioned schema of this representation - of an object. Servers should convert recognized schemas to the latest - internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources type: string kind: - description: 'Kind is a string value representing the REST resource this - object represents. Servers may infer this from the endpoint the client - submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds type: string metadata: type: object @@ -48,13 +52,14 @@ spec: minLength: 1 type: string parameters: - description: Binding parameters. Do not provide any sensitve data - here; instead use ParametersFrom for such data. + description: |- + Binding parameters. + Do not provide any sensitve data here; instead use ParametersFrom for such data. x-kubernetes-preserve-unknown-fields: true parametersFrom: - description: References to secrets containing binding parameters. - Top level keys must occur only once across Parameters and the secrest - listed here. + description: |- + References to secrets containing binding parameters. + Top level keys must occur only once across Parameters and the secrest listed here. items: description: ParametersFromSource represents the source of a set of Parameters @@ -77,21 +82,21 @@ spec: type: object type: array secretKey: - description: Secret key (referring to SecretName) where the binding - credentials will be stored. If unspecified, the top level keys of - the binding credentials will become the secret keys. + description: |- + Secret key (referring to SecretName) where the binding credentials will be stored. + If unspecified, the top level keys of the binding credentials will become the secret keys. minLength: 1 type: string secretName: - description: Secret name where the binding credentials shall be stored - (in the same namespace where the binding exists). If unspecified, - metadata.name will be used. + description: |- + Secret name where the binding credentials shall be stored (in the same namespace where the binding exists). + If unspecified, metadata.name will be used. minLength: 1 type: string serviceInstanceName: - description: Name of a ServiceInstance resource in the same namespace, - identifying the Cloud Foundry service instance this binding refers - to. + description: |- + Name of a ServiceInstance resource in the same namespace, + identifying the Cloud Foundry service instance this binding refers to. minLength: 1 type: string required: @@ -103,24 +108,28 @@ spec: description: ServiceBindingStatus defines the observed state of ServiceBinding properties: conditions: - description: List of status conditions to indicate the status of a - ServiceBinding. Known condition types are `Ready`. + description: |- + List of status conditions to indicate the status of a ServiceBinding. + Known condition types are `Ready`. items: description: ServiceBindingCondition contains condition information for a ServiceBinding. properties: lastTransitionTime: - description: LastTransitionTime is the timestamp corresponding - to the last status change of this condition. + description: |- + LastTransitionTime is the timestamp corresponding to the last status + change of this condition. format: date-time type: string message: - description: Message is a human readable description of the - details of the last transition, complementing reason. + description: |- + Message is a human readable description of the details of the last + transition, complementing reason. type: string reason: - description: Reason is a brief machine readable explanation - for the condition's last transition. + description: |- + Reason is a brief machine readable explanation for the condition's last + transition. type: string status: description: Status of the condition, one of ('True', 'False', diff --git a/crds/cf.cs.sap.com_serviceinstances.yaml b/crds/cf.cs.sap.com_serviceinstances.yaml index b88c531..01590ca 100644 --- a/crds/cf.cs.sap.com_serviceinstances.yaml +++ b/crds/cf.cs.sap.com_serviceinstances.yaml @@ -3,8 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.9.2 - creationTimestamp: null + controller-gen.kubebuilder.io/version: v0.14.0 name: serviceinstances.cf.cs.sap.com spec: group: cf.cs.sap.com @@ -28,14 +27,19 @@ spec: description: ServiceInstance is the Schema for the serviceinstances API properties: apiVersion: - description: 'APIVersion defines the versioned schema of this representation - of an object. Servers should convert recognized schemas to the latest - internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources type: string kind: - description: 'Kind is a string value representing the REST resource this - object represents. Servers may infer this from the endpoint the client - submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds type: string metadata: type: object @@ -43,9 +47,10 @@ spec: description: ServiceInstanceSpec defines the desired state of ServiceInstance properties: clusterSpaceName: - description: Name of a ClusterSpace resource, identifying the Cloud - Foundry space where the instance will be provisioned. Exactly one - of SpaceName and ClusterSpaceName have to be specified. + description: |- + Name of a ClusterSpace resource, + identifying the Cloud Foundry space where the instance will be provisioned. + Exactly one of SpaceName and ClusterSpaceName have to be specified. minLength: 1 type: string name: @@ -54,13 +59,14 @@ spec: minLength: 1 type: string parameters: - description: Instance parameters. Do not provide any sensitve data - here; instead use ParametersFrom for such data. + description: |- + Instance parameters. + Do not provide any sensitve data here; instead use ParametersFrom for such data. x-kubernetes-preserve-unknown-fields: true parametersFrom: - description: References to secrets containing instance parameters. - Top level keys must occur only once across Parameters and the secrest - listed here. + description: |- + References to secrets containing instance parameters. + Top level keys must occur only once across Parameters and the secrest listed here. items: description: ParametersFromSource represents the source of a set of Parameters @@ -83,24 +89,27 @@ spec: type: object type: array serviceOfferingName: - description: Name of the service offering in Cloud Foundry. Either - ServiceOfferingName and ServicePlanName, or ServicePlanGuid must - be specified. + description: |- + Name of the service offering in Cloud Foundry. + Either ServiceOfferingName and ServicePlanName, or ServicePlanGuid must be specified. minLength: 1 type: string servicePlanGuid: - description: GUID of the service plan in Cloud Foundry. Either ServiceOfferingName - and ServicePlanName, or ServicePlanGuid must be specified. + description: |- + GUID of the service plan in Cloud Foundry. + Either ServiceOfferingName and ServicePlanName, or ServicePlanGuid must be specified. minLength: 1 type: string servicePlanName: - description: Name of the service plan in Cloud Foundry. Either ServiceOfferingName - and ServicePlanName, or ServicePlanGuid must be specified. + description: |- + Name of the service plan in Cloud Foundry. + Either ServiceOfferingName and ServicePlanName, or ServicePlanGuid must be specified. minLength: 1 type: string spaceName: - description: Name of a Space resource in the same namespace, identifying - the Cloud Foundry space where the instance will be provisioned. + description: |- + Name of a Space resource in the same namespace, + identifying the Cloud Foundry space where the instance will be provisioned. Exactly one of SpaceName and ClusterSpaceName have to be specified. minLength: 1 type: string @@ -116,24 +125,28 @@ spec: description: ServiceInstanceStatus defines the observed state of ServiceInstance properties: conditions: - description: List of status conditions to indicate the status of a - ServiceInstance. Known condition types are `Ready`. + description: |- + List of status conditions to indicate the status of a ServiceInstance. + Known condition types are `Ready`. items: description: ServiceInstanceCondition contains condition information for a ServiceInstance. properties: lastTransitionTime: - description: LastTransitionTime is the timestamp corresponding - to the last status change of this condition. + description: |- + LastTransitionTime is the timestamp corresponding to the last status + change of this condition. format: date-time type: string message: - description: Message is a human readable description of the - details of the last transition, complementing reason. + description: |- + Message is a human readable description of the details of the last + transition, complementing reason. type: string reason: - description: Reason is a brief machine readable explanation - for the condition's last transition. + description: |- + Reason is a brief machine readable explanation for the condition's last + transition. type: string status: description: Status of the condition, one of ('True', 'False', @@ -160,10 +173,20 @@ spec: description: Last reconciliation timestamp format: date-time type: string + maxRetries: + description: |- + This is the maximum number of retries that are allowed for the reconciliation of this service instance. + If the retry counter exceeds this value, the service instance will be marked as failed. + type: integer observedGeneration: description: Observed generation format: int64 type: integer + retryCounter: + description: |- + Counts the number of retries that have been attempted for the reconciliation of this service instance. + This counter can be used to fail the instance if too many retries occur. + type: integer serviceInstanceDigest: description: Digest identifying the current target state of the service instance (including praameters) diff --git a/crds/cf.cs.sap.com_spaces.yaml b/crds/cf.cs.sap.com_spaces.yaml index 5286712..6a4324f 100644 --- a/crds/cf.cs.sap.com_spaces.yaml +++ b/crds/cf.cs.sap.com_spaces.yaml @@ -3,8 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.9.2 - creationTimestamp: null + controller-gen.kubebuilder.io/version: v0.14.0 name: spaces.cf.cs.sap.com spec: group: cf.cs.sap.com @@ -28,14 +27,19 @@ spec: description: Space is the Schema for the spaces API properties: apiVersion: - description: 'APIVersion defines the versioned schema of this representation - of an object. Servers should convert recognized schemas to the latest - internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources type: string kind: - description: 'Kind is a string value representing the REST resource this - object represents. Servers may infer this from the endpoint the client - submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds type: string metadata: type: object @@ -48,18 +52,21 @@ spec: minLength: 1 type: string guid: - description: Space GUID. Must not be specified if Name or OrganizationName - is present. + description: |- + Space GUID. + Must not be specified if Name or OrganizationName is present. minLength: 1 type: string name: - description: Space name. Must not be specified if Guid is present; - defauls to metadata.name otherwise. + description: |- + Space name. + Must not be specified if Guid is present; defauls to metadata.name otherwise. minLength: 1 type: string organizationName: - description: Organization name. Must not be specified if Guid is present; - required otherwise. + description: |- + Organization name. + Must not be specified if Guid is present; required otherwise. minLength: 1 type: string required: @@ -71,24 +78,28 @@ spec: description: SpaceStatus defines the observed state of Space. properties: conditions: - description: List of status conditions to indicate the status of a - Space. Known condition types are `Ready`. + description: |- + List of status conditions to indicate the status of a Space. + Known condition types are `Ready`. items: description: SpaceCondition contains condition information for a Space. properties: lastTransitionTime: - description: LastTransitionTime is the timestamp corresponding - to the last status change of this condition. + description: |- + LastTransitionTime is the timestamp corresponding to the last status + change of this condition. format: date-time type: string message: - description: Message is a human readable description of the - details of the last transition, complementing reason. + description: |- + Message is a human readable description of the details of the last + transition, complementing reason. type: string reason: - description: Reason is a brief machine readable explanation - for the condition's last transition. + description: |- + Reason is a brief machine readable explanation for the condition's last + transition. type: string status: description: Status of the condition, one of ('True', 'False', diff --git a/internal/cf/instance.go b/internal/cf/instance.go index 422ea43..7dcfa03 100644 --- a/internal/cf/instance.go +++ b/internal/cf/instance.go @@ -134,6 +134,7 @@ func (c *spaceClient) UpdateInstance(ctx context.Context, guid string, name stri } func (c *spaceClient) DeleteInstance(ctx context.Context, guid string) error { + // TODO: return jobGUID to enable querying the job deletion status _, err := c.client.ServiceInstances.Delete(ctx, guid) return err } diff --git a/internal/controllers/servicebinding_controller.go b/internal/controllers/servicebinding_controller.go index 6077a03..b9099fd 100644 --- a/internal/controllers/servicebinding_controller.go +++ b/internal/controllers/servicebinding_controller.go @@ -311,7 +311,11 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque } else if serviceBinding.Annotations["service-operator.cf.cs.sap.com/with-sap-binding-metadata"] == "false" { withMetadata = false } - r.storeBindingSecret(ctx, serviceInstance, serviceBinding, cfbinding.Credentials, spec.SecretName, spec.SecretKey, withMetadata) + err = r.storeBindingSecret(ctx, serviceInstance, serviceBinding, cfbinding.Credentials, spec.SecretName, spec.SecretKey, withMetadata) + if err != nil { + // TODO: implement error handling + return ctrl.Result{RequeueAfter: 10 * time.Minute}, nil + } // TODO: apply some increasing period, depending on the age of the last update return ctrl.Result{RequeueAfter: 10 * time.Minute}, nil case facade.BindingStateCreatedFailed, facade.BindingStateDeleteFailed: diff --git a/internal/controllers/serviceinstance_controller.go b/internal/controllers/serviceinstance_controller.go index 1dc4581..5b0efa0 100644 --- a/internal/controllers/serviceinstance_controller.go +++ b/internal/controllers/serviceinstance_controller.go @@ -8,8 +8,11 @@ package controllers import ( "context" "fmt" + "math" + "strconv" "time" + "github.com/go-logr/logr" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" @@ -36,6 +39,15 @@ const ( serviceInstanceReadyConditionReasonError = "Error" serviceInstanceReadyConditionReasonDeletionBlocked = "DeletionBlocked" // Additionally, all of facade.InstanceState* may occur as Ready condition reason + + // Default values while waiting for ServiceInstance creation (state Progressing) + serviceInstanceDefaultReconcileInterval = 1 * time.Second + //serviceInstanceDefaultMaxReconcileInterval = 10 * time.Minute + + // Default values for error cases during ServiceInstance creation + serviceInstanceDefaultMaxRetries = math.MaxInt32 // infinite number of retries + serviceInstanceDefaultRetryInterval = 1 * time.Second + serviceInstanceDefaultMaxRetryInterval = 1 * time.Minute ) // ServiceInstanceReconciler reconciles a ServiceInstance object @@ -46,6 +58,9 @@ type ServiceInstanceReconciler struct { ClientBuilder facade.SpaceClientBuilder } +// RetryError is a special error to indicate that the operation should be retried. +var RetryError = errors.New("retry") + // +kubebuilder:rbac:groups=cf.cs.sap.com,resources=serviceinstances,verbs=get;list;watch;update // +kubebuilder:rbac:groups=cf.cs.sap.com,resources=serviceinstances/status,verbs=get;update;patch // +kubebuilder:rbac:groups=cf.cs.sap.com,resources=serviceinstances/finalizers,verbs=update @@ -81,9 +96,12 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ if skipStatusUpdate { return } + if err != nil { - serviceInstance.SetReadyCondition(cfv1alpha1.ConditionFalse, serviceInstanceReadyConditionReasonError, err.Error()) + result, err = r.HandleError(ctx, serviceInstance, err, log) } + + // update service instance CR if updateErr := r.Status().Update(ctx, serviceInstance); updateErr != nil { err = utilerrors.NewAggregate([]error{err, updateErr}) result = ctrl.Result{} @@ -93,6 +111,7 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ // Set a first status (and requeue, because the status update itself will not trigger another reconciliation because of the event filter set) if ready := serviceInstance.GetReadyCondition(); ready == nil { serviceInstance.SetReadyCondition(cfv1alpha1.ConditionUnknown, serviceInstanceReadyConditionReasonNew, "First seen") + SetMaxRetries(serviceInstance, log) return ctrl.Result{Requeue: true}, nil } @@ -146,6 +165,8 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ ); err != nil { return ctrl.Result{}, errors.Wrap(err, "failed to list depending service bindings") } + // Retrieve reconcileTimeout + reconcileTimeout := getReconcileTimeout(serviceInstance) // Require readiness of space unless in deletion case if serviceInstance.DeletionTimestamp.IsZero() { @@ -229,7 +250,7 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ status.ServiceInstanceDigest = facade.ObjectHash(map[string]interface{}{"generation": serviceInstance.Generation, "parameters": parameters}) - recreateOnCreationFailure := serviceInstance.Annotations["service-operator.cf.cs.sap.com/recreate-on-creation-failure"] == "true" + recreateOnCreationFailure := serviceInstance.Annotations[cfv1alpha1.AnnotationRecreate] == "true" inRecreation := false if cfinstance == nil { @@ -243,17 +264,17 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ string(serviceInstance.UID), serviceInstance.Generation, ); err != nil { - return ctrl.Result{}, err + return ctrl.Result{}, RetryError } status.LastModifiedAt = &[]metav1.Time{metav1.Now()}[0] } else { if cfinstance.State == facade.InstanceStateDeleting { // This is the re-creation case; nothing to, we just wait until it is gone - } else if recreateOnCreationFailure && cfinstance.State == facade.InstanceStateCreatedFailed { + } else if recreateOnCreationFailure && (cfinstance.State == facade.InstanceStateCreatedFailed || cfinstance.State == facade.InstanceStateDeleteFailed) { // Re-create instance log.V(1).Info("triggering re-creation") if err := client.DeleteInstance(ctx, cfinstance.Guid); err != nil { - return ctrl.Result{}, err + return ctrl.Result{}, RetryError } status.LastModifiedAt = &[]metav1.Time{metav1.Now()}[0] inRecreation = true @@ -324,16 +345,18 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ switch cfinstance.State { case facade.InstanceStateReady: serviceInstance.SetReadyCondition(cfv1alpha1.ConditionTrue, string(cfinstance.State), cfinstance.StateDescription) + serviceInstance.Status.RetryCounter = 0 // Reset the retry counter // TODO: apply some increasing period, depending on the age of the last update return ctrl.Result{RequeueAfter: 10 * time.Minute}, nil case facade.InstanceStateCreatedFailed, facade.InstanceStateUpdateFailed, facade.InstanceStateDeleteFailed: - serviceInstance.SetReadyCondition(cfv1alpha1.ConditionFalse, string(cfinstance.State), cfinstance.StateDescription) - // TODO: apply some increasing period, depending on the age of the last update - return ctrl.Result{RequeueAfter: 1 * time.Minute}, nil + // Check if the retry counter exceeds the maximum allowed retries. + // Check if the maximum retry limit is exceeded. + return ctrl.Result{}, RetryError default: + // Processing case serviceInstance.SetReadyCondition(cfv1alpha1.ConditionUnknown, string(cfinstance.State), cfinstance.StateDescription) // TODO: apply some increasing period, depending on the age of the last update - return ctrl.Result{RequeueAfter: 10 * time.Second}, nil + return ctrl.Result{RequeueAfter: reconcileTimeout}, nil } } else if len(serviceBindingList.Items) > 0 { serviceInstance.SetReadyCondition(cfv1alpha1.ConditionUnknown, serviceInstanceReadyConditionReasonDeletionBlocked, "Waiting for deletion of depending service bindings") @@ -381,3 +404,82 @@ func (r *ServiceInstanceReconciler) SetupWithManager(mgr ctrl.Manager) error { WithEventFilter(predicate.Or(predicate.GenerationChangedPredicate{}, predicate.AnnotationChangedPredicate{})). Complete(r) } + +// IncrementRetryCounterAndCheckRetryLimit increments the retry counter for a ServiceInstance and checks if the number of retries has exceeded the maximum allowed retries. +// The maximum retries is configured per ServiceInstance via the annotation, AnnotationMaxRetries. If not specified, +// a default value is used. +// This function updates the ServiceInstance's Condition and State to indicate a failure when the retry limit is reached. +// Returns:A boolean indicating whether the retry limit has been reached. +func SetMaxRetries(serviceInstance *cfv1alpha1.ServiceInstance, log logr.Logger) { + // Default to an infinite number number of retries + serviceInstance.Status.MaxRetries = serviceInstanceDefaultMaxRetries + + // Use max retries from annotation + maxRetriesStr, found := serviceInstance.GetAnnotations()[cfv1alpha1.AnnotationMaxRetries] + if found { + maxRetries, err := strconv.Atoi(maxRetriesStr) + if err != nil { + log.V(1).Info("Invalid max retries annotation value, using default", "AnnotationMaxRetries", maxRetriesStr) + } else { + serviceInstance.Status.MaxRetries = maxRetries + } + } +} + +// function to read/get reconcile timeout annotation from the service instance "AnnotationReconcileTimeout = "service-operator.cf.cs.sap.com/timeout-on-reconcile" " +// if the annotation is not set, the default value is used serviceInstanceDefaultRequeueTimeout +// else returns the reconcile timeout as a time duration +func getReconcileTimeout(serviceInstance *cfv1alpha1.ServiceInstance) time.Duration { + // Use reconcile timeout from annotation, use default if annotation is missing or not parsable + reconcileTimeoutStr, ok := serviceInstance.GetAnnotations()[cfv1alpha1.AnnotationReconcileTimeout] + if !ok { + return serviceInstanceDefaultReconcileInterval + } + reconcileTimeout, err := time.ParseDuration(reconcileTimeoutStr) + if err != nil { + return serviceInstanceDefaultReconcileInterval + } + return reconcileTimeout +} + +// HandleError sets conditions and the context to handle the error. +// Special handling for retryable errros: +// - retry after certain time interval +// - doubling time interval for consecutive errors +// - time interval is capped at a certain maximum value +func (r *ServiceInstanceReconciler) HandleError(ctx context.Context, serviceInstance *cfv1alpha1.ServiceInstance, issue error, log logr.Logger) (ctrl.Result, error) { + if issue != RetryError { + serviceInstance.SetReadyCondition(cfv1alpha1.ConditionUnknown, serviceInstanceReadyConditionReasonError, issue.Error()) + return ctrl.Result{}, issue + } + + // re-create case + + // Check if the retry counter exceeds the maximum allowed retries. + serviceInstance.Status.RetryCounter++ + if serviceInstance.Status.MaxRetries != serviceInstanceDefaultMaxRetries && serviceInstance.Status.RetryCounter >= serviceInstance.Status.MaxRetries { + // Update the instance's status to reflect the failure due to too many retries. + serviceInstance.SetReadyCondition(cfv1alpha1.ConditionFalse, "MaximumRetriesExceeded", "The service instance has failed due to too many retries.") + return ctrl.Result{}, nil // finish reconcile loop + } + + // double the requeue interval + condition := serviceInstance.GetReadyCondition() + requeueAfter := 1 * time.Second + // TODO: do we need this: && condition.Status == cfv1alpha1.ConditionStatus(corev1.ConditionFalse)? + if condition != nil && !condition.LastTransitionTime.Time.IsZero() { + conditionRequeueAfter := time.Since(condition.LastTransitionTime.Time).Round(time.Second) + if conditionRequeueAfter > requeueAfter { + requeueAfter = conditionRequeueAfter + } + } + // cap the requeue interval if necessary + if requeueAfter > serviceInstanceDefaultMaxRetryInterval { + requeueAfter = serviceInstanceDefaultMaxRetryInterval + } + + log.V(1).Info("***Retry after interval", "RequeueAfter", requeueAfter.String()) + + serviceInstance.SetReadyCondition(cfv1alpha1.ConditionUnknown, serviceInstanceReadyConditionReasonError, issue.Error()) + return ctrl.Result{RequeueAfter: requeueAfter}, nil +} diff --git a/internal/controllers/serviceinstance_controller_integration_test.go b/internal/controllers/serviceinstance_controller_integration_test.go new file mode 100644 index 0000000..37e4083 --- /dev/null +++ b/internal/controllers/serviceinstance_controller_integration_test.go @@ -0,0 +1,345 @@ +/* +SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and cf-service-operator contributors +SPDX-License-Identifier: Apache-2.0 +*/ +package controllers + +import ( + "context" + "strconv" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/sap/cf-service-operator/api/v1alpha1" + "github.com/sap/cf-service-operator/internal/facade" + "github.com/sap/cf-service-operator/internal/facade/facadefakes" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// constants useful for this controller +// Note: +// - if constants are used in multiple controllers, consider moving them to suite_test.go +// - use separate resource names to prevent collisions between tests +const ( + testCfInstName = "test-instance" + testK8sInstNameCreate = "test-instance-create" + testK8sInstNameRecreate = "test-instance-recreate" + testK8sInstNameCreateInstanceFails = "test-instance-create-instance-fails" + testK8sInstNameStateCreatedFailed = "test-instance-state-created-failed" + testK8sInstNameStateDeleteFailed = "test-instance-state-delete-failed" + testK8sInstNameStateDeleteFailedInfinite = "test-instance-state-delete-failed-infinite" + testK8sInstNameRecreateInfinite = "test-instance-recreate-infinite" + testSpaceNameInstances = "test-space-instances" // used for K8s CR and CF space +) + +var fakeInstanceReady = &facade.Instance{ + Guid: testCfSpaceGuid, + Name: testCfInstName, + ServicePlanGuid: testCfPlanGuid, + Owner: testCfOwner, + Generation: 1, + State: facade.InstanceStateReady, + StateDescription: string(facade.InstanceStateReady), +} + +// ----------------------------------------------------------------------------------------------- +// Tests +// ----------------------------------------------------------------------------------------------- + +var _ = Describe("Service Instance Controller Integration Tests", Ordered, func() { + ctx := context.Background() + + BeforeAll(func() { + // reset all fake clients to always start with clean state (e.g. call counts of zero) + fakeOrgClient = &facadefakes.FakeOrganizationClient{} + fakeSpaceClient = &facadefakes.FakeSpaceClient{} + fakeSpaceHealthChecker = &facadefakes.FakeSpaceHealthChecker{} + + fakeSpace := &facade.Space{ + Guid: testCfSpaceGuid, + Name: testSpaceNameInstances, + Owner: testCfOwner, + Generation: 1, + } + + fakeOrgClient.CreateSpaceReturns(nil) + fakeOrgClient.GetSpaceReturns(fakeSpace, nil) + // only the first call returns no resource to force the creation by the controller + fakeOrgClient.GetSpaceReturnsOnCall(0, nil, nil) + + By("creating space CR") + spaceCR := createSpaceCR(ctx, testSpaceNameInstances) + waitForSpaceCR(ctx, client.ObjectKeyFromObject(spaceCR)) + Expect(fakeOrgClient.CreateSpaceCallCount()).To(Equal(1)) + }) + + Describe("Reconcile", func() { + BeforeEach(func() { + // reset all fake clients to always start with clean state (e.g. call counts of zero) + fakeOrgClient = &facadefakes.FakeOrganizationClient{} + fakeSpaceClient = &facadefakes.FakeSpaceClient{} + fakeSpaceHealthChecker = &facadefakes.FakeSpaceHealthChecker{} + }) + + It("should create instance", func() { + // prepare fake CF responses + fakeSpaceClient.FindServicePlanReturns(testCfPlanGuid, kNoError) + fakeSpaceClient.CreateInstanceReturns(kNoError) + + // 0) GetInstance is called before CreateInstance to check existence => simulate non-existing instance + // 1) GetInstance is called after CreateInstance to check current state => simulate ready instance + fakeSpaceClient.GetInstanceReturnsOnCall(0, kNoInstance, kNoError) + fakeSpaceClient.GetInstanceReturns(fakeInstanceReady, kNoError) + + // perform actual test + infinite := false + instanceCR := createInstanceCR(ctx, testK8sInstNameCreate, testSpaceNameInstances, infinite) + waitForInstanceCR(ctx, client.ObjectKeyFromObject(instanceCR)) + + // check expectations on reconcile loop + Expect(fakeSpaceClient.CreateInstanceCallCount()).To(Equal(1)) + Expect(fakeSpaceClient.GetInstanceCallCount()).To(Equal(2)) + }) + + It("should re-create instance", func() { + // prepare fake CF responses + fakeInstanceFailed := *fakeInstanceReady // copy struct + fakeInstanceFailed.State = facade.InstanceStateCreatedFailed + fakeInstanceFailed.StateDescription = string(facade.InstanceStateCreatedFailed) + fakeSpaceClient.FindServicePlanReturns(testCfPlanGuid, kNoError) + fakeSpaceClient.DeleteInstanceReturns(kNoError) + fakeSpaceClient.CreateInstanceReturns(kNoError) + + // 0) simulate failed instance to force deletion by controller + fakeSpaceClient.GetInstanceReturnsOnCall(0, &fakeInstanceFailed, kNoError) + // 1) simulate missing instance to force re-creation by controller + fakeSpaceClient.GetInstanceReturnsOnCall(1, kNoInstance, kNoError) + fakeSpaceClient.GetInstanceReturnsOnCall(2, kNoInstance, kNoError) + // 3) simulate ready instance to finish the test + fakeSpaceClient.GetInstanceReturnsOnCall(3, fakeInstanceReady, kNoError) + // other) GetInstance should return errors if called more often than expected + fakeSpaceClient.GetInstanceReturns(kNoInstance, errNotExpected) + + // perform actual test + recreateFlag := true + infinite := false + instanceCR := createInstanceCR(ctx, testK8sInstNameRecreate, testSpaceNameInstances, infinite, recreateFlag) + waitForInstanceCR(ctx, client.ObjectKeyFromObject(instanceCR)) + + // check expectations on reconcile loop + Expect(fakeSpaceClient.DeleteInstanceCallCount()).To(Equal(1)) + Expect(fakeSpaceClient.CreateInstanceCallCount()).To(Equal(1)) + Expect(fakeSpaceClient.GetInstanceCallCount()).To(Equal(4)) + // TODO: check if number of calls to GetSpace can be reduced + }) + + // Context when the creation of a service instance exceeds the maximum number of retry attempts. + It("should not re-create instance after max retries (CreateInstance fails)", func() { + // Prepare fake CF responses to simulate instance creation failure on the below CreateInstance calls + fakeInstanceFailed := *fakeInstanceReady + fakeInstanceFailed.State = facade.InstanceStateCreatedFailed + + fakeOrgClient.GetSpaceReturns(&facade.Space{Guid: testCfSpaceGuid}, nil) + fakeSpaceClient.FindServicePlanReturns(testCfPlanGuid, kNoError) + fakeSpaceClient.DeleteInstanceReturns(kNoError) + + // CreateInstance shall always fail directly + fakeSpaceClient.CreateInstanceReturns(errCreateInstanceFail) + + // GetInstance shall return errors except for below cases + fakeSpaceClient.GetInstanceReturns(kNoInstance, errNotExpected) + fakeSpaceClient.GetInstanceReturnsOnCall(0, &fakeInstanceFailed, kNoError) // Instance creation fails + for i := 1; i <= 8; i++ { + fakeSpaceClient.GetInstanceReturnsOnCall(i, kNoInstance, kNoError) + } + + // Perform the actual test + recreateFlag := true + infinite := false + srvInstanceCR := createInstanceCR(ctx, testK8sInstNameCreateInstanceFails, testSpaceNameInstances, infinite, recreateFlag) + finalInstanceCR := waitForInstanceCRToFail(ctx, client.ObjectKeyFromObject(srvInstanceCR)) + + // Verify the instance CR is in a failed state after exceeding retries + Expect(finalInstanceCR.Status.State).To(Equal(v1alpha1.ServiceInstanceStateError), "ServiceInstance should be in error state after max retries exceeded") + + // Check the last condition matches the "MaximumRetriesExceeded" reason + conditions := finalInstanceCR.Status.Conditions + var reason string + var message string + for _, condition := range conditions { + if condition.Status == v1alpha1.ConditionFalse { + reason = condition.Reason + message = condition.Message + } + } + Expect(reason).To(Equal("MaximumRetriesExceeded")) + Expect(message).To(Equal("The service instance has failed due to too many retries.")) + + // MaxRetries and RetryCounter assertions + // The ServiceInstance CR has recorded the expected number of retries and it matches the max retries limit. + maxRetriesAnnotationValue := finalInstanceCR.Annotations[v1alpha1.AnnotationMaxRetries] + Expect(maxRetriesAnnotationValue).NotTo(BeEmpty(), "MaxRetries annotation should be set on the ServiceInstance CR") + maxRetries, err := strconv.Atoi(maxRetriesAnnotationValue) + Expect(err).NotTo(HaveOccurred(), "MaxRetries annotation should be an integer") + Expect(finalInstanceCR.Status.RetryCounter).To(BeNumerically(">=", maxRetries), "RetryCounter should be higher than MaxRetries") + + // Check that CreateInstance was called several times, respecting the max retries limit + Expect(fakeSpaceClient.CreateInstanceCallCount()).To(Equal(testServiceInstanceDefaultMaxRetries)) + Expect(fakeSpaceClient.GetInstanceCallCount()).To(Equal(testServiceInstanceDefaultMaxRetries + 2)) + }) + + It("should not re-create instance after max retries (state CreatedFailed)", func() { + // Prepare fake CF responses to simulate instance creation failure on the below CreateInstance calls + fakeInstanceFailed := *fakeInstanceReady + fakeInstanceFailed.State = facade.InstanceStateCreatedFailed + + fakeOrgClient.GetSpaceReturns(&facade.Space{Guid: testCfSpaceGuid}, nil) + fakeSpaceClient.FindServicePlanReturns(testCfPlanGuid, kNoError) + fakeSpaceClient.DeleteInstanceReturns(kNoError) + + // CreateInstance shall always succeed, but the instance shall go to CreatedFailed state later on + fakeSpaceClient.CreateInstanceReturns(kNoError) + + // GetInstance shall always return instance in state CreatedFailed + fakeSpaceClient.GetInstanceReturns(kNoInstance, errNotExpected) + for i := 0; i < testServiceInstanceDefaultMaxRetries*4; i += 4 { + fakeSpaceClient.GetInstanceReturnsOnCall(i+0, &fakeInstanceFailed, kNoError) + fakeSpaceClient.GetInstanceReturnsOnCall(i+1, kNoInstance, kNoError) + fakeSpaceClient.GetInstanceReturnsOnCall(i+2, kNoInstance, kNoError) + fakeSpaceClient.GetInstanceReturnsOnCall(i+3, &fakeInstanceFailed, kNoError) + } + + // Perform the actual test + recreateFlag := true + infinite := false + srvInstanceCR := createInstanceCR(ctx, testK8sInstNameStateCreatedFailed, testSpaceNameInstances, infinite, recreateFlag) + finalInstanceCR := waitForInstanceCRToFail(ctx, client.ObjectKeyFromObject(srvInstanceCR)) + + // Verify the instance CR is in a failed state after exceeding retries + Expect(finalInstanceCR.Status.State).To(Equal(v1alpha1.ServiceInstanceStateError), "ServiceInstance should be in error state after max retries exceeded") + + // Check the last condition matches the "MaximumRetriesExceeded" reason + conditions := finalInstanceCR.Status.Conditions + var reason string + var message string + for _, condition := range conditions { + if condition.Status == v1alpha1.ConditionFalse { + reason = condition.Reason + message = condition.Message + } + } + Expect(reason).To(Equal("MaximumRetriesExceeded")) + Expect(message).To(Equal("The service instance has failed due to too many retries.")) + + // MaxRetries and RetryCounter assertions + // The ServiceInstance CR has recorded the expected number of retries and it matches the max retries limit. + maxRetriesAnnotationValue := finalInstanceCR.Annotations[v1alpha1.AnnotationMaxRetries] + Expect(maxRetriesAnnotationValue).NotTo(BeEmpty(), "MaxRetries annotation should be set on the ServiceInstance CR") + maxRetries, err := strconv.Atoi(maxRetriesAnnotationValue) + Expect(err).NotTo(HaveOccurred(), "MaxRetries annotation should be an integer") + Expect(finalInstanceCR.Status.RetryCounter).To(BeNumerically(">=", maxRetries), "RetryCounter should be higher than MaxRetries") + + // Check that CreateInstance was called several times, respecting the max retries limit + Expect(fakeSpaceClient.CreateInstanceCallCount()).To(Equal(testServiceInstanceDefaultMaxRetries)) + Expect(fakeSpaceClient.GetInstanceCallCount()).To(Equal(testServiceInstanceDefaultMaxRetries * 4)) + }) + + It("should retry delete instance until max retries (state DeleteFailed)", func() { + // Prepare fake CF responses to simulate instance creation failure on the below CreateInstance calls + fakeInstanceFailed := *fakeInstanceReady + fakeInstanceFailed.State = facade.InstanceStateDeleteFailed + fakeOrgClient.GetSpaceReturns(&facade.Space{Guid: testCfSpaceGuid}, nil) + fakeSpaceClient.FindServicePlanReturns(testCfPlanGuid, kNoError) + fakeSpaceClient.DeleteInstanceReturns(errDeleteInstanceFail) + + // CreateInstance shall always succeed, but the instance shall go to DeleteFailed state later on + fakeSpaceClient.CreateInstanceReturns(kNoError) + + // GetInstance shall always return instance in state DeleteFailed + fakeSpaceClient.GetInstanceReturns(&fakeInstanceFailed, kNoError) + + // Perform the actual test + recreateFlag := true + infinite := false + srvInstanceCR := createInstanceCR(ctx, testK8sInstNameStateDeleteFailed, testSpaceNameInstances, infinite, recreateFlag) + finalInstanceCR := waitForInstanceCRToFail(ctx, client.ObjectKeyFromObject(srvInstanceCR)) + + // Verify the instance CR is in a failed state after exceeding retries + Expect(finalInstanceCR.Status.State).To(Equal(v1alpha1.ServiceInstanceStateError), "ServiceInstance should be in error state after max retries exceeded") + + // Check that CreateInstance was called several times, respecting the max retries limit + Expect(fakeSpaceClient.DeleteInstanceCallCount()).To(Equal(5)) + }) + + It("should retry delete instance until infinite retries (state DeleteFailed)", func() { + // Prepare fake CF responses to simulate instance creation failure on the below CreateInstance calls + fakeInstanceFailed := *fakeInstanceReady + fakeInstanceFailed.State = facade.InstanceStateDeleteFailed + fakeInstanceFailed.StateDescription = string(facade.InstanceStateDeleteFailed) + fakeOrgClient.GetSpaceReturns(&facade.Space{Guid: testCfSpaceGuid}, nil) + fakeSpaceClient.FindServicePlanReturns(testCfPlanGuid, kNoError) + fakeSpaceClient.DeleteInstanceReturns(kNoError) + + // CreateInstance shall always succeed, but the instance shall go to DeleteFailed state later on + fakeSpaceClient.CreateInstanceReturns(kNoError) + + for i := 0; i <= 3; i++ { + fakeSpaceClient.GetInstanceReturnsOnCall(i, &fakeInstanceFailed, kNoError) + fakeSpaceClient.DeleteInstanceReturnsOnCall(i, errDeleteInstanceFail) + } + fakeSpaceClient.GetInstanceReturnsOnCall(4, &fakeInstanceFailed, kNoError) + fakeSpaceClient.GetInstanceReturnsOnCall(5, kNoInstance, kNoError) + fakeSpaceClient.GetInstanceReturnsOnCall(6, kNoInstance, kNoError) + fakeSpaceClient.GetInstanceReturnsOnCall(7, fakeInstanceReady, kNoError) + + // Perform the actual test + recreateFlag := true + infinite := true + srvInstanceCR := createInstanceCR(ctx, testK8sInstNameStateDeleteFailedInfinite, testSpaceNameInstances, infinite, recreateFlag) + finalInstanceCR := waitForInstanceCR(ctx, client.ObjectKeyFromObject(srvInstanceCR)) + + // Verify the instance CR is in a failed state after exceeding retries + Expect(finalInstanceCR.Status.State).To(Equal(v1alpha1.ServiceInstanceStateReady)) + + // Check that CreateInstance was called several times, respecting the max retries limit + Expect(fakeSpaceClient.DeleteInstanceCallCount()).To(Equal(5)) + }) + + It("should try re-create instance for Infinite tries", func() { + // Prepare fake CF responses to simulate instance creation failure on the below CreateInstance calls + fakeInstanceFailed := *fakeInstanceReady + fakeInstanceFailed.State = facade.InstanceStateCreatedFailed + fakeInstanceFailed.StateDescription = string(facade.InstanceStateCreatedFailed) + fakeOrgClient.GetSpaceReturns(&facade.Space{Guid: testCfSpaceGuid}, nil) + fakeSpaceClient.FindServicePlanReturns(testCfPlanGuid, kNoError) + fakeSpaceClient.DeleteInstanceReturns(kNoError) + + // CreateInstance shall always fail directly + fakeSpaceClient.CreateInstanceReturns(errCreateInstanceFail) + + // GetInstance shall return errors except for below cases + fakeSpaceClient.GetInstanceReturns(kNoInstance, errNotExpected) + fakeSpaceClient.GetInstanceReturnsOnCall(0, &fakeInstanceFailed, kNoError) // Instance creation fails + for i := 1; i <= 6; i++ { + fakeSpaceClient.GetInstanceReturnsOnCall(i, kNoInstance, kNoError) + } + + //simulate ready instance to finish the test + fakeSpaceClient.CreateInstanceReturnsOnCall(4, kNoError) + fakeSpaceClient.GetInstanceReturnsOnCall(7, fakeInstanceReady, kNoError) + + // perform actual test + recreateFlag := true + infinite := true + instanceCR := createInstanceCR(ctx, testK8sInstNameRecreateInfinite, testSpaceNameInstances, infinite, recreateFlag) + waitForInstanceCR(ctx, client.ObjectKeyFromObject(instanceCR)) + + // check expectations on reconcile loop + Expect(fakeSpaceClient.DeleteInstanceCallCount()).To(Equal(1)) + Expect(fakeSpaceClient.CreateInstanceCallCount()).To(Equal(5)) + Expect(fakeSpaceClient.GetInstanceCallCount()).To(Equal(8)) + + }) + + }) +}) diff --git a/internal/controllers/space_controller_integration_test.go b/internal/controllers/space_controller_integration_test.go index 9edc1fa..51e1f1d 100644 --- a/internal/controllers/space_controller_integration_test.go +++ b/internal/controllers/space_controller_integration_test.go @@ -6,133 +6,22 @@ package controllers import ( "context" - "fmt" - "path/filepath" - "testing" - "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "github.com/sap/cf-service-operator/api/v1alpha1" "github.com/sap/cf-service-operator/internal/facade" "github.com/sap/cf-service-operator/internal/facade/facadefakes" - "k8s.io/client-go/kubernetes/scheme" - ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/envtest" - logf "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/log/zap" - - corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/rest" - metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" ) +// constants useful for this controller +// Note: +// - if constants are used in multiple controllers, consider moving them to suite_test.go +// - use separete resource names to prevent collisions between tests const ( - testNamespace = "cf-env-test" - testSpace = "test-space" - testSecret = "test-secret" - testOrganization = "test-organization" - testGuid = "test-guid" -) - -// timeout used for waiting on changes of custom resource (overridden by environment variable TEST_TIMEOUT) -var timeout = 3 * time.Second - -// interval used for polling custom resource -var interval = 500 * time.Millisecond - -var ( - k8sConfig *rest.Config - k8sClient client.Client - testCluster *envtest.Environment - k8sManager ctrl.Manager - cancelManager context.CancelFunc - fakeOrgClient *facadefakes.FakeOrganizationClient - fakeSpaceHealthChecker *facadefakes.FakeSpaceHealthChecker + testSpaceName = "test-space" // used for K8s CR and CF space ) -func TestSpaceController(t *testing.T) { - RegisterFailHandler(Fail) - RunSpecs(t, "Space Controller Test Suite") -} - -// ----------------------------------------------------------------------------------------------- -// Suite -// ----------------------------------------------------------------------------------------------- - -var _ = BeforeSuite(func() { - By("bootstrapping test environment") - - var err error - - // enable logging of operator - logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) - - By("spin-up new cluster") - testCluster = &envtest.Environment{ - CRDDirectoryPaths: []string{filepath.Join("..", "..", "crds")}, - } - k8sConfig, err = testCluster.Start() - Expect(err).ToNot(HaveOccurred()) - Expect(k8sConfig).ToNot(BeNil()) - - By("creating K8s client") - Expect(v1alpha1.AddToScheme(scheme.Scheme)).To(Succeed()) - k8sClient, err = client.New(k8sConfig, client.Options{Scheme: scheme.Scheme}) - Expect(err).ToNot(HaveOccurred()) - Expect(k8sClient).ToNot(BeNil()) - - By("creating K8s manager") - k8sManager, err = ctrl.NewManager(k8sConfig, ctrl.Options{ - Scheme: scheme.Scheme, - Metrics: metricsserver.Options{ - BindAddress: "0", - }, - HealthProbeBindAddress: "0", - }) - Expect(err).ToNot(HaveOccurred()) - - By("adding controllers") - fakeOrgClient = &facadefakes.FakeOrganizationClient{} - fakeSpaceHealthChecker = &facadefakes.FakeSpaceHealthChecker{} - spaceReconciler := &SpaceReconciler{ - Kind: "Space", - Client: k8sManager.GetClient(), - Scheme: k8sManager.GetScheme(), - ClientBuilder: func(organizationName string, url string, username string, password string) (facade.OrganizationClient, error) { - return fakeOrgClient, nil - }, - HealthCheckerBuilder: func(spaceGuid string, url string, username string, password string) (facade.SpaceHealthChecker, error) { - return fakeSpaceHealthChecker, nil - }, - // see main.go for more parameters - } - Expect(spaceReconciler.SetupWithManager(k8sManager)).To(Succeed()) - - By("starting manager") - ctx, cancel := context.WithCancel(context.Background()) - cancelManager = cancel - go func() { - defer GinkgoRecover() - Expect(k8sManager.Start(ctx)).To(Succeed()) - }() -}) - -// ----------------------------------------------------------------------------------------------- - -var _ = AfterSuite(func() { - By("tearing down the test environment") - if cancelManager != nil { - cancelManager() - } - if testCluster != nil { - Expect(testCluster.Stop()).To(Succeed()) - } -}) - // ----------------------------------------------------------------------------------------------- // Tests // ----------------------------------------------------------------------------------------------- @@ -142,95 +31,36 @@ var _ = Describe("Space Controller Integration Tests", func() { ctx := context.Background() BeforeEach(func() { - Expect(k8sClient).ToNot(BeNil()) - - By("creating namespace") - namespace := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: testNamespace, - }, - } - err := k8sClient.Create(ctx, namespace) - if err != nil && !apierrors.IsAlreadyExists(err) { - Fail(err.Error()) - Fail("failed to create namespace") - } - - // Create secret (that will be read during reconcile) - secret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: testSecret, - Namespace: testNamespace, - }, - Data: map[string][]byte{ - "username": []byte("test-username"), - "password": []byte("test-password"), - "url": []byte("https://api.cf.sap.hana.ondemand.com"), - }, - } - Expect(k8sClient.Create(ctx, secret)).To(Succeed()) + // reset all fake clients to always start with clean state (e.g. call counts of zero) + fakeOrgClient = &facadefakes.FakeOrganizationClient{} + fakeSpaceClient = &facadefakes.FakeSpaceClient{} + fakeSpaceHealthChecker = &facadefakes.FakeSpaceHealthChecker{} }) It("should create space", func() { - By("creating space CR") - var dummySpace = &facade.Space{Guid: testGuid, Name: testSpace, Owner: "me", Generation: 1} - fakeOrgClient.CreateSpaceReturns(nil) - fakeOrgClient.GetSpaceReturns(dummySpace, nil) - fakeOrgClient.GetSpaceReturnsOnCall(0, nil, nil) // only the 1st call returns no space, that controller will create the space. - fakeSpaceHealthChecker.CheckReturns(nil) - var sleep = 0 - // sleep = 500 // with Debugger - if sleep > 0 { - createSpaceCR(ctx) - time.Sleep(time.Duration(sleep) * time.Second) - } else { - spaceCR := createSpaceCR(ctx) - waitForSpaceCR(ctx, client.ObjectKeyFromObject(spaceCR)) + // prepare fake CF responses + fakeSpace := &facade.Space{ + Guid: testCfSpaceGuid, + Name: testSpaceName, + Owner: testCfOwner, + Generation: 1, } + fakeOrgClient.CreateSpaceReturns(kNoError) + + // 0) GetSpace is called before CreateSpace to check existence => simulate non-existing space + // 1) GetSpace is called after CreateSpace to check current state => simulate ready space + fakeOrgClient.GetSpaceReturnsOnCall(0, kNoSpace, kNoError) + fakeOrgClient.GetSpaceReturns(fakeSpace, kNoError) + + // perform the actual test + spaceCR := createSpaceCR(ctx, testSpaceName) + waitForSpaceCR(ctx, client.ObjectKeyFromObject(spaceCR)) + + // check expectations on reconcile loop Expect(fakeOrgClient.CreateSpaceCallCount()).To(Equal(1)) + Expect(fakeOrgClient.GetSpaceCallCount()).To(BeNumerically(">=", 4)) + // TODO: check if number of calls to GetSpace can be reduced }) }) }) - -// ----------------------------------------------------------------------------------------------- -// Helper Functions -// ----------------------------------------------------------------------------------------------- - -func createSpaceCR(ctx context.Context) *v1alpha1.Space { - spaceCR := &v1alpha1.Space{ - ObjectMeta: metav1.ObjectMeta{ - Name: testSpace, - Namespace: testNamespace, - }, - Spec: v1alpha1.SpaceSpec{ - AuthSecretName: testSecret, - OrganizationName: testOrganization, - }, - } - Expect(k8sClient.Create(ctx, spaceCR)).To(Succeed()) - - return spaceCR -} - -// ----------------------------------------------------------------------------------------------- - -func waitForSpaceCR(ctx context.Context, spaceKey client.ObjectKey) *v1alpha1.Space { - spaceCR := &v1alpha1.Space{} - expState := v1alpha1.SpaceStateReady - - Eventually(func() error { - By("waiting for status 'Ready' of space CR") - err := k8sClient.Get(ctx, spaceKey, spaceCR) - fmt.Println("*** Status is ", spaceCR.Status.State) - if err != nil { - return err - } - if spaceCR.Status.State != expState { - return fmt.Errorf("expected state %s but got %s", expState, spaceCR.Status.State) - } - return nil // success - }, timeout, interval).Should(Succeed(), "space CR should have been started") - - return spaceCR -} diff --git a/internal/controllers/suite_test.go b/internal/controllers/suite_test.go new file mode 100644 index 0000000..f6895ec --- /dev/null +++ b/internal/controllers/suite_test.go @@ -0,0 +1,342 @@ +/* +SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and cf-service-operator contributors +SPDX-License-Identifier: Apache-2.0 +*/ +package controllers + +import ( + "context" + "fmt" + "os" + "path/filepath" + "strconv" + "testing" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/sap/cf-service-operator/api/v1alpha1" + "github.com/sap/cf-service-operator/internal/facade" + "github.com/sap/cf-service-operator/internal/facade/facadefakes" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" +) + +var kNoSpace *facade.Space = nil +var kNoInstance *facade.Instance = nil + +var kNoError error = nil +var errNotExpected = fmt.Errorf("not expected") +var errCreateInstanceFail = fmt.Errorf("create instance failed") +var errDeleteInstanceFail = fmt.Errorf("delete instance failed") + +// constants useful for all tests +const ( + testK8sNamespace = "test-namespace" + testK8sSecretName = "test-secret" + testCfOrgName = "test-organization" + testCfSpaceGuid = "test-space-guid" + testCfPlanGuid = "test-plan-guid" + testCfOwner = "test-owner" + + // credentials for CF client + testCfUsername = "test-username" + testCfPassword = "test-password" + testCfUrl = "https://api.cf.example.com" + + testServiceInstanceDefaultReconcileInterval = 1 * time.Second + testServiceInstanceDefaultMaxRetries int = 5 + testServiceInstanceDefaultRetryInterval = 1 * time.Second + testServiceInstanceDefaultMaxRetryInterval = 1 * time.Minute +) + +// timeout used for waiting on changes of custom resource +// (overridden by environment variable TEST_TIMEOUT) +var timeout = 5 * time.Minute + +// interval used for polling custom resource +var interval = 500 * time.Millisecond + +var ( + k8sConfig *rest.Config + k8sClient client.Client + testCluster *envtest.Environment + cancelManager context.CancelFunc + fakeOrgClient *facadefakes.FakeOrganizationClient + fakeSpaceClient *facadefakes.FakeSpaceClient + fakeSpaceHealthChecker *facadefakes.FakeSpaceHealthChecker +) + +// ----------------------------------------------------------------------------------------------- + +func TestController(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Controller Test Suite") +} + +// ----------------------------------------------------------------------------------------------- + +var _ = BeforeSuite(func() { + // enable logging of operator + logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) + + // for debugging of integration tests, allow setting of timeout by environment variable + if testTimeout, err := strconv.Atoi(os.Getenv("TEST_TIMEOUT")); err == nil { + timeout = time.Duration(testTimeout) * time.Second + } + + prepareKubernetesEnvironment() + prepareOperatorResources() + + // create operator with some of the controllers required for testing + // the "real" operator is constructed in main.go + // disabling metrics server and health probe (not required for tests, but if activate might + // cause problems with already used ports in the GitHub action runner) + + By("creating K8s manager") + k8sManager, err := ctrl.NewManager(k8sConfig, ctrl.Options{ + Scheme: scheme.Scheme, + Metrics: metricsserver.Options{ + BindAddress: "0", + }, + HealthProbeBindAddress: "0", + }) + Expect(err).ToNot(HaveOccurred()) + + addControllers(k8sManager) + + By("starting K8s manager") + ctx, cancel := context.WithCancel(context.Background()) + cancelManager = cancel + go func() { + defer GinkgoRecover() + Expect(k8sManager.Start(ctx)).To(Succeed()) + }() +}) + +// ----------------------------------------------------------------------------------------------- + +var _ = AfterSuite(func() { + By("tearing down the test environment") + if cancelManager != nil { + cancelManager() + } + if testCluster != nil { + Expect(testCluster.Stop()).To(Succeed()) + } +}) + +// ----------------------------------------------------------------------------------------------- +// Helper Functions +// ----------------------------------------------------------------------------------------------- + +// prepare test K8s cluster and K8s client +func prepareKubernetesEnvironment() { + var err error + + By("spinning up new K8s cluster") + testCluster = &envtest.Environment{ + CRDDirectoryPaths: []string{filepath.Join("..", "..", "crds")}, + } + k8sConfig, err = testCluster.Start() + Expect(err).ToNot(HaveOccurred()) + Expect(k8sConfig).ToNot(BeNil()) + + By("creating K8s client") + Expect(v1alpha1.AddToScheme(scheme.Scheme)).To(Succeed()) + k8sClient, err = client.New(k8sConfig, client.Options{Scheme: scheme.Scheme}) + Expect(err).ToNot(HaveOccurred()) + Expect(k8sClient).ToNot(BeNil()) +} + +// ----------------------------------------------------------------------------------------------- + +// prepare K8s resources for cf-service-operator +func prepareOperatorResources() { + var err error + + By("creating K8s namespace") + namespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: testK8sNamespace, + }, + } + ctx := context.Background() + err = k8sClient.Create(ctx, namespace) + if err != nil && !apierrors.IsAlreadyExists(err) { + Fail(err.Error()) + Fail("failed to create K8s namespace") + } + + // Create secret (that will be read during reconcile) + By("creating K8s secret") + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: testK8sSecretName, + Namespace: testK8sNamespace, + }, + Data: map[string][]byte{ + "username": []byte(testCfUsername), + "password": []byte(testCfPassword), + "url": []byte(testCfUrl), + }, + } + Expect(k8sClient.Create(ctx, secret)).To(Succeed()) +} + +// ----------------------------------------------------------------------------------------------- + +func addControllers(k8sManager ctrl.Manager) { + // add space controller + spaceReconciler := &SpaceReconciler{ + Kind: "Space", + Client: k8sManager.GetClient(), + Scheme: k8sManager.GetScheme(), + ClusterResourceNamespace: testK8sNamespace, + ClientBuilder: func(organizationName string, url string, username string, password string) (facade.OrganizationClient, error) { + return fakeOrgClient, nil + }, + HealthCheckerBuilder: func(spaceGuid string, url string, username string, password string) (facade.SpaceHealthChecker, error) { + return fakeSpaceHealthChecker, nil + }, + } + Expect(spaceReconciler.SetupWithManager(k8sManager)).To(Succeed()) + + // add service instance controller + instanceReconciler := &ServiceInstanceReconciler{ + Client: k8sManager.GetClient(), + Scheme: k8sManager.GetScheme(), + ClusterResourceNamespace: testK8sNamespace, + ClientBuilder: func(organizationName string, url string, username string, password string) (facade.SpaceClient, error) { + return fakeSpaceClient, nil + }, + } + Expect(instanceReconciler.SetupWithManager(k8sManager)).To(Succeed()) + + // TODO: add service binding controller if required for tests + // TODO: add another space controller for ClusterSpace resources if required for tests +} + +// ----------------------------------------------------------------------------------------------- + +func createSpaceCR(ctx context.Context, spaceName string) *v1alpha1.Space { + spaceCR := &v1alpha1.Space{ + ObjectMeta: metav1.ObjectMeta{ + Name: spaceName, + Namespace: testK8sNamespace, + }, + Spec: v1alpha1.SpaceSpec{ + AuthSecretName: testK8sSecretName, + OrganizationName: testCfOrgName, + }, + } + Expect(k8sClient.Create(ctx, spaceCR)).To(Succeed()) + + return spaceCR +} + +// ----------------------------------------------------------------------------------------------- + +func waitForSpaceCR(ctx context.Context, spaceKey client.ObjectKey) *v1alpha1.Space { + spaceCR := &v1alpha1.Space{} + expState := v1alpha1.SpaceStateReady + + Eventually(func() error { + By(fmt.Sprintf("waiting for state '%s' of space CR", expState)) + err := k8sClient.Get(ctx, spaceKey, spaceCR) + fmt.Println("*** State is ", spaceCR.Status.State) + if err != nil { + return err + } + if spaceCR.Status.State != expState { + return fmt.Errorf("expected state '%s' but got '%s'", expState, spaceCR.Status.State) + } + return kNoError + }, timeout, interval).Should(Succeed(), "space CR should have been started") + + return spaceCR +} + +// ----------------------------------------------------------------------------------------------- + +func createInstanceCR(ctx context.Context, instanceName, spaceName string, infinite bool, recreate ...bool) *v1alpha1.ServiceInstance { + instanceCR := &v1alpha1.ServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: instanceName, + Namespace: testK8sNamespace, + }, + Spec: v1alpha1.ServiceInstanceSpec{ + Name: instanceName, + ServiceOfferingName: "test-service", + ServicePlanName: "test-plan", + SpaceName: spaceName, + }, + } + + if len(recreate) > 0 && recreate[0] { + annotations := make(map[string]string) + annotations[v1alpha1.AnnotationRecreate] = "true" + if !infinite { + annotations[v1alpha1.AnnotationMaxRetries] = fmt.Sprint(testServiceInstanceDefaultMaxRetries) + } + annotations[v1alpha1.AnnotationReconcileTimeout] = testServiceInstanceDefaultReconcileInterval.String() + instanceCR.SetAnnotations(annotations) + } + + Expect(k8sClient.Create(ctx, instanceCR)).To(Succeed()) + + return instanceCR +} + +// ----------------------------------------------------------------------------------------------- + +func waitForInstanceCR(ctx context.Context, instanceKey client.ObjectKey) *v1alpha1.ServiceInstance { + instanceCR := &v1alpha1.ServiceInstance{} + expState := v1alpha1.ServiceInstanceStateReady + + Eventually(func() error { + By(fmt.Sprintf("waiting for state '%s' of instance CR", expState)) + err := k8sClient.Get(ctx, instanceKey, instanceCR) + fmt.Println("*** State is ", instanceCR.Status.State) + if err != nil { + return err + } + if instanceCR.Status.State != expState { + return fmt.Errorf("expected state %s but got %s", expState, instanceCR.Status.State) + } + return kNoError + }, timeout, interval).Should(Succeed(), "instance CR should have been started") + + return instanceCR +} + +func waitForInstanceCRToFail(ctx context.Context, instanceKey client.ObjectKey) *v1alpha1.ServiceInstance { + instanceCR := &v1alpha1.ServiceInstance{} + expState := v1alpha1.ServiceInstanceStateError + + Eventually(func() error { + By(fmt.Sprintf("waiting for state '%s' of instance CR", expState)) + err := k8sClient.Get(ctx, instanceKey, instanceCR) + fmt.Println("*** State is ", instanceCR.Status.State) + if err != nil { + return err + } + if instanceCR.Status.State != expState { + return kNoError + } + return fmt.Errorf("expected state %s but got %s", expState, instanceCR.Status.State) + }, timeout, interval).ShouldNot(Succeed(), "instance CR should be in state Error") + + return instanceCR +} diff --git a/website/content/en/docs/usage/_index.md b/website/content/en/docs/usage/_index.md index d8fe8a6..7fa7e48 100644 --- a/website/content/en/docs/usage/_index.md +++ b/website/content/en/docs/usage/_index.md @@ -18,4 +18,3 @@ cf-service-operator introduces the following resource types: * [ServiceBinding](./servicebinding): used to manage (create/update) a Cloud Foundry service binding. A ServiceBinding references a ServiceInstance Object, and defines the Kubernetes secret used to store the retrieved service key. Optionally binding parameters can be specified. - diff --git a/website/content/en/docs/usage/serviceinstance.md b/website/content/en/docs/usage/serviceinstance.md index 9af4288..befebdf 100644 --- a/website/content/en/docs/usage/serviceinstance.md +++ b/website/content/en/docs/usage/serviceinstance.md @@ -7,10 +7,10 @@ description: > Manage Cloud Foundry service instances --- -Objects of type `serviceinstances.cf.cs.sap.com` represent Cloud Foundry service instances. For example, -deploying the following descriptor will let the controller create or update a Cloud Foundry instance -of the service offering 'xsuaa' with plan 'application', -in the Cloud Foundry space referenced through the Space object given in `spec.spaceName`: +Objects of type `serviceinstances.cf.cs.sap.com` represent Cloud Foundry service instances. For +example, deploying the following descriptor will let the controller create or update a Cloud Foundry +instance of the service offering 'xsuaa' with plan 'application', in the Cloud Foundry space +referenced through the Space object given in `spec.spaceName`: ```yaml apiVersion: cf.cs.sap.com/v1alpha1 @@ -46,8 +46,8 @@ spec: servicePlanName: application ``` -Furthermore, instead of specifying service offering and plan by name, it is possible to directly provide -the guid of the service plan, such as: +Furthermore, instead of specifying service offering and plan by name, it is possible to directly +provide the guid of the service plan, such as: ```yaml apiVersion: cf.cs.sap.com/v1alpha1 @@ -89,8 +89,9 @@ spec: key: parameters.json ``` -Following the logic implemented by similar controllers (e.g. the K8s service catalog) it is allowed to specify both `parameters` and `parametersFrom`, -but it is considered an error if a top level key occurs in more than one of the sources. +Following the logic implemented by similar controllers (e.g. the K8s service catalog) it is allowed +to specify both `parameters` and `parametersFrom`, but it is considered an error if a top level key +occurs in more than one of the sources. In addition, it is possible to annotate custom instance tags, such as: @@ -115,8 +116,67 @@ spec: - authentication ``` -Finally there is a way to tweak the controller's behavior in case the creation of a service instance fails in Cloud Foundry. -By default, in all kinds of error situations, the controller will send an update request, in order to trigger a reconciliation of the instance. -However some service brokers do not really support this approach, specifically when the initial creation of the instance has failed. -To overcome this, the annotation `service-operator.cf.cs.sap.com/recreate-on-creation-failure: "true"` may be set on the service instance object. -If present, the controller will drop and recreate instances which are in a failed creation state (which precisely means that the `LastOperation` reported by the Cloud Foundry API is of type `create` and state `failed`). \ No newline at end of file +## Annotations + +Kubernetes annotations provide a flexible way of controlling the behavior of the reconciliation +process for custom resources. + +The `cf-service-operator` uses several such annotations for tweaking the behavior of the +reconciliation for service instances. + +1. `service-operator.cf.cs.sap.com/recreate-on-creation-failure`: + By default, in all kinds of error situations, the controller will send an update request, in + order to trigger a reconciliation of the instance. However some service brokers do not really + support this approach, specifically when the initial creation of the instance has failed. + To overcome this, this annotation can be set on the service instance object. If present, the + controller will drop and recreate instances which are in a failed creation statem, i.e. + the `LastOperation` reported by the Cloud Foundry API is of type `create` and state `failed`. + +2. `service-operator.cf.cs.sap.com/max-retries`: + This annotation defines the maximum number of retries for a failed operation before considering + the operation as failed permanently. It allows other operators using the custom resources for + service instances to specify how many times the controller should attempt to reconcile the + specific service instance before giving up, providing a mechanism to handle transient errors. + **If this annotations is not set the number of retries is unlimited.** + +3. `service-operator.cf.cs.sap.com/timeout-on-reconcile`: + Specifies the timeout for the reconciliation process. If set, this annotation determines how + long the controller should wait before timing out the reconciliation process. This is useful for + operations that are expected to take longer than usual, allowing them to complete without + prematurely terminating. + +### How to use these annotations + +Here are examples on how these annotations are set in the metadata section of the `ServiceInstance` +custom resource: + +```yaml +apiVersion: cf.cs.sap.com/v1alpha1 +kind: ServiceInstance +metadata: + name: example-instance + annotations: + service-operator.cf.cs.sap.com/recreate-on-creation-failure: "true" + service-operator.cf.cs.sap.com/max-retries: "3" + service-operator.cf.cs.sap.com/timeout-on-reconcile: "10m" +spec: + spaceName: development + serviceOfferingName: my-service + servicePlanName: standard +``` + +In this example, the service instance `example-instance` is configured to automatically recreate on +initial creation failure, retry up to three times on subsequent failures, and has a timeout of ten +minutes for each reconciliation attempt. + +### Setting Annotations in Kubernetes + +To set these annotations, you can either add them directly in the YAML file when creating or +updating a service instance or use the `kubectl` command line tool to patch an existing instance: + +```bash +kubectl annotate serviceinstances example-instance service-operator.cf.cs.sap.com/max-retries=5 --overwrite +``` + +This command will set (or update) the `max-retries` annotation to 5 for the `example-instance` +service instance.