Deploy apps on both primary and secondary cluster #9123
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pull-request-size
bot
added
size/XL
and removed
size/L
prsurve
reviewed
May 9, 2024
tests/functional/disaster-recovery/metro-dr/test_multiple_apps_failover_and_relocate.py
Outdated
Show resolved
Hide resolved
Shrivaibavi
changed the title
am-agrawa
reviewed
Aug 20, 2024
tests/functional/disaster-recovery/metro-dr/test_multiple_apps_failover_and_relocate.py
Outdated
Show resolved
Hide resolved
tests/functional/disaster-recovery/metro-dr/test_multiple_apps_failover_and_relocate.py
Show resolved
Hide resolved
tests/functional/disaster-recovery/metro-dr/test_multiple_apps_failover_and_relocate.py
Outdated
Show resolved
Hide resolved
tests/functional/disaster-recovery/metro-dr/test_multiple_apps_failover_and_relocate.py
Outdated
Show resolved
Hide resolved
|
Shrivaibavi
added
Verified
am-agrawa
reviewed
Aug 29, 2024
ocs_ci/helpers/dr_helpers_ui.py
Outdated
| bool: True if the application is not present, false otherwise | ||
|
|
||
| """ | ||
| if workload_to_check: |
There was a problem hiding this comment.
This condition will always return True based upon the current param in function if workload_to_check
There was a problem hiding this comment.
Now workload_to_check is a list, hence it will return false if its empty
There was a problem hiding this comment.
@Shrivaibavi Aren't you returning True in line 684 if the list is empty?
There was a problem hiding this comment.
Workload_to_check becomes a necessary argument, if its empty ideally it wont enter the loop. I hope this answer helps.
And sorry if my above answer is confusing.
There was a problem hiding this comment.
I am just asking what would it return if the list is empty?
There was a problem hiding this comment.
Nothing... Ideally it shouldn't be an empty list. Validating if its an empty list or contains element and raising exception wouldn't make much difference as workloads_to_check had been added as necessary param IMO.
There was a problem hiding this comment.
No, ideally we should raise an exception or fail the test if the list is empty if what I feel.
ocs_ci/helpers/dr_helpers_ui.py
Outdated
| log.info(f"Application {workload_to_delete} got deleted successfully") | ||
| return True | ||
| else: | ||
| log.error("Application not present to delete") |
There was a problem hiding this comment.
Is this is right error message?
There was a problem hiding this comment.
@Shrivaibavi Could you pls respond to this query?
There was a problem hiding this comment.
yes IMO.
if verify_application_present_in_ui(
acm_obj, workloads_to_check=workloads_to_delete, timeout=timeout
):
else:
log.error("Applications not present to delete from UI")
return False
Please check the modified version
There was a problem hiding this comment.
What if we have multiple apps with the same name and it fails the test? That's why I suggested to apply the filter for the type of workload in one of my other comments.
There was a problem hiding this comment.
The main intention of the PR is to deploy apps on both the clusters and failover multiple apps from C1 to C2 and slowly got into delete app from UI as teardown.
Its a very nice suggestion, But can only be incorporated in the upcoming PR. I believe there is no point in holding this PR for the above small correction. Please add your comment in this open issue #10365
ocs_ci/ocs/dr/dr_workload.py
Outdated
| git_kustomization_yaml_data, self.git_repo_kustomization_yaml_file | ||
| ) | ||
|
|
||
| workload_namespaces.append(self._get_workload_namespace()) |
There was a problem hiding this comment.
The workload_namespaces variable is populated with values within the loop but is not used anywhere else in the code. Please check if there might be a missing use case.
| Test to deploy applications on both the managed clusters and test Failover and Relocate actions on them | ||
|
|
||
| """ | ||
| self.workload_namespaces = [] |
There was a problem hiding this comment.
This variable appears to be unused. Please check if it's required or if it can be removed.
ocs_ci/ocs/dr/dr_workload.py
Outdated
| @@ -307,7 +483,7 @@ def delete_workload(self, force=False, rbd_name="rbd", switch_ctx=None): | |||
| try: | |||
| config.switch_ctx(switch_ctx) if switch_ctx else config.switch_acm_ctx() | |||
| run_cmd( | |||
| f"oc delete -k {self.workload_subscription_dir}/{self.workload_name}" | |||
| f"oc delete -k {self.workload_subscription_dir}/{self.workload_name} -n {self.workload_namespace}" | |||
There was a problem hiding this comment.
Is this change still required, given that you are using delete_application_ui for workload deletion?
ocs_ci/ocs/dr/dr_workload.py
Outdated
| @@ -356,7 +532,7 @@ def delete_workload(self, force=False, rbd_name="rbd", switch_ctx=None): | |||
|
|
|||
| finally: | |||
| config.switch_ctx(switch_ctx) if switch_ctx else config.switch_acm_ctx() | |||
| run_cmd(f"oc delete -k {self.workload_subscription_dir}") | |||
| run_cmd(f"oc delete -k {self.workload_subscription_dir} -n {ramen_ns}") | |||
There was a problem hiding this comment.
Same as above, is this still required?
am-agrawa
reviewed
Aug 29, 2024
Comment on lines
+276
to
+212
| clusters = [self.preferred_primary_cluster] if primary_cluster else [] | ||
| if secondary_cluster: |
There was a problem hiding this comment.
If I want to deploy workloads on secondary_cluster which is set to False by default, the function would fail because clusters will be equal to [] empty list and we can not iterate over it. We should handle this use case.
There was a problem hiding this comment.
If you will set the parameters like:
primary_cluster=False
secondary_cluster=True
than it will set the clusters variable to empty list on the first line (276) and then add the secondary cluster to the empty list in clusters, which then will be used in the following for loop.
There was a problem hiding this comment.
But where are we passing the secondary cluster to the empty list?
There was a problem hiding this comment.
on the following line:
clusters.append(self.preferred_secondary_cluster)
Signed-off-by: Shrivaibavi Raghaventhiran <[email protected]>
Signed-off-by: Shrivaibavi Raghaventhiran <[email protected]>
Signed-off-by: Shrivaibavi Raghaventhiran <[email protected]>
…app_ui Signed-off-by: Shrivaibavi Raghaventhiran <[email protected]>
Signed-off-by: Shrivaibavi Raghaventhiran <[email protected]>
Signed-off-by: Shrivaibavi Raghaventhiran <[email protected]>
Signed-off-by: Shrivaibavi Raghaventhiran <[email protected]>
Signed-off-by: Shrivaibavi Raghaventhiran <[email protected]>
Signed-off-by: Shrivaibavi Raghaventhiran <[email protected]>
dahorak
approved these changes
Sep 3, 2024
am-agrawa
approved these changes
Sep 3, 2024
sidhant-agrawal
approved these changes
Sep 4, 2024
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: am-agrawa, dahorak, Shrivaibavi, sidhant-agrawal The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.