Identify clustered sap instances #3397

arbulu89 · 2025-03-20T14:22:30Z

Description

Identify clustered SAP instances as right now, all SAP application and database instances running in a cluster host are identified as clustered instances, which is not always the case.

With this, we can be precise in the ApplicationInstanceMoved event, and only do it when the instance is clustered.

All the new logic is almost implemented in the policies code. Everything else is just side-effect work.

Now that we have a more detailed SAP instances data, I have removed the sid and additional_sids entries from all the code, except the frontend.

In the next PR I want to deprecate the sid and additional_sids elements from the API (keep them for backward compatibility) and send the new SAP instances to the frontend. This way, we can display properly if the instance is clustered as well.

How was this tested?

UT, regression test, manual testing...

arbulu89 · 2025-03-20T14:26:43Z

lib/trento/discovery/policies/cluster_policy.ex

@@ -669,7 +715,7 @@ defmodule Trento.Discovery.Policies.ClusterPolicy do
       }) do
    Enum.concat(
      primitives,
-      Enum.flat_map(clones, &Map.get(&1, :primitives, [])) ++
+      Enum.flat_map(clones, &(&1 |> Map.get(:primitive, []) |> List.wrap())) ++


This was a small bug we had. We didn't notice as we were not using cloned resources coming from this function

arbulu89 · 2025-03-20T16:37:13Z

lib/trento_web/controllers/v2/cluster_json.ex

+    cluster
+    |> Map.put(:sid, SapInstance.get_hana_instance_sid(sap_instances))
+    |> Map.put(:additional_sids, SapInstance.get_sap_instance_sids(sap_instances))
+    |> Map.delete(:sap_instances)


Removing the sap_instances by now.
I will work on the frontend part in a next implementation and see how to give the information to the frontend

abravosuse

Tested using http://ghcr.io/trento-project/trento-web:3397-env.

nelsonkopliku

hey @arbulu89 the code looks good. These kind of changes require possibly a lot of changes indeed 😄 and I trust the testing that has been made.

Question about the modified events: why are we changing legacy events as well? And did not changes to events require upcasting? 🤔

lib/trento/clusters/value_objects/sap_instance.ex

arbulu89 · 2025-03-25T12:31:38Z

Question about the modified events: why are we changing legacy events as well? And did not changes to events require upcasting?

Indeed. Changing legacy events was not needed. I thought that the params would be superseded, but they are not, we simply redirect to the new module. Good catch.

About the upcasting, as the embeds_many defaults to an empty list, everything works without it. It is true that we lose some partial data though. I will implement the upcasting just for completeness, but the result would be the same due eventual consistency.

arbulu89 · 2025-03-25T13:51:20Z

Hey @nelsonkopliku ,
About the upcasting again. I have done some coding and adding "advanced" upcasting code looks unnecessary again. At the end, we just simply update some partial incomplete data in the aggregate, that we don't even use there (it is there only for projection purposes). And this data will be overriden 100% cases when the next payload arrives. So it is not used for anything.

I think that I will avoid adding not needed extra complexity/code and simply bump the version of the event, at least to be informative (because if there is not any upcasting function, it doesn't really do anything).
What do you think?

PD: I'm going to even remove the version bump. This forces me to have the upcast function implemented, and it doesn't make much sense as it wouldn't do any change at all in the coming parameters, returning the same object

nelsonkopliku · 2025-03-25T13:53:48Z

Sounds reasonable @arbulu89, thanks for looking into it.

nelsonkopliku

LGTM

gagandeepb

Just one comment/side-note.

gagandeepb · 2025-03-25T14:35:39Z

lib/trento/clusters.ex

+    case Repo.one(query) do
+      nil -> []
+      sap_instances -> sap_instances
+    end


This will raise an exception if there's more than one result, IIRC.

This is the host_id, UUID and primary key in the database. It is impossible by database definition as the host can only have one cluster.
But thank you for pointing it out, you never know until you double-check hehe

arbulu89 added the env label Mar 20, 2025

arbulu89 commented Mar 20, 2025

View reviewed changes

arbulu89 added the regression label Mar 20, 2025

arbulu89 commented Mar 20, 2025

View reviewed changes

arbulu89 force-pushed the identify-clustered-sap-instances branch from af24c5e to 30ff192 Compare March 24, 2025 08:39

arbulu89 added 5 commits March 24, 2025 15:06

Identify clustered sap instances

e98f0c7

Move application instance only when clustered

7cb643b

Remove not used sid and additional_sids fields

ca07aed

Update and adapt existing tests

5d821bf

Add new tests

dba7f30

arbulu89 force-pushed the identify-clustered-sap-instances branch from 30ff192 to dba7f30 Compare March 24, 2025 14:06

Remove duplicated code

55dadfd

arbulu89 requested review from nelsonkopliku, gagandeepb and abravosuse March 24, 2025 14:49

arbulu89 marked this pull request as ready for review March 24, 2025 14:56

abravosuse approved these changes Mar 24, 2025

View reviewed changes

nelsonkopliku reviewed Mar 25, 2025

View reviewed changes

lib/trento/clusters/value_objects/sap_instance.ex Outdated Show resolved Hide resolved

arbulu89 added 2 commits March 25, 2025 13:23

Fix function spec

c2f21b8

Undo changes in legacy events

3b131a4

arbulu89 requested a review from nelsonkopliku March 25, 2025 13:55

nelsonkopliku approved these changes Mar 25, 2025

View reviewed changes

arbulu89 force-pushed the identify-clustered-sap-instances branch from 1828cf5 to 3b131a4 Compare March 25, 2025 14:03

gagandeepb approved these changes Mar 25, 2025

View reviewed changes

arbulu89 merged commit bef97ba into main Mar 25, 2025
39 checks passed

arbulu89 deleted the identify-clustered-sap-instances branch March 25, 2025 15:21

arbulu89 mentioned this pull request Mar 26, 2025

Expose cluster sap instances frontend #3410

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Identify clustered sap instances #3397

Identify clustered sap instances #3397

arbulu89 commented Mar 20, 2025 •

edited

Loading

arbulu89 Mar 20, 2025

arbulu89 Mar 20, 2025

abravosuse left a comment

nelsonkopliku left a comment

arbulu89 commented Mar 25, 2025 •

edited

Loading

arbulu89 commented Mar 25, 2025 •

edited

Loading

nelsonkopliku commented Mar 25, 2025

nelsonkopliku left a comment

gagandeepb left a comment

gagandeepb Mar 25, 2025

arbulu89 Mar 25, 2025 •

edited

Loading

Identify clustered sap instances #3397

Identify clustered sap instances #3397

Conversation

arbulu89 commented Mar 20, 2025 • edited Loading

Description

How was this tested?

arbulu89 Mar 20, 2025

Choose a reason for hiding this comment

arbulu89 Mar 20, 2025

Choose a reason for hiding this comment

abravosuse left a comment

Choose a reason for hiding this comment

nelsonkopliku left a comment

Choose a reason for hiding this comment

arbulu89 commented Mar 25, 2025 • edited Loading

arbulu89 commented Mar 25, 2025 • edited Loading

nelsonkopliku commented Mar 25, 2025

nelsonkopliku left a comment

Choose a reason for hiding this comment

gagandeepb left a comment

Choose a reason for hiding this comment

gagandeepb Mar 25, 2025

Choose a reason for hiding this comment

arbulu89 Mar 25, 2025 • edited Loading

Choose a reason for hiding this comment

arbulu89 commented Mar 20, 2025 •

edited

Loading

arbulu89 commented Mar 25, 2025 •

edited

Loading

arbulu89 commented Mar 25, 2025 •

edited

Loading

arbulu89 Mar 25, 2025 •

edited

Loading