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

reinstate hive health metrics #32603

Merged
merged 5 commits into from
May 31, 2024

Conversation

bimmlerd
Copy link
Member

As part of #32020, the hive module health metrics were accidentially dropped. Reinstate them in a new form, and add a test for the functionality.

@bimmlerd bimmlerd added release-note/misc This PR makes changes that have no direct user impact. sig/agent Cilium agent related. area/modularization labels May 17, 2024
@maintainer-s-little-helper

This comment was marked as resolved.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 17, 2024
@bimmlerd bimmlerd force-pushed the pr/bimmlerd/hive-health-metrics branch from 02d445d to b7af21f Compare May 17, 2024 11:35
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 17, 2024
@bimmlerd bimmlerd force-pushed the pr/bimmlerd/hive-health-metrics branch 4 times, most recently from 3281e3b to a285970 Compare May 17, 2024 12:25
@bimmlerd bimmlerd marked this pull request as ready for review May 17, 2024 12:31
@bimmlerd bimmlerd requested review from a team as code owners May 17, 2024 12:31
@bimmlerd
Copy link
Member Author

/test

@bimmlerd
Copy link
Member Author

ah, CI found a legitimate issue - marking as draft for now, sorry for the noise

@bimmlerd bimmlerd marked this pull request as draft May 17, 2024 12:55
@bimmlerd bimmlerd force-pushed the pr/bimmlerd/hive-health-metrics branch 4 times, most recently from 2901d35 to 4d7d063 Compare May 22, 2024 08:01
@bimmlerd
Copy link
Member Author

/test

@bimmlerd bimmlerd marked this pull request as ready for review May 23, 2024 14:44
@bimmlerd bimmlerd force-pushed the pr/bimmlerd/hive-health-metrics branch from 4d7d063 to 02140ac Compare May 24, 2024 08:59
@bimmlerd
Copy link
Member Author

/test

Copy link
Member

@nebril nebril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops!

Copy link
Member

@dylandreimerink dylandreimerink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bimmlerd bimmlerd force-pushed the pr/bimmlerd/hive-health-metrics branch from 02140ac to 06ba3fa Compare May 29, 2024 08:42
@bimmlerd bimmlerd requested a review from a team as a code owner May 29, 2024 08:42
@bimmlerd bimmlerd requested review from marseel and removed request for danehans May 29, 2024 08:42
@bimmlerd
Copy link
Member Author

/test

Moves the healthv2 implementation to hive/health to get rid of the
somewhat ugly v2 part of the pkg name, and to give some logical
structure.

Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
Pulling this bugfix in manually to ensure we don't hit race conditions
after the next commit.

Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
With the introduction of the hive module health metric publisher in a
subsequent path, a precondition of the node manager EmitStatus test will
break: the test assumes the node manager to be the only writer to the
status table. Without this patch, a race occurs.

Since it seems likely that the set of default hive cells will grow (and
that they may want to update their status too) change the test so that
it no longer assumes exclusivity. It now looks specifically at its own
status, and uses revisions to understand when the status changes.

Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
Somewhere in the move from pkg/hive to cilium/hive we lost the module
health metrics. Add them back in the new form of a query on the status
table, and add a test.

Fixes: 2311f3d (treewide: rebase on cilium/hive)

Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
This ensures that logging output of the metrics job and the rest of the
health subsystem are piped through the same logging mechanism. In
addition, unexport what need not be exported, and avoid some amount of
stuttering in "health.HealthTableName", for example.

Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
@bimmlerd bimmlerd force-pushed the pr/bimmlerd/hive-health-metrics branch from 06ba3fa to 79e1b96 Compare May 29, 2024 12:01
@bimmlerd
Copy link
Member Author

/test

@bimmlerd
Copy link
Member Author

@marseel if you could take a look not only at the vendor stuff but also the mildly adapted test, I believe you've recently worked on the bg node sync stuff (in node/manager: test: avoid future race in status)

Copy link
Contributor

@marseel marseel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from vendor point of view, one non-blocking idea for the node manager test.

pkg/node/manager/manager_test.go Show resolved Hide resolved
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 31, 2024
@joamaki joamaki enabled auto-merge May 31, 2024 13:29
@joamaki joamaki added this pull request to the merge queue May 31, 2024
Merged via the queue into cilium:main with commit 2af18f1 May 31, 2024
64 checks passed
@bimmlerd bimmlerd deleted the pr/bimmlerd/hive-health-metrics branch May 31, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/modularization ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/agent Cilium agent related.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants