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

[OADP-450] Reduce "perform updated on X ..." reconciler logs #584

Merged

Conversation

kaovilai
Copy link
Member

@kaovilai kaovilai commented Mar 4, 2022

Reduce 🙈 update events and eliminate ObjectMeta update race 🏎 conditions for velero deployment and restic daemonset.

Now results from CreateOrUpdate are either created, or unchanged, update events are minimal.
No longer producing race condition update events thus reduces 99% of logs currently printed from these reconcilers.

@kaovilai kaovilai self-assigned this Mar 4, 2022
@kaovilai kaovilai force-pushed the reconcileLogsReduction branch 2 times, most recently from 2163a0e to ceb92d1 Compare March 4, 2022 17:08
@codecov-commenter
Copy link

codecov-commenter commented Mar 4, 2022

Codecov Report

Merging #584 (0aff52a) into master (c5293b1) will decrease coverage by 0.11%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #584      +/-   ##
==========================================
- Coverage   37.60%   37.49%   -0.12%     
==========================================
  Files          14       14              
  Lines        2917     2915       -2     
==========================================
- Hits         1097     1093       -4     
- Misses       1737     1739       +2     
  Partials       83       83              
Impacted Files Coverage Δ
pkg/credentials/credentials.go 25.32% <0.00%> (-0.22%) ⬇️
controllers/restic.go 52.57% <100.00%> (-0.96%) ⬇️
controllers/velero.go 48.29% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5293b1...0aff52a. Read the comment docs.

@shubham-pampattiwar
Copy link
Member

@kaovilai Do you mind adding a fix for this issue #585 in this PR itself ?

@kaovilai
Copy link
Member Author

kaovilai commented Mar 4, 2022

@kaovilai Do you mind adding a fix for this issue #585 in this PR itself ?

Do you think it'll be too big. Could do in one pr

@kaovilai kaovilai changed the title Eliminate "perform updated on dpa metrics service" from logs Reduce "perform updated on X ..." reconciler logs Mar 5, 2022
@kaovilai kaovilai marked this pull request as draft March 5, 2022 00:32
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 5, 2022
@kaovilai kaovilai force-pushed the reconcileLogsReduction branch 2 times, most recently from 2ae3a95 to 0b6df3a Compare March 5, 2022 01:24
@kaovilai kaovilai marked this pull request as ready for review March 5, 2022 02:13
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 5, 2022
@openshift-ci openshift-ci bot requested review from rayfordj and sseago March 5, 2022 02:13
@kaovilai
Copy link
Member Author

kaovilai commented Mar 5, 2022

/retest

controllers/restic.go Outdated Show resolved Hide resolved
controllers/restic.go Outdated Show resolved Hide resolved
@kaovilai
Copy link
Member Author

kaovilai commented Mar 8, 2022

Maybe copy typemeta instead of ignoring in test.

@openshift-ci
Copy link

openshift-ci bot commented Mar 8, 2022

@kaovilai: all tests passed!

Full PR test history. Your PR dashboard.

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/test-infra repository. I understand the commands that are listed here.

@shubham-pampattiwar shubham-pampattiwar merged commit 18fd77f into openshift:master Mar 9, 2022
@kaovilai kaovilai changed the title Reduce "perform updated on X ..." reconciler logs [OADP-450] Reduce "perform updated on X ..." reconciler logs May 6, 2022
kaovilai added a commit to kaovilai/oadp-operator that referenced this pull request May 6, 2022
* Eliminate "perform updated on dpa metrics service" from logs

* restic reconcile log reduction

* reduce velero deployment reconcile logs

* setDsDefaults is not needed

* Revert "Eliminate "perform updated on dpa metrics service" from logs"

This reverts commit 45bd6fc.

* defaultMode no longer needed

* remove prevDs

* No need to save deploymentName and ownerRefs

* rm typemeta ignores
dymurray pushed a commit that referenced this pull request May 6, 2022
* Eliminate "perform updated on dpa metrics service" from logs

* restic reconcile log reduction

* reduce velero deployment reconcile logs

* setDsDefaults is not needed

* Revert "Eliminate "perform updated on dpa metrics service" from logs"

This reverts commit 45bd6fc.

* defaultMode no longer needed

* remove prevDs

* No need to save deploymentName and ownerRefs

* rm typemeta ignores
@kaovilai
Copy link
Member Author

/cherry-pick oadp-1.0

@openshift-cherrypick-robot
Copy link
Contributor

@kaovilai: #584 failed to apply on top of branch "oadp-1.0":

Applying: Eliminate "perform updated on dpa metrics service" from logs
Using index info to reconstruct a base tree...
M	controllers/monitor.go
Falling back to patching base and 3-way merge...
Auto-merging controllers/monitor.go
CONFLICT (content): Merge conflict in controllers/monitor.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Eliminate "perform updated on dpa metrics service" from logs
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick oadp-1.0

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/test-infra repository.

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