Cluster deregistered side effects

[CDimonaco]

Description

ClusterReadModel soft delete side effects.

We should exclude from the query in which is involved the ClusterReadModel, the soft-deleted entities.

This Schema is not involved in any association.

How was this tested?

Automated tests

[arbulu89]

I miss some more thorough test for the clusters use case honestly. For example, i think that the request_clusters_checks_execution condition is wrong, so the test would be helpful there.

Besides that, I think the HostRemovedFromCluster in the hosts projection is missing.
There, we should put the cluster_id of the host to nil.
I know that this cannot happen when you run the HostDeregistrationRequest, but if you deregister the cluster from the cli (or in the future with the delta deregistration), this scenario must be considered.

[CDimonaco]

@arbulu89 I addressed the feedbacks but I don't really understand what do you mean here

Besides that, I think the HostRemovedFromCluster in the hosts projection is missing.
There, we should put the cluster_id of the host to nil.
I know that this cannot happen when you run the HostDeregistrationRequest, but if you deregister the cluster from the cli (or in the future with the delta deregistration), this scenario must be considered.

[fabriziosestito]

Overall great work! Some nitpick and I agree with @arbulu89 comment about the HostRemovedFromCluster projection.

[arbulu89]

@arbulu89 I addressed the feedbacks but I don't really understand what do you mean here

Besides that, I think the HostRemovedFromCluster in the hosts projection is missing.
There, we should put the cluster_id of the host to nil.
I know that this cannot happen when you run the HostDeregistrationRequest, but if you deregister the cluster from the cli (or in the future with the delta deregistration), this scenario must be considered.

In one of the latest #1274 we have included the HostRemovedFromCluster event. This event happens when a host is removed from the cluster, that can lead to a total cluster deregistration, if it was the last host in the cluster. To host/cluster relationship in the frontend read-models is done with the cluster_id field in the host read model. So as far as we have removed the host, we should put this field to nil, so it reflects the real state of the host.

This is important if we have a standalone cluster de-registration (without de-registering the host itself).

At the end we need to revert what we do here: https://github.com/trento-project/web/blob/deregistration/lib/trento/application/projectors/host_projector.ex#L66

[rtorrero]

Besides from what's already been commented, it looks good @CDimonaco 👍

[CDimonaco]

@arbulu89 @fabriziosestito Done, please review again

[arbulu89]

Great!
I proposed other alternative, but it should do the same (as the test passes hehe)

[arbulu89]

Maybe this looks better:

fn multi ->
  changeset =
    HostReadModel.changeset(%HostReadModel{id: id, cluster_id: ""}, %{
      cluster_id: nil
    })

    Ecto.Multi.update(multi, :host, changeset)
  end

At the end you simply want to put the cluster_id to nil for this host

[CDimonaco]

Yes but no, I mean we don't know the order of the events, and we want to make sure to not update anything that is already soft delete, to mantain consistency and logic.

This guard will just make that happen. Right know I know that this could potentially be a no-op, but I don't really like the idea to pollute deleted entities :D

Anyway if you like it we can merge.

[arbulu89]

I don't really think this could ever happen. You cannot de-register a host without de-registering it first from a cluster (and in any case, what you "say" about polluting would be reflecting the source of truth in that strange scenario XD)
I guess that the cost of doing an extra query is low.
Anyway, you already had my green light hehe

[fabriziosestito]

@arbulu89 @CDimonaco I think it's my fault on this one.
We cannot guarantee the order of the events, since HostRemovedFromCluster is emitted by the Cluster aggregate and might arrive after the HostDeregistered event which is emitted concurrently by the Host aggregate. I was confused and thought we were deleting the entry for good, therefore my suggestion was to use update_all.

I still prefer Xabi's solution since the host is just soft deleted so the read model can be found. It doesn't matter if we update a soft deleted read model, actually, we must update it to have a consistent entry if we then plan to roll back the deregistration.

[arbulu89]

I don't think you need to insert this cluster

[CDimonaco]

I strongly disagree on updating a soft delete model, if this is the only way we have to rollback I will do it but it’s maybe the symptom of other design problems. Anyway, for the sake of pr I will update the code so we can close