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

[WIP] Updated notifications reconciliation #924

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

ciiay
Copy link
Collaborator

@ciiay ciiay commented Jun 5, 2023

Signed off: @ciiay

What type of PR is this?
/kind bug

What does this PR do / why we need it:
For flaky kuttl test 1-055_validate_notification_controller
When notifications controller deployment was getting deleted, it got this error

message: >-
        pods "example-argocd-notifications-controller-78b644d677-b77pp" is
        forbidden: error looking up service account
        kuttl-test-whole-toad/example-argocd-argocd-notifications-controller:
        serviceaccount "example-argocd-argocd-notifications-controller" not
        found

Major updates for best practice:

  • Moved cr.Spec.Notifications.Enabled condition check from each resource reconciliation function into higher-level function, reconcileResources
  • Split the original resource reconcile function into two functions. One for reconciliation, another one for deletion.
  • Removed deletion from deleteNotificationsPred function, so that it's purely for predication of Notifications.Enabled value change.

Have you updated the necessary documentation?

  • Documentation update is required by this PR.
  • Documentation has been updated.

Which issue(s) this PR fixes:

Fixes #? GITOPS-2857

How to test changes / Special notes to the reviewer:
kuttl test parallel/1-055_validate_notification_controller shouldn't be flaky or fail anymore. This PR works along with gitops-operator PR which adds teststep to wait for notification controller pod to be ready.

Note:
Corresponding test case doesn't need to change. This only adds a not found check before executing deletion of service account. I tried to update the test case, but since it's a race condition issue, deployment and rolebinding instance will always be deleted first before reconcile function deletes the service account.

@ciiay ciiay force-pushed the gitops-2857-fix-055-kuttl-parallel-test branch from 54f9489 to a0cc78a Compare June 5, 2023 21:43
@ciiay ciiay changed the title Add not found check for notifications sa deletion [WIP] Add not found check for notifications sa deletion Jun 6, 2023
@ciiay ciiay changed the title [WIP] Add not found check for notifications sa deletion Add not found check for notifications sa deletion Jun 7, 2023
@ciiay ciiay changed the title Add not found check for notifications sa deletion [WIP] Updated notifications reconciliation Jun 7, 2023
@ciiay ciiay force-pushed the gitops-2857-fix-055-kuttl-parallel-test branch 3 times, most recently from 0bf612d to 5cf1951 Compare June 8, 2023 17:41
@ciiay
Copy link
Collaborator Author

ciiay commented Jun 8, 2023

Hi @jaideepr97 @iam-veeramalla I have raised a PR #929 to fix the failed 1-014_validate_parallelism_limit test. Please take a look. Thanks!

@ciiay
Copy link
Collaborator Author

ciiay commented Jun 8, 2023

1-020_validate_redis_ha_nonha passes on local test run.

➜  argocd-operator git:(gitops-2857-fix-055-kuttl-parallel-test) kubectl kuttl test ./tests/ha --config ./tests/kuttl-tests.yaml --test 1-020_validate_redis_ha_nonha

2023/06/08 16:37:21 kutt-test config testdirs is overridden with args: [ ./tests/ha ]
=== RUN   kuttl
    harness.go:462: starting setup
    harness.go:252: running tests using configured kubeconfig.
    harness.go:275: Successful connection to cluster at: https://0.0.0.0:60671
    harness.go:360: running tests
    harness.go:73: going to run test suite with timeout of 300 seconds for each step
    harness.go:372: testsuite: ./tests/ha has 1 tests
=== RUN   kuttl/harness
=== RUN   kuttl/harness/1-020_validate_redis_ha_nonha
=== PAUSE kuttl/harness/1-020_validate_redis_ha_nonha
=== CONT  kuttl/harness/1-020_validate_redis_ha_nonha
    logger.go:42: 16:37:22 | 1-020_validate_redis_ha_nonha | Creating namespace: kuttl-test-maximum-basilisk
    logger.go:42: 16:37:22 | 1-020_validate_redis_ha_nonha/1-basic | starting test step 1-basic
    logger.go:42: 16:37:22 | 1-020_validate_redis_ha_nonha/1-basic | ArgoCD:kuttl-test-maximum-basilisk/example-argocd created
    logger.go:42: 16:37:25 | 1-020_validate_redis_ha_nonha/1-basic | test step completed 1-basic
    logger.go:42: 16:37:25 | 1-020_validate_redis_ha_nonha/2-enable-ha | starting test step 2-enable-ha
    logger.go:42: 16:37:26 | 1-020_validate_redis_ha_nonha/2-enable-ha | ArgoCD:kuttl-test-maximum-basilisk/example-argocd updated
    logger.go:42: 16:43:48 | 1-020_validate_redis_ha_nonha/2-enable-ha | test step completed 2-enable-ha
    logger.go:42: 16:43:48 | 1-020_validate_redis_ha_nonha/3-update-ha-resources | starting test step 3-update-ha-resources
    logger.go:42: 16:43:48 | 1-020_validate_redis_ha_nonha/3-update-ha-resources | ArgoCD:kuttl-test-maximum-basilisk/example-argocd updated
    logger.go:42: 16:55:07 | 1-020_validate_redis_ha_nonha/3-update-ha-resources | test step completed 3-update-ha-resources
    logger.go:42: 16:55:07 | 1-020_validate_redis_ha_nonha/4-disable-ha | starting test step 4-disable-ha
    logger.go:42: 16:55:08 | 1-020_validate_redis_ha_nonha/4-disable-ha | ArgoCD:kuttl-test-maximum-basilisk/example-argocd updated
    logger.go:42: 16:55:11 | 1-020_validate_redis_ha_nonha/4-disable-ha | test step completed 4-disable-ha
    logger.go:42: 16:55:11 | 1-020_validate_redis_ha_nonha | skipping kubernetes event logging
    logger.go:42: 16:55:11 | 1-020_validate_redis_ha_nonha | Deleting namespace: kuttl-test-maximum-basilisk
=== CONT  kuttl
    harness.go:405: run tests finished
    harness.go:513: cleaning up
    harness.go:570: removing temp folder: ""
--- PASS: kuttl (1143.25s)
    --- PASS: kuttl/harness (0.00s)
        --- PASS: kuttl/harness/1-020_validate_redis_ha_nonha (1142.12s)
PASS

@iam-veeramalla Can we trigger this ci checks again?

ciiay and others added 19 commits June 20, 2023 17:51
Signed-off-by: Yi Cai <[email protected]>
Signed-off-by: Yi Cai <[email protected]>
Signed-off-by: Yi Cai <[email protected]>
Signed-off-by: Yi Cai <[email protected]>
Signed-off-by: Yi Cai <[email protected]>
* Add sharding configuration options for dynamic scaling of application controller

---------

Signed-off-by: ishitasequeira <[email protected]>
Signed-off-by: Yi Cai <[email protected]>
* upgrade golangci-lint

Signed-off-by: Jaideep Rao <[email protected]>

* add kuttl test to verify hpa handling behavior

Signed-off-by: Jaideep Rao <[email protected]>

* fix typo in assert file name

Signed-off-by: Jaideep Rao <[email protected]>

---------

Signed-off-by: Jaideep Rao <[email protected]>
Signed-off-by: Yi Cai <[email protected]>
Signed-off-by: Yi Cai <[email protected]>
@ciiay ciiay force-pushed the gitops-2857-fix-055-kuttl-parallel-test branch from 0adaac2 to 1e74381 Compare June 20, 2023 21:51
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.

7 participants