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

Clean up degenerate deployments #3538

Merged

Conversation

WeiQuan0605
Copy link
Contributor

@WeiQuan0605 WeiQuan0605 commented Dec 5, 2023

With the fix, job finalized-degenerate-deployment only considers active deployments.status.reason will not have DEGENERATEanymore. The old DEGENERATE records should be cleaned up through DB migration.

  • I have reviewed the contributing guide

  • I have viewed, signed, and submitted the Contributor License Agreement

  • I have made this pull request to the main branch

  • I have run all the unit tests using bundle exec rake

  • I have run CF Acceptance Tests

@svkrieger svkrieger self-requested a review December 14, 2023 10:24
Copy link
Contributor

@svkrieger svkrieger left a comment

Choose a reason for hiding this comment

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

As far as I could see the DEGENERATE state will not appear anymore.
Actually it should have never been introduced, so it's good that we clean it up now.
Initially the DEGENERATE state was meant for deployments, which were "half way created" because the process was not created in one transaction together with the deployment. But this has been fixed already.
So apps using deployments, be it a new app or a running app, will always have a process guid already, which makes this state obsolete.

A small test for the migration, which creates a few deployments with state DEGENERATE and checks that the migration deletes them would be nice, because we get early feedback whether the migration runs on all supported DBs.

Copy link
Contributor

@svkrieger svkrieger left a comment

Choose a reason for hiding this comment

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

LGTM

@svkrieger svkrieger merged commit ba4cf06 into cloudfoundry:main Dec 18, 2023
9 checks passed
@svkrieger svkrieger deleted the clean-up-degenerate-deployments branch December 18, 2023 10:35
@ChrisMcGowan
Copy link

As far as I could see the DEGENERATE state will not appear anymore. Actually it should have never been introduced, so it's good that we clean it up now. Initially the DEGENERATE state was meant for deployments, which were "half way created" because the process was not created in one transaction together with the deployment. But this has been fixed already. So apps using deployments, be it a new app or a running app, will always have a process guid already, which makes this state obsolete.

A small test for the migration, which creates a few deployments with state DEGENERATE and checks that the migration deletes them would be nice, because we get early feedback whether the migration runs on all supported DBs.

@svkrieger & @WeiQuan0605 - curious on your testing if you ran into this. In prod today we migrated from cf-deployment v35.3 to v35.4 which moved CAPI from 0.167.0 to 0.168.0 which has this change. We have not filed a Issue yet but probably will soon.

The migration script when it fires runs into the following error:

[2024-01-09 18:20:21+0000] Sequel::ForeignKeyConstraintViolation: PG::ForeignKeyViolation: ERROR:  update or delete on table "deployments" violates foreign key constraint "fk_deployment_processes_deployment_guid" on table "deployment_processes" (Sequel::ForeignKeyConstraintViolation)
[2024-01-09 18:20:21+0000] DETAIL:  Key (guid)=(<app guid scrubbed>) is still referenced from table "deployment_processes".

We have over 31K rows in deployments and by count over 7,500 rows marked for delete with this migration script. Going back in this repo we found the initial framework for this in this migration script from here back in 2018.

This appears to be deleting the parent record but not the child records since a "cascade" delete is not taking place.

Can you advise on your testing why you did not get the errors ?

For reference we are using Postgres 12.14 on Amazon RDS

Thanks

@johha
Copy link
Contributor

johha commented Jan 10, 2024

@ChrisMcGowan Looks like something we missed 😞
I created #3592 and will adjust the release notes accordingly

@ChrisMcGowan
Copy link

Thank you @johha

@WeiQuan0605 WeiQuan0605 restored the clean-up-degenerate-deployments branch January 12, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants