MGDAPI-4472 CRO can reconnect in the case of a postgres cr recreation #513

valerymo · 2022-09-19T08:04:11Z

Overview

Jira: https://issues.redhat.com/browse/MGDAPI-4472

WHAT

If a postgres is removed from the cluster in a case where the RDS is still in place, recreation of a postgres cr could reconnect with the RDS and CRO puts back in place the requried connection details into the cluster for applications to reconnect.

SCENARIO

Install CRO, install a Postgres cr, Delete the namespace which removes CRO and the postgres but not the DB because the CRO controller wont have time to cleanup.

Reinstall CRO and the Postgres cr. Does it put back the connection secret etc. If not update the logic in CRO to cover this.

Verification

Clone this branch
Run make cluster/prepare
Run make run
Follow as in Scenario

Checklist

Please see Scenario - CRO recovers Postgres CR after deletion

codecov · 2022-09-19T09:05:42Z

Codecov Report

Merging #513 (134cc55) into master (4e1bd20) will increase coverage by 0.02%.
The diff coverage is 69.23%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #513      +/-   ##
==========================================
+ Coverage   68.44%   68.46%   +0.02%     
==========================================
  Files          38       38              
  Lines        5099     5109      +10     
==========================================
+ Hits         3490     3498       +8     
- Misses       1254     1255       +1     
- Partials      355      356       +1

Impacted Files	Coverage Δ
pkg/providers/aws/provider_postgres.go	`63.51% <69.23%> (+0.22%)`	⬆️

cecobask · 2022-09-29T10:53:30Z

pkg/providers/aws/provider_postgres.go

@@ -379,6 +379,14 @@ func (p *PostgresProvider) reconcileRDSInstance(ctx context.Context, cr *v1alpha
 			Database: *foundInstance.DBName,
 			Port:     int(*foundInstance.Endpoint.Port),
 		}
+
+		if !annotations.Has(cr, ResourceIdentifierAnnotation) {


I believe this path isn't covered by unit tests. Would you be able to add a test case for it, if possible?

@cecobask Thank you for comment and discussion.
Updated and retested. Thanks

cecobask · 2022-09-29T13:38:33Z

Verification:

Created an AWS CCS cluster
Ran CRO locally
Created a Postgres CR and waited for completion
Deleted the CRO namespace - had to manually remove the cloud-resources-operator.integreatly.org/finalizers finalizer from the Postgres CR in order to proceed
Ran CRO locally again
Created another Postgres CR with the same spec as earlier
Confirmed that CRO reconnected to the existing RDS instance and had the secret reference in place

@valerymo I'm ready to place an lgtm but I'll wait until the unit test case for the missing coverage is added 👍🏻

cecobask · 2022-10-06T08:05:16Z

Thank you for adding the test cases, @valerymo!
/lgtm

valerymo · 2022-10-18T05:03:08Z

/test codecov/patch

openshift-ci · 2022-10-18T05:03:11Z

@valerymo: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test coverage
/test e2e
/test format
/test gosec
/test images
/test unit
/test vendor

Use /test all to run all jobs.

In response to this:

/test codecov/patch

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

valerymo · 2022-10-18T05:03:30Z

/test coverage

cecobask · 2022-10-18T09:28:13Z

/test all

cecobask

/lgtm

laurafitzgerald · 2022-10-20T11:54:53Z

Makefile

@@ -128,7 +128,7 @@ cluster/seed/managed/postgressnapshot:

 .PHONY: cluster/clean
 cluster/clean:
-	@$(KUSTOMIZE) build config/crd | kubectl delete -f -
+	@$(KUSTOMIZE) build config/crd | oc delete -f -


any reason to use oc over kubectl here?

@laurafitzgerald I didn't have kubectl locally : ), and i found that it was the only place where oc was not used but kubectl, so I changed it to oc, as in other places in Makefile. Thank you for comment

sorry, ignore please my reply, regarding rebase & missing kubectl. I looked at not correct Makefile.
So answer - I didn't have kubectl locally : ), and this is the only place where kubectl was used, so didn't see any reason to use it

ok. no problem. I think we had agreed a long time ago that we would use kubectl in all cases where possible and only oc where we had to but looks like it's used elsewhere so I think this is fine.

laurafitzgerald · 2022-10-20T11:57:49Z

pkg/providers/aws/provider_postgres.go

+	if err := client.Update(ctx, cr); err != nil {
+		return "failed to add annotation", err
+	}
+	return "", nil


can we use cro.StatusEmpty here.

Thank you, I will check

cecobask · 2022-10-25T08:57:01Z

/lgtm

laurafitzgerald · 2022-10-25T11:27:05Z

/approve

openshift-ci · 2022-10-25T11:27:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cecobask, laurafitzgerald

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [laurafitzgerald]

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

valerymo force-pushed the valery_MGDAPI-4472 branch from 0bfdb14 to 1151c4d Compare September 21, 2022 08:13

cecobask reviewed Sep 29, 2022

View reviewed changes

valerymo force-pushed the valery_MGDAPI-4472 branch 2 times, most recently from a1532b1 to 7425bd1 Compare October 2, 2022 16:21

openshift-ci bot assigned cecobask Oct 6, 2022

openshift-ci bot added the lgtm label Oct 6, 2022

valerymo force-pushed the valery_MGDAPI-4472 branch from 7425bd1 to dd2b82f Compare October 18, 2022 05:09

openshift-ci bot removed the lgtm label Oct 18, 2022

cecobask approved these changes Oct 18, 2022

View reviewed changes

openshift-ci bot added the lgtm label Oct 18, 2022

laurafitzgerald reviewed Oct 20, 2022

View reviewed changes

valerymo force-pushed the valery_MGDAPI-4472 branch from dd2b82f to 2758271 Compare October 24, 2022 12:41

openshift-ci bot removed the lgtm label Oct 24, 2022

MGDAPI-4472 CRO can reconnect in the case of a postgres cr recreation

134cc55

valerymo force-pushed the valery_MGDAPI-4472 branch from 2758271 to 134cc55 Compare October 24, 2022 13:10

openshift-ci bot added the lgtm label Oct 25, 2022

openshift-ci bot added the approved label Oct 25, 2022

openshift-merge-robot merged commit eefff91 into integr8ly:master Oct 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MGDAPI-4472 CRO can reconnect in the case of a postgres cr recreation #513

MGDAPI-4472 CRO can reconnect in the case of a postgres cr recreation #513

valerymo commented Sep 19, 2022 •

edited

Loading

codecov bot commented Sep 19, 2022 •

edited

Loading

cecobask Sep 29, 2022

valerymo Oct 2, 2022 •

edited

Loading

cecobask commented Sep 29, 2022

cecobask commented Oct 6, 2022

valerymo commented Oct 18, 2022

openshift-ci bot commented Oct 18, 2022

valerymo commented Oct 18, 2022

cecobask commented Oct 18, 2022

cecobask left a comment

laurafitzgerald Oct 20, 2022

valerymo Oct 20, 2022 •

edited

Loading

valerymo Oct 20, 2022

laurafitzgerald Oct 20, 2022

laurafitzgerald Oct 20, 2022

valerymo Oct 20, 2022

cecobask commented Oct 25, 2022

laurafitzgerald commented Oct 25, 2022

openshift-ci bot commented Oct 25, 2022

MGDAPI-4472 CRO can reconnect in the case of a postgres cr recreation #513

MGDAPI-4472 CRO can reconnect in the case of a postgres cr recreation #513

Conversation

valerymo commented Sep 19, 2022 • edited Loading

Overview

WHAT

SCENARIO

Verification

Checklist

codecov bot commented Sep 19, 2022 • edited Loading

Codecov Report

cecobask Sep 29, 2022

Choose a reason for hiding this comment

valerymo Oct 2, 2022 • edited Loading

Choose a reason for hiding this comment

cecobask commented Sep 29, 2022

cecobask commented Oct 6, 2022

valerymo commented Oct 18, 2022

openshift-ci bot commented Oct 18, 2022

valerymo commented Oct 18, 2022

cecobask commented Oct 18, 2022

cecobask left a comment

Choose a reason for hiding this comment

laurafitzgerald Oct 20, 2022

Choose a reason for hiding this comment

valerymo Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

valerymo Oct 20, 2022

Choose a reason for hiding this comment

laurafitzgerald Oct 20, 2022

Choose a reason for hiding this comment

laurafitzgerald Oct 20, 2022

Choose a reason for hiding this comment

valerymo Oct 20, 2022

Choose a reason for hiding this comment

cecobask commented Oct 25, 2022

laurafitzgerald commented Oct 25, 2022

openshift-ci bot commented Oct 25, 2022

valerymo commented Sep 19, 2022 •

edited

Loading

codecov bot commented Sep 19, 2022 •

edited

Loading

valerymo Oct 2, 2022 •

edited

Loading

valerymo Oct 20, 2022 •

edited

Loading