Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PWX-34118: Skip service accounts, roles and rolebindings as part of pruning of resources with ownerreferences. #1560

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

diptiranjanpx
Copy link
Contributor

@diptiranjanpx diptiranjanpx commented Nov 9, 2023

Signed-Off-By: Diptiranjan

What type of PR is this?
improvement

What this PR does / why we need it:
Some of the operators like postgres were not able to reconcile on the CR and create the pods if service account, roles and rolebindings were already created.

Does this PR change a user-facing CRD or CLI?:

Is a release note needed?:

Issue: Postgres operator throws error of serviceaccount, role and rolebindings already exist after migration.
User Impact: Unable to scale up postgres app installed via openshift operator hub after migration.
Resolution: Postgres pods came up fine after skipping the resources serviceaccount, role and rolebindings as part of resource migration if they have ownerreference set.

Does this change need to be cherry-picked to a release branch?:

23.11.0

Test
Tested with crunchydata postgres operator 5.4.3 and details have been updated in the ticket.

@cnbu-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

Copy link
Contributor

@pp511 pp511 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks ok

Copy link

codecov bot commented Nov 10, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8f9c682) 66.47% compared to head (58451d2) 66.53%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1560      +/-   ##
==========================================
+ Coverage   66.47%   66.53%   +0.05%     
==========================================
  Files          43       43              
  Lines        5453     5453              
==========================================
+ Hits         3625     3628       +3     
+ Misses       1493     1491       -2     
+ Partials      335      334       -1     

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pp511 pp511 self-requested a review November 10, 2023 23:48
Comment on lines 755 to +760
objectType.GetKind() != "StatefulSet" &&
objectType.GetKind() != "ReplicaSet" &&
objectType.GetKind() != "DeploymentConfig" &&
objectType.GetKind() != "ServiceAccount" &&
objectType.GetKind() != "Role" &&
objectType.GetKind() != "RoleBinding" &&
Copy link
Contributor

@pp511 pp511 Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do all these checks have to be in the for loop? We should check it outside of for _, owner := range owners loop. Am I missing anything?
I think we can update the logic here to make it cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a check for if a pod is an owner is there before we are checking for if object is not one among these types (deploy, sts, service etc...). If we take the decision of collecting the object before the for loop, there can be scenario the owner might be a pod itself and the real intention was to skip it.

Anyway we will get rid of all these checks and make it generic for all resources not to be collected or something similar. So don't want to alter the logic now.

@diptiranjanpx diptiranjanpx merged commit e1d23e7 into master Nov 16, 2023
7 checks passed
@diptiranjanpx diptiranjanpx deleted the PWX-34118 branch May 27, 2024 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants