Make discovery payload nullable in schema

[arbulu89]

Description

Make discovery payload nullable in schema.
This field can be null when for example a host doesn't have a cluster installed.

This bug affects the scenario when a cluster node is removed from the cluster, because we wouldn't remove it properly as the discovery fails. This is unlikely scenario. But it happens.
Besides that, we remove an "wrong" error log in the agent side.
This error in the agent:

11:49:26.350 request_id=F9EwDTjMRMb4yvsAB4oR [info] POST /api/v1/collect
11:49:26.350 request_id=F9EwDTjMRMb4yvsAB4oR [debug] Processing with TrentoWeb.V1.DiscoveryController.collect/2
  Parameters: %{"agent_id" => "8b8f6760-e9f5-5014-893d-b154c537e964", "discovery_type" => "ha_cluster_discovery", "payload" => nil}
  Pipelines: [:api, :apikey_authenticated, :api_v1]
11:49:26.358 request_id=F9EwDTjMRMb4yvsAB4oR [debug] QUERY OK source="settings" db=6.1ms queue=0.4ms idle=249.3ms
SELECT s0."id", s0."api_key_settings_jti", s0."api_key_settings_created_at", s0."api_key_settings_expire_at", s0."inserted_at", s0."updated_at", s0."type" FROM "settings" AS s0 WHERE (s0."type" = 'api_key_settings') []
11:49:26.358 request_id=F9EwDTjMRMb4yvsAB4oR [info] Sent 422 in 7ms
11:49:26.358 request_id=F9EwDTjMRMb4yvsAB4oR [debug] TrentoWeb.V1.DiscoveryController halted in OpenApiSpex.Plug.CastAndValidate.call/2

FYI: @abravosuse

How was this tested?

Test added

[abravosuse]

If it is possible to have a PR environment for this, I will test the corresponding image. If not, I approve it now and will test in rolling once merged.

[arbulu89]

If it is possible to have a PR environment for this, I will test the corresponding image. If not, I approve it now and will test in rolling once merged.

PR env on the way!

[dottorblaster]

It looks good, maybe I'm saying something really weird but shouldn't we also apply some kind of check in the agent to guard against sending nil payloads?

Anyway, if it's just a matter of adding a nullable: true to a schema I'm pretty much in favor of this 👍

[nelsonkopliku]

LGTM

[arbulu89]

@al

It looks good, maybe I'm saying something really weird but shouldn't we also apply some kind of check in the agent to guard against sending nil payloads?

Anyway, if it's just a matter of adding a nullable: true to a schema I'm pretty much in favor of this 👍

But sending null is an existing scenario. For example in this case, that you don't have a cluster. And we work based on this nil.
If we would like to change the behaviour, we would need to change agent and web code, both. And not only the schema

[dottorblaster]

You're right @arbulu89, no other questions 👍

[abravosuse]

I've tested this with the PR environment. Now I don't see errors in the agent journal. What I see is the following:
May 20 13:32:05 vmnwhso01 trento-agent[2238]: time="2024-05-20 13:32:05" level=info msg="ha_cluster_discovery discovery tick output: No HA cluster discovered on this host"

[rtorrero]

Nice catch @arbulu89 @abravosuse ! LGTM

[EMaksy]

👍

[gagandeepb]

Question/comment : If I understood this PR correctly OpenAPI 3.1 has no support for nullable. However, I am not sure to what version of the OpenAPI spec the elixir library implementation (open-api-spex; we are using version 3.11 of this library) maintains conformance. Something that needs further looking into, perhaps ?

[arbulu89]

Question/comment : If I understood this PR correctly OpenAPI 3.1 has no support for nullable. However, I am not sure to what version of the OpenAPI spec the elixir library implementation (open-api-spex; we are using version 3.11 of this library) maintains conformance. Something that needs further looking into, perhaps ?

By now it is compliant to 3.0.x version set.
So we should be fine.
If they change, we would need to apply more changes I guess.
But good point. We will need to have an eye on it