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

Extend logging and fix missing namespace in where clause #153

Merged
merged 1 commit into from
Jul 21, 2022

Conversation

asmorodskyi
Copy link
Collaborator

No description provided.

@asmorodskyi asmorodskyi force-pushed the updaterun_fixing branch 2 times, most recently from 5bef4e7 to b034d59 Compare July 20, 2022 19:10
@codecov
Copy link

codecov bot commented Jul 20, 2022

Codecov Report

Merging #153 (6ee2e8b) into master (a26e6c3) will decrease coverage by 0.19%.
The diff coverage is 17.64%.

@@            Coverage Diff             @@
##           master     #153      +/-   ##
==========================================
- Coverage   81.03%   80.84%   -0.20%     
==========================================
  Files          26       26              
  Lines        1962     1968       +6     
==========================================
+ Hits         1590     1591       +1     
- Misses        372      377       +5     
Impacted Files Coverage Δ
ocw/lib/db.py 38.21% <0.00%> (-0.25%) ⬇️
ocw/models.py 84.72% <42.85%> (-4.84%) ⬇️

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 a26e6c3...6ee2e8b. Read the comment docs.

ocw/lib/db.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@cfconrad cfconrad left a comment

Choose a reason for hiding this comment

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

LGTM beside the discussed thing...

ocw/lib/db.py Outdated Show resolved Hide resolved
ocw/lib/db.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@grisu48 grisu48 left a comment

Choose a reason for hiding this comment

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

One logging suggestion otherwise LGTM!

If you implement my suggestion from my point of view you can proceed and merge

o.region = i.region
if o.state == StateChoice.DELETED:
logger.error("Update already DELETED instance %s:%s\n\t%s", provider, i.instance_id, i.csp_info)
logger.error("[%s] Update already DELETED instance %s:%s %s",
namespace, provider, i.instance_id, i.all_time_fields())
if o.state != StateChoice.DELETING:
o.state = StateChoice.ACTIVE
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
o.state = StateChoice.ACTIVE
logger.error("[%s] Update and reactivating DELETING instance %s:%s %s",
namespace, provider, i.instance_id, i.all_time_fields())
o.state = StateChoice.ACTIVE

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Keep in mind that message coming just after this for same instance logger.debug("[%s] Update instance %s:%s", namespace, provider, i.instance_id) . While I agree with idea to mention instance activation I would suggest different message :
logger.error("[%s] %s:%s instance which still exists has DELETED state in DB. Reactivating %s", namespace, provider, i.instance_id, i.all_time_fields())

@asmorodskyi asmorodskyi force-pushed the updaterun_fixing branch 2 times, most recently from bee5ce8 to 07147c7 Compare July 21, 2022 09:45
Before this PR instance_id was unique in local DB.
It appears that people creating same entities in different
namespaces. This PR change DB structure and logic around it
to be able to handle such flow.
Also we extending logging of what actually got deleted.
@asmorodskyi asmorodskyi merged commit c803665 into SUSE:master Jul 21, 2022
@asmorodskyi asmorodskyi deleted the updaterun_fixing branch July 21, 2022 11:35
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.

4 participants