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 Kubernetes collector #836

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

rkleinem
Copy link
Collaborator

@rkleinem rkleinem commented Jun 3, 2024

A first version of the Kubernetes collector.

Still to do:

  • Helm charts
  • Document charts
  • Dockerfile
  • Check whether an integration test is feasible

I found that it doesn't make much sense to use the queued client for this because we need a separate persistent storage anyway. The client would just be a second one.

Note:
I made a little change to AUDITOR: use the Display formatter for the Display implementation of ValidName. Is this ok? If not I can revert this and apply a workaround in the collector.

@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2024

Codecov Report

Attention: Patch coverage is 66.70574% with 284 lines in your changes missing coverage. Please review.

Project coverage is 65.38%. Comparing base (a04358a) to head (5013480).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #836      +/-   ##
==========================================
+ Coverage   64.90%   65.38%   +0.48%     
==========================================
  Files          49       56       +7     
  Lines        6211     7063     +852     
  Branches       64       66       +2     
==========================================
+ Hits         4031     4618     +587     
- Misses       2180     2445     +265     
Components Coverage Δ
apel_plugin 78.46% <ø> (ø)
auditor 70.36% <100.00%> (+1.51%) ⬆️
auditor_client 94.38% <ø> (ø)
slurm_collector 71.91% <ø> (ø)
slurm_epilog_collector 0.00% <ø> (ø)
htcondor_collector ∅ <ø> (∅)
priority_plugin 37.82% <ø> (ø)

Copy link
Collaborator

@raghuvar-vijay raghuvar-vijay left a comment

Choose a reason for hiding this comment

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

Thanks a lot:-) I've added a few minor comments.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Local dependencies can be transitioned to workspace dependencies

if ! [ -x "$(command -v sqlx)" ]; then
echo >&2 "Error: sqlx is not installed."
echo >&2 "Use:"
echo >&2 " cargo install --version=0.7.2 sqlx-cli --no-default-features --features postgres,rustls,sqlite"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This version number is different from the Cargo.toml sqlx dependency version number (v0.7.4)

use serde::Deserialize;
use tracing_subscriber::filter::LevelFilter;

pub fn load_configuration(p: impl AsRef<Path>) -> Config {
Copy link
Collaborator

Choose a reason for hiding this comment

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

load_configuration can be modified to return Result<Config, config::ConfigError>

@rkleinem
Copy link
Collaborator Author

Will do another commit in an hour or so

@rkleinem
Copy link
Collaborator Author

Hi, I guess it got lost on mattermost, so I will post it here.
There are two questions about the CI chain:

  • For the container build: Adding auditor-kubernetes-collector to the list containers in docker-build-push.yml:23 is all I need to do, right?
  • Also, what happens in the migration test? Doesn't test_db_migration.yml:63 override the version we want to test?

raghuvar-vijay
raghuvar-vijay previously approved these changes Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants