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.

Sign up for GitHub

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

Jump to bottom

Implement dry-run for install/upgrade process #464

Closed
wants to merge 14 commits into from
Closed

Implement dry-run for install/upgrade process #464

wants to merge 14 commits into from

Conversation

Deo121996
Copy link

@Deo121996 Deo121996 commented Jun 26, 2023 •
edited
Loading

Description

This improvement is to enable dry-run for installation and upgrade procedure of Kubemarine using the existing --without-act option

  • This dry-run implementation will perform all the possible checks and generate all the possible files in the dumps directory to simulate the actual install/upgrade process.
  • It will not make changes to actual nodes but perform all the possible checks by executing read commands.
  • User can simulate the actual procedure by running a dry run before executing the actual procedure.

Solution

Improvement
*

How to apply

Code change

Test Cases

TestCase 1
Steps:

  1. Configure a valid cluster.yaml for the all-in-one cluster.
  2. Run kubemarine install --without-act and check if the process succeeds and configuration files are created in the dumps directory.

TestCase 2
Steps:

  1. Pass invalid plugin yaml template inside kubemarine/plugins/yaml directory.
  2. Run kubemarine install --without-act and check if the process fails and configuration files are created in the dumps directory.

TestCase 3
Steps:

  1. Configure a valid procedure.yaml for the upgrade.
  2. Run kubemarine upgrade procedure.yaml --without-act and check if the process succeeds and configuration files are created in the dumps directory.

Checklist

  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • Integration CI passed
  • Unit tests. If Yes list of new/changed tests with brief description
  • There is no merge conflicts

@Deo121996 Deo121996 changed the title Implement dry-run for install/upgrade process [Draft] Implement dry-run for install/upgrade process Jun 26, 2023
@Deo121996
Copy link
Author

Deo121996 commented Jun 26, 2023 •
edited
Loading

Results

test-case(failure): kubemarine install --without-act
dump files: dry-run-install-fail.zip

test-case(success): kubemarine install --without-act
dump files: dry-run-install.zip

test case(success): kubemarine upgrade procedure.yaml --without-act
dump files: dry-run-upgrade.zip

@pranavcracker pranavcracker marked this pull request as draft June 26, 2023 05:26
@pranavcracker pranavcracker marked this pull request as ready for review June 26, 2023 05:26
@Deo121996 Deo121996 marked this pull request as draft June 26, 2023 05:27
@pranavcracker pranavcracker changed the title [Draft] Implement dry-run for install/upgrade process Implement dry-run for install/upgrade process Jun 26, 2023
@pranavcracker pranavcracker linked an issue Jun 26, 2023 that may be closed by this pull request
Extended dry-run for all procedures #455
Open
@koryaga koryaga requested a review from ilia1243 June 26, 2023 07:38
@ilia1243
Copy link
Contributor

The approach is ok, working on the implementation details.

@ilia1243
Copy link
Contributor

All procedure should be supported. If it is impossible, please introduce new option --dry-run and specify which procedures support it and which do not. Currently, --without-act is supported by all procedures except migrate_kubemarine and do.

@ilia1243
Copy link
Contributor

If it is possible, please make unit tests that will run the procedures in dry-run.

ilia1243
ilia1243 reviewed Jun 27, 2023
View reviewed changes
kubemarine/core/flow.py
Comment on lines 186 to +187
if args.get('without_act', False):
cluster.context["dry_run"] = True
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to not recreate the inventory on the deployer. For example, if you run upgrade, the input cluster.yaml will be changed. This does not seem as "dry-run". I suggest to create separate file in dump/ and reflect the changes there.

kubemarine/core/group.py Outdated
Comment on lines 431 to 432
@_handle_dry_run
def run(self, *args, **kwargs) -> Union[NodeGroupResult, int]:
Copy link
Contributor

@ilia1243 ilia1243 Jun 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I suggest to not call NodeGroup.run/sudo/put for RW operations in dry run at all. The reason is it is difficult to simulate its behaviour. For example, you return empty results, but in fact it returns not empty results in real run. So this can lead to unpredictable changes in behaviour of Kubemarine. Also, put does not return result at all, run/sudo might return int in some cases, and so on...

kubemarine/plugins/__init__.py
@@ -1018,14 +1029,14 @@ def apply_source(cluster: KubernetesCluster, config: dict) -> None:
else:
apply_common_group = cluster.create_group_from_groups_nodes_names(apply_groups, apply_nodes)

destination_common_group.put(source, destination_path, backup=True, sudo=use_sudo)
destination_common_group.put(source, destination_path, backup=True, sudo=use_sudo, dry_run=dry_run)

if apply_required:
method = apply_common_group.run
if use_sudo:
method = apply_common_group.sudo
cluster.log.debug("Applying yaml...")
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor moment, rendering of plugins' templates might depend on previous shell commands. See runtime_vars. If it is possible, I suggest to skip rendering of templates in such cases, because its result will likely be not the same as during the real run.

@Deo121996 Deo121996 requested a review from ilia1243 July 20, 2023 10:16
@koryaga
Copy link
Contributor

koryaga commented Oct 8, 2023

Need to complete rework

@koryaga koryaga closed this Oct 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilia1243 ilia1243 Awaiting requested review from ilia1243

Assignees

@koryaga koryaga

Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extended dry-run for all procedures
4 participants
@Deo121996 @ilia1243 @koryaga @devnimbekar