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

Sync Mode drift detection #70811

Merged
merged 10 commits into from
May 14, 2024
Merged

Sync Mode drift detection #70811

merged 10 commits into from
May 14, 2024

Conversation

kneeyo1
Copy link
Contributor

@kneeyo1 kneeyo1 commented May 13, 2024

In Sync mode, we do not want options that are not called in configoptions but set_on_disk to show up as drift.

Some options have the prioritize_disk flag and have values on the db and the disk.
configoptions should only update these options if they are not set_on_disk. Right now, if it is set_on_disk but also set in the db, we write it as drift. This PR adds a check for that.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label May 13, 2024
@kneeyo1 kneeyo1 requested a review from fpacifici May 13, 2024 21:33
@kneeyo1 kneeyo1 marked this pull request as ready for review May 13, 2024 21:33
@kneeyo1 kneeyo1 changed the title WIP ignore set on disk options as drift Ignore set on disk options as drift May 13, 2024
Copy link

codecov bot commented May 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.02%. Comparing base (bef6fe8) to head (347dccc).
Report is 74 commits behind head on master.

❗ Current head 347dccc differs from pull request most recent head a034c80. Consider uploading reports for the commit a034c80 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #70811   +/-   ##
=======================================
  Coverage   80.01%   80.02%           
=======================================
  Files        6505     6505           
  Lines      290769   290764    -5     
  Branches    50112    50118    +6     
=======================================
  Hits       232670   232670           
+ Misses      57662    57657    -5     
  Partials      437      437           

see 18 files with indirect coverage changes

@kneeyo1 kneeyo1 force-pushed the fix/options-on-disk branch from c01685a to 45f1808 Compare May 13, 2024 23:33
@kneeyo1 kneeyo1 requested a review from fpacifici May 13, 2024 23:33
Copy link
Contributor

@fpacifici fpacifici left a comment

Choose a reason for hiding this comment

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

I think I am missing something here.
Are you saying that we have options that are set on disk and also on the DB to a value that is not the default value for the option?

This seems something that should not happen as we do not allow an option to be updated on DB if it is set on disk.
This could only have happened if the option was already set to a value on the DB (different than the default) and then set on disk to something different again.

But in that case it means the option cannot be updated with any channel, we should not hide this scenario which is broken.

@kneeyo1
Copy link
Contributor Author

kneeyo1 commented May 14, 2024

I think I am missing something here. Are you saying that we have options that are set on disk and also on the DB to a value that is not the default value for the option?

This seems something that should not happen as we do not allow an option to be updated on DB if it is set on disk. This could only have happened if the option was already set to a value on the DB (different than the default) and then set on disk to something different again.

But in that case it means the option cannot be updated with any channel, we should not hide this scenario which is broken.

We have options that are set on disk. In the db, they are set to their default value.
This means that they exist on the DB, but do not read from it, they are still reading the disk value (which is not a default value).

I don't think this behavior is broken, as FLAG_PRIORTIZE_DISK seems to do exactly as said, take the disk value if it exists, otherwise read from the db.

Previously, when configoptions sync is called, for all options that exist and are set (even if just set to their default value), if they were not last updated by the automator, we write it as drift.

My change just checks if an option is already set on disk, if it is, don't write it as drift.

@kneeyo1 kneeyo1 changed the title Ignore set on disk options as drift Improve Drift message in sync mode May 14, 2024
@kneeyo1 kneeyo1 force-pushed the fix/options-on-disk branch from 0ddc36b to b6c29b8 Compare May 14, 2024 22:18
@kneeyo1 kneeyo1 requested a review from fpacifici May 14, 2024 22:19
@kneeyo1 kneeyo1 changed the title Improve Drift message in sync mode Sync Mode drift detection May 14, 2024
presenter_delegator.drift(opt.name, "")
drift_found = True
if (
options.can_update(
Copy link
Contributor Author

@kneeyo1 kneeyo1 May 14, 2024

Choose a reason for hiding this comment

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

if an option is passed into configoptions, and it is set on disk, we check for it here (when we call attempt_update on line 244) https://github.com/getsentry/sentry/blob/fix/options-on-disk/src/sentry/options/manager.py#L188-L200

otherwise, if it is set on disk but not defined in the automator, since options.get() and options.isset() will still return true,(even if the options DNE in the DB) we again check to see if the option is on disk. if it is, we continue.

@kneeyo1 kneeyo1 enabled auto-merge (squash) May 14, 2024 22:55
@kneeyo1 kneeyo1 merged commit 300f843 into master May 14, 2024
47 checks passed
@kneeyo1 kneeyo1 deleted the fix/options-on-disk branch May 14, 2024 22:57
@github-actions github-actions bot locked and limited conversation to collaborators May 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants