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

Options to load and save states #197

Merged
merged 11 commits into from
May 26, 2020
Merged

Options to load and save states #197

merged 11 commits into from
May 26, 2020

Conversation

nritsche
Copy link
Contributor

@nritsche nritsche commented May 23, 2020

With this, cocos state can be written to disk using

coco save-state foo

and later restored again with

coco load-state foo

See all saved states with

coco saved-states

BREAKING CHANGE: the state path in the config is not a file but a directory now. Move the state in the coco installation from /old/location to /old/location/active (or replace it with an empty directory).

Closes #192

@nritsche nritsche added the enhancement New feature or request label May 23, 2020
@nritsche nritsche requested review from jrs65 and anjakefala May 23, 2020 06:41
@nritsche nritsche self-assigned this May 23, 2020
@nritsche
Copy link
Contributor Author

nritsche commented May 23, 2020

  • add test for overwrite option

nritsche added 3 commits May 24, 2020 10:13
- refactor: simplify coco_runner arguments (for testing)
- Add internal endpoints and client commands to save and load endpoints.
  New commands / internal endpoints:
  - save-state
  - load-state
  - saved-states

  Parts of the state configured to be excluded from resets are also
  excluded from being loaded from a previously loaded state. But they do
  get saved.

BREAKING CHANGE:
The config variable 'storage_path' is not a file anymore, but a
directory. The state is saved there under the name 'active' and saved
states are added here as well. If you are updating your coco instance,
you have to manually move the persistent state from 'storage_path' to
'storage_path'/active.
- blacken scripts
- add check to travis CI
self._name_active_state = "active"

# List saved states on disk
p = Path(self._storage_path).glob("**/*")

Choose a reason for hiding this comment

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

The Path() here might be redundant since _storage_path seems to be guaranteed to be PathLike.

However, it also does not seem to hurt if it is there. =)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I tried that, but self._storage_path was a str.

@nritsche
Copy link
Contributor Author

nritsche commented May 25, 2020

  • remove active from this options list make the message less confusing
[rick@cndhn ~]$ coco load-state active
Sending request...


message: Can't load state active. This name is reserved. Choose one of ['active',
  'pre-pattern-test_2020-05-25-12-57'].
status_code: 400

Done.

coco/state.py Outdated
for excluded in self.exclude_from_reset:
if excluded.startswith(path):
if len(path) == 0:
excluded = excluded[len(path) :]
Copy link

@anjakefala anjakefala May 25, 2020

Choose a reason for hiding this comment

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

If len(path) is 0, is this not excluded = excluded?

if path_first in state:
self._exclude_paths(path_first, state[path_first])
if excluded in state:
del state[excluded]

Choose a reason for hiding this comment

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

You can do all this in-place because python allows you to pass references to dictionaries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, right?

Choose a reason for hiding this comment

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

Yeah, that is right, it just always catches me off-guard!

Are dicts the only object this applies to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

@anjakefala anjakefala left a comment

Choose a reason for hiding this comment

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

Man, this is such a big change!

The main things look good. You are very thorough with your error checks, and I feel more comfortable with there being tests.

I am curious about what the use-case is for excluded?

@nritsche
Copy link
Contributor Author

I am curious about what the use-case is for excluded?

This https://github.com/chime-experiment/ch_ansible/blob/62ea700463d75212c5b972618a6ea4c00b6fb18c/roles/internal/coco_common/templates/coco.conf.j2#L286

@nritsche
Copy link
Contributor Author

Thanks for the thorough review. I think I addressed all the comments.

@nritsche nritsche merged commit 70be940 into master May 26, 2020
@nritsche nritsche deleted the rn/backupstate branch May 26, 2020 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add command to backup and restore state
2 participants