-
Notifications
You must be signed in to change notification settings - Fork 684
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
feat(mirror): Clear state parts gcp bucket on reset #12502
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #12502 +/- ##
==========================================
- Coverage 69.96% 69.94% -0.02%
==========================================
Files 838 838
Lines 169135 169139 +4
Branches 169135 169139 +4
==========================================
- Hits 118338 118307 -31
- Misses 45666 45698 +32
- Partials 5131 5134 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
@@ -235,6 +236,25 @@ def _apply_config_changes(node, state_sync_location): | |||
do_update_config(node, f'{key}={json.dumps(change)}') | |||
|
|||
|
|||
def _clear_state_parts_if_exists(args, nodes): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity what is the naming convention here for functions that start with _
vs those that don't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually refers to a "private" function although python doesn't directly have a concept of private functions.
|
||
state_dumper_node = nodes.find(lambda n: n.want_state_dump) | ||
if state_dumper_node is None: | ||
logger.info('No state dumper node found, skipping state parts cleanup.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check for a dumper? Would it be more robust if we simply always clear the bucket?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clear command is executed on the dumper. This is because we want to have the right access to the bucket and the user on the dumper already has the write access.
On top of this, If you choose to reset any other nodes in the network, maybe you don't want to clear the bucket.
i.e. you are testing epoch sync / state sync and you want to revert a single node to a backup point. Unless that node is the dumper node, the bucket will not be cleared.
By the way are we sure we are cleaning this up when the test is ultimately torn down? Does terraform handle it? |
Unless you change the config between running |
The bucket is created along with the state dumper node. If the state dumper node is present we want to clear the bucket every time we reset the network to a previous state.
This can be on
hard-reset
,new-test
or when we runreset
to restore from a backup.The command will be executed on the dumper node to make sure that it has the edit access to the bucket.