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

Fix: Ensure ConfigMap and StatefulSet updates are applied during operator upgrades #1619

Merged
merged 12 commits into from
Jan 9, 2025

Conversation

Mangaal
Copy link
Contributor

@Mangaal Mangaal commented Dec 12, 2024

What type of PR is this?
This PR fixes an issue where updates to the argocd-redis-ha-configmap, argocd-redis-ha-health-configmap, and the StatefulSet argocd-redis-ha-server were not being reflected during OpenShift GitOps operator upgrades.

The current code missed checking for changes in:

  • The data field of argocd-redis-ha-configmap and argocd-redis-ha-health-configmap.
  • The env variables in the StatefulSet argocd-redis-ha-server.

As a result, changes introduced in higher versions of the operator were not applied unless the config maps and StatefulSet were manually deleted or the .spec.ha.enabled field was toggled in the ArgoCD Custom Resource (CR). This PR ensures these changes are correctly detected and applied.

This PR:
Adds logic to check for changes in the argocd-redis-ha-configmap and argocd-redis-ha-health-configmap data fields during operator upgrades.
Includes validation for the env variables in the argocd-redis-ha-server StatefulSet to ensure updates are reflected.
Ensures that any required updates are applied automatically without manual intervention.

/kind bug

Steps to reproduce the issue:

  • Install OpenShift GitOps Operator v1.12.0 on an OCP 4.16.4 cluster.
  • Create an ArgoCD instance with .spec.ha.enabled = true.
  • Upgrade the operator to v1.14.0, v1.14.1, and v1.14.2.
  • Observe that the argocd-redis-ha-configmap and argocd-redis-ha-health-configmap are not updated as expected.
  • Delete the config maps manually, and observe that they are recreated correctly but result in AUTH errors.
  • Toggle the .spec.ha.enabled field to fix the issue.

@Mangaal Mangaal changed the title Fix upgrade sync Fix: Ensure ConfigMap and StatefulSet updates are applied during operator upgrades Dec 12, 2024
Signed-off-by: Mangaal <[email protected]>
Signed-off-by: Mangaal <[email protected]>
@svghadi
Copy link
Collaborator

svghadi commented Dec 24, 2024

Can we add a unit test to check configmap reconciliation? Create the cm resource using reconcileRedisHAHealthConfigMap function, modify the created configmap resource and run the reconcileRedisHAHealthConfigMap again to ensure the values are reset to default

Copy link
Collaborator

@svghadi svghadi left a comment

Choose a reason for hiding this comment

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

Just a small nit, everything else looks great! Awesome job on the unit tests. Thanks!

controllers/argocd/configmap.go Show resolved Hide resolved
controllers/argocd/configmap.go Show resolved Hide resolved
Copy link
Collaborator

@svghadi svghadi left a comment

Choose a reason for hiding this comment

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

Hi, Sorry nitpicking again. Your previous comments made sense in 2 places. Can you revert the changes for this. Also please rebase, I have just merged a PR which will fix failing extra-cmd e2e test.

controllers/argocd/configmap.go Outdated Show resolved Hide resolved
controllers/argocd/configmap.go Outdated Show resolved Hide resolved
@Mangaal Mangaal requested a review from svghadi January 9, 2025 06:44
@svghadi
Copy link
Collaborator

svghadi commented Jan 9, 2025

--- FAIL: kuttl/harness/1-036_validate_role_rolebinding_for_source_namespace (64.05s)

is a known flaky failure. Ignoring it and merging the PR.

@svghadi svghadi merged commit 5049cdf into argoproj-labs:master Jan 9, 2025
6 of 7 checks passed
@svghadi
Copy link
Collaborator

svghadi commented Jan 9, 2025

/cherry-pick release-0.13

Copy link

Cherry-pick failed with Merge error 5049cdfee963972964cfb89d510274136802c3d5 into temp-cherry-pick-32de84-release-0.13

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.

2 participants