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.

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

Add claimed_id for child nodes streamed to their parents #9804

Merged
merged 24 commits into from
Aug 26, 2020

Conversation

underhood
Copy link
Contributor

@underhood underhood commented Aug 21, 2020

Summary

claimed_id of nodes is streamed to parents. For easy testing also added to /api/v1/info (where cloud will need it anyway sooner or later)
Example of 3-level streaming from the view of top-level parent:
Screenshot from 2020-08-25 11-38-41

Component Name

Streaming

Test Plan

Have some streaming setup and look at /api/v1/info. Check nothing breaks with Cloud by connecting this node to staging.netdata.cloud. Claim nodes and see if their claiming Id propagates up the network by checking /api/v1/info of the topmost node. All nodes have to be running this PR for it to work.

What was tested together with the cloud team:

  1. Claim node to Staging and connect it. Node had 2 children.
  2. Message as per screenshot above (with new mirrored_hosts_status field) is send to cloud as part of onconnect payload over ACLK
  3. Checked Cloud GUI still works as expected with this new fields
  4. Cloud team checked there are no errors in the background (not visible to us from GUI)

What was tested additionally:

  • node that is claimed during runtime and then netdatacli reload-claiming-state
  • up to 4 level hierarchy of streaming
    • TopLevelMaster (VM,not claimed)
    • timowss (real machine,claimed) -> TopLevelMaster (VM)
    • Fed32-Layer1Host1 (VM,claimed) -> timowss (real machine, claimed)
    • Fed32-Layer1Host2 (VM,not claimed) -> timowss (real machine, claimed)
    • Fed32-Layer2Host1 (VM,not claimed) -> Fed21-Layer1Host1 (VM, claimed)
    • this represents final test with biggest amount of hosts tested, during development and testing different combination of nodes and claiming states were used
  • both claimed and unclaimed nodes have correct status reported
Additional Information

Codacy false triggers here. It doesn't seem to recognize rrdhost_foreach_read macro is a loop and as a result, thinks if(count > 0) { is always false (which is not true).

claim/claim.c Outdated Show resolved Hide resolved
streaming/receiver.c Outdated Show resolved Hide resolved
streaming/rrdpush.c Outdated Show resolved Hide resolved
web/api/web_api_v1.c Outdated Show resolved Hide resolved
@amoss
Copy link
Contributor

amoss commented Aug 21, 2020

Overall it looks ok, I know it is still [WIP] - I'll do a review after the version negotiation changes.

claim/claim.c Outdated Show resolved Hide resolved
streaming/receiver.c Outdated Show resolved Hide resolved
web/api/web_api_v1.c Show resolved Hide resolved
@underhood
Copy link
Contributor Author

underhood commented Aug 25, 2020

@amoss
ZO_12i8m38RlVOgm-mwjDr7OIJpCcoyGjuYJr4gJAO8FhyLDN57myfU4VlnVAb6CsdisONUaHc4RpZLTY3SeIj5CHvX3mzXvQc49UsIX4IjMrCvSMUEDebq7KLi02h0zM9DYGchitw1sFbn5pF3c_tuNkB70cHJ_gRaFDVTKI_65pCzUtCioDtqjqstPPZ2omvXgAZgrlUS7

this is topology of the 4 layer test However during testing I tried different combinations (especially with claiming/unclaiming nodes) and topologies

@netdata netdata deleted a comment from ktsaou Aug 25, 2020
@amoss
Copy link
Contributor

amoss commented Aug 25, 2020

Ok good, now the test plan is comprehensive. I'll let the other reviewer take a look before I go back for a second look at the code.

@amoss
Copy link
Contributor

amoss commented Aug 25, 2020

I've disabled the Codacy warnings as they are just noise - it does not understand the loop macro for some reason.

@netdata netdata deleted a comment from ktsaou Aug 25, 2020
@underhood underhood requested review from mfundul and amoss August 25, 2020 10:36
web/api/web_api_v1.c Outdated Show resolved Hide resolved
streaming/receiver.c Outdated Show resolved Hide resolved
web/api/web_api_v1.c Outdated Show resolved Hide resolved
web/api/web_api_v1.c Outdated Show resolved Hide resolved
@underhood
Copy link
Contributor Author

underhood commented Aug 26, 2020

@Ferroin arch CI build started failing yesterday with unrelated error:
configure: error: libuv required but not found. Try installing 'libuv1-dev' or 'libuv-devel'.

Can you take a look if you have 5'

@underhood underhood requested review from amoss and mfundul August 26, 2020 07:40
@amoss amoss dismissed their stale review August 26, 2020 07:50

stale

@underhood
Copy link
Contributor Author

I checked also with production that it doesnt kill something just to be paranoid before merge.

@underhood underhood merged commit ab7ff31 into netdata:master Aug 26, 2020
@underhood underhood deleted the stream_cid branch August 26, 2020 13:16
@joelhans joelhans changed the title Stream Claim Id Add claimed_id for child nodes streamed to their parents Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants