Skip to content
This repository has been archived by the owner on Aug 19, 2024. It is now read-only.

fix: Make sure to use backward-compatible names for resources created by the operator [RHIDP-2432] #369

Merged
merged 8 commits into from
Jun 4, 2024
Merged

fix: Make sure to use backward-compatible names for resources created by the operator [RHIDP-2432] #369

merged 8 commits into from
Jun 4, 2024

Conversation

rm3l
Copy link
Member

@rm3l rm3l commented May 29, 2024 •
edited
Loading

Description

This fixes a critical issue where, when upgrading existing resources using an operator upgraded from 1.1 to 1.2 (main branch), the operator would create and use a brand new database StatefulSet. This would cause the upgraded Backstage application to boot using an empty database, rather than starting with the existing database (which could already contain some data).

It also fixes a more general issue where resource naming was not backward-compatible, which would create for example 2 different ConfigMaps after an upgrade: one named backstage-dynamic-plugins-<cr> (created by the operator on the main branch) besides an already existing one named <cr>-dynamic-plugins (created in 1.1), which could create some confusion when looking at the resources in the namespace.

Which issue(s) does this PR fix or relate to

PR acceptance criteria

  • Tests
  • Documentation
  • If the bundle manifests have been updated, make sure to review the rhdh-operator.csv.yaml file accordingly

How to test changes / Special notes to the reviewer

See the repro steps depicted in https://issues.redhat.com/browse/RHIDP-2432 and run the tests with the following command:

make test
make integration-test \
  ARGS=-focus='creates runtime object using raw configuration then updates StatefulSet to replace some immutable fields' \
  USE_EXISTING_CLUSTER=true

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress PR should not merge because it is a draft or work in progress. Required by Prow. label May 29, 2024
@github-actions GitHub Actions
Copy link
Contributor

PR images are available:

  1. https://quay.io/janus-idp/operator:0.2.0-pr-369-6cb5780
  2. https://quay.io/janus-idp/operator-bundle:0.2.0-pr-369-6cb5780
  3. https://quay.io/janus-idp/operator-catalog:0.2.0-pr-369-6cb5780

@rm3l rm3l changed the title [WIP] Make sure to use backward-compatible names for resources created by the operator Make sure to use backward-compatible names for resources created by the operator May 29, 2024
@rm3l rm3l changed the title Make sure to use backward-compatible names for resources created by the operator fix: Make sure to use backward-compatible names for resources created by the operator [RHIDP-2432] May 29, 2024
@rm3l rm3l marked this pull request as ready for review May 29, 2024 16:37
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress PR should not merge because it is a draft or work in progress. Required by Prow. label May 29, 2024
@openshift-ci openshift-ci bot requested review from coreydaley and gazarenkov May 29, 2024 16:37
@github-actions GitHub Actions
Copy link
Contributor

PR images are available:

  1. https://quay.io/janus-idp/operator:0.2.0-pr-369-7b108ad
  2. https://quay.io/janus-idp/operator-bundle:0.2.0-pr-369-7b108ad
  3. https://quay.io/janus-idp/operator-catalog:0.2.0-pr-369-7b108ad

@rm3l rm3l mentioned this pull request May 29, 2024
Closed
@github-actions GitHub Actions
Copy link
Contributor

PR images are available:

  1. https://quay.io/janus-idp/operator:0.2.0-pr-369-ef1ea9d
  2. https://quay.io/janus-idp/operator-bundle:0.2.0-pr-369-ef1ea9d
  3. https://quay.io/janus-idp/operator-catalog:0.2.0-pr-369-ef1ea9d

gazarenkov
gazarenkov suggested changes May 30, 2024
View reviewed changes
Copy link
Member

@gazarenkov gazarenkov left a comment

Choose a reason for hiding this comment

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

I would not like to have different name patterns for generated objects forever, let's unify it sooner than later.

controllers/backstage_controller.go Show resolved Hide resolved
integration_tests/db_test.go Outdated Show resolved Hide resolved
pkg/utils/utils.go Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link
Contributor

PR images are available:

  1. https://quay.io/janus-idp/operator:0.2.0-pr-369-ec43e69
  2. https://quay.io/janus-idp/operator-bundle:0.2.0-pr-369-ec43e69
  3. https://quay.io/janus-idp/operator-catalog:0.2.0-pr-369-ec43e69

@rm3l rm3l requested a review from gazarenkov May 31, 2024 08:55
@rm3l rm3l force-pushed the RHIDP-2432--upgrading-operator-from-1.1-to-1.2-makes-existing-app-to-start-with-new-empty-db branch from ec43e69 to efe463e Compare May 31, 2024 13:45
@rm3l rm3l force-pushed the RHIDP-2432--upgrading-operator-from-1.1-to-1.2-makes-existing-app-to-start-with-new-empty-db branch from efe463e to 4ac5a97 Compare May 31, 2024 13:51
@openshift-cherrypick-robot
Copy link

@rm3l: once the present PR merges, I will cherry-pick it on top of 1.2.x in a new PR and assign it to you.

In response to this:

/cherry-pick 1.2.x

But putting on hold until #376 is merged, to avoid the change to the CSV.

/hold

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-sigs/prow repository.

@openshift-ci openshift-ci bot added the do-not-merge/hold PR should not merge yet because someone has issued a /hold command. Required by Prow. label Jun 4, 2024
@openshift-ci openshift-ci bot added lgtm PR is ready to be merged. Required by Prow. approved PR has been approved by an approver from all required OWNERS files. Required by Prow. labels Jun 4, 2024
rm3l and others added 8 commits June 4, 2024 10:21
@rm3l
Add test highlighting the issue and setting the expectations
b173c81
@rm3l
Revert the DB StatefulSet name for backward compatibility with 1.1
f163bd9 
Otherwise, when upgrading from 1.1, a new StatefulSet would be created
with a new data PVC, causing the application to boot with from a brand
new database.
This would break the upgrade path.
@rm3l
Make sure to use backward compatible names for all resources
d4a7ba1 
This generalizes the previous commit, as I realized that, otherwise,  we might
actually be creating duplicate resources like app-config ConfigMaps/Secrets/Services
with different names (while keeping the existing ones),
which might confuse people upgrading from 1.1.

This makes sure we avoid such a similar situation in the future by
ensuring naming is done in a backward-compatible manner.
@rm3l
Handle patch error and delete StatefulSet so it can be recreated
c0caaa4 
Some resources like StatefulSets allow patching a limited set of fields.
For cases where we are upgrading existing instances, we might need to
change the StatefulSet spec.
The recommended approach to doing this is to delete the existing
StatefulSet, but keep its dependents (like PVCs and Pods) orphan, then
recreate the StatefulSet.
This way, it will be upgraded (with some downtime) with no data loss.
@rm3l
Revert "Make sure to use backward compatible names for all resources"
52a96f7 
This reverts commit 0b1be1c2720d96dd6bdd60f230e66819b2bc9e4b.
@rm3l
Address review comments on testing
0e2250f 
Co-authored-by: Gennady Azarenkov <[email protected]>
@rm3l
Revert useless change to integration_tests/suite_test.go
c3e4028
@rm3l
Address review comments on testing
604b7ed 
Co-authored-by: Gennady Azarenkov <[email protected]>
@rm3l rm3l force-pushed the RHIDP-2432--upgrading-operator-from-1.1-to-1.2-makes-existing-app-to-start-with-new-empty-db branch from e8cba22 to 604b7ed Compare June 4, 2024 08:22
@openshift-ci openshift-ci bot removed the lgtm PR is ready to be merged. Required by Prow. label Jun 4, 2024
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Jun 4, 2024

Quality Gate Passed Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
2.0% Duplication on New Code

See analysis details on SonarCloud

@rm3l
Copy link
Member Author

rm3l commented Jun 4, 2024

/hold cancel

Rebased onto main, now that #376 has been merged.

@openshift-ci openshift-ci bot removed the do-not-merge/hold PR should not merge yet because someone has issued a /hold command. Required by Prow. label Jun 4, 2024
@gazarenkov gazarenkov self-requested a review June 4, 2024 08:27
@openshift-ci openshift-ci bot added the lgtm PR is ready to be merged. Required by Prow. label Jun 4, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Jun 4, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gazarenkov

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:

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

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 4, 2024

PR images are available:

  1. https://quay.io/janus-idp/operator:0.3.0-pr-369-604b7ed
  2. https://quay.io/janus-idp/operator-bundle:0.3.0-pr-369-604b7ed
  3. https://quay.io/janus-idp/operator-catalog:0.3.0-pr-369-604b7ed

@openshift-merge-bot openshift-merge-bot bot merged commit ed19db2 into janus-idp:main Jun 4, 2024
8 checks passed
@openshift-cherrypick-robot
Copy link

@rm3l: new pull request created: #377

In response to this:

/cherry-pick 1.2.x

But putting on hold until #376 is merged, to avoid the change to the CSV.

/hold

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-sigs/prow repository.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Jun 4, 2024
Merged
@rm3l rm3l deleted the RHIDP-2432--upgrading-operator-from-1.1-to-1.2-makes-existing-app-to-start-with-new-empty-db branch June 4, 2024 08:32
@rm3l rm3l mentioned this pull request Jun 10, 2024
Merged
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gazarenkov gazarenkov gazarenkov approved these changes

@coreydaley coreydaley Awaiting requested review from coreydaley

Assignees

@gazarenkov gazarenkov

Labels
approved PR has been approved by an approver from all required OWNERS files. Required by Prow. lgtm PR is ready to be merged. Required by Prow.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rm3l @openshift-cherrypick-robot @gazarenkov