Skip to content

WIP: rework accounting scripts - #288

Draft
anish-mudaraddi wants to merge 6 commits into
masterfrom
wip-accounting
Draft

WIP: rework accounting scripts #288
anish-mudaraddi wants to merge 6 commits into
masterfrom
wip-accounting

Conversation

@anish-mudaraddi

@anish-mudaraddi anish-mudaraddi commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

make accounting scripts run as a docker container

requirements:

  1. the docker container must run continuously - to get accounting data every interval

  2. also, the container can be used to collect past accounting data by providing timestamps from the past.

  3. there should be good logs so it can run on kayobe and be debugged easily

  4. there should be a dry-run feature - so we can test improvements in the future without sending data anywhere

  5. there should be good testing and linting to keep up with our coding standards

  6. it should work on a supported python version and the dependencies should be up-to-date

Still TODO:

  • more testing on monitoring nodes
  • acceptance testing on dev
  • automated CI to run test
  • moving this out into https://github.com/stfc/cloud-image-builders
    so that we can auto-build and version the docker container
  • getting a playbook together in our kayobe config to install this alongside monitoring containers
  • setting up perms on dev db to allow these scripts to run on monitoring nodes

@gmatthews20 gmatthews20 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I haven't yet reviewed any of the tests. Will take a look once we're happy with the functionality

Comment on lines +8 to +10
# Get log level from environment, defaulting to DEBUG
log_level_name = os.getenv("THECOUNT_LOG_LEVEL", "INFO").upper()
log_level = getattr(logging, log_level_name, logging.INFO)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The default level is INFO no?

Comment on lines +45 to +46
"--start-time", metavar="TIME", default=None,
help="start of the range (default: current time) ISO 8601 format( yyyy-mm-ddTHH:MM:SS )",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

help statement says default is current time but default is set to None?

Comment on lines +95 to +97
interval_regex = re.compile(
r'((?P<days>\d+?)d)?((?P<hours>\d+?)h)?((?P<minutes>\d+?)m)?((?P<seconds>\d+?)s)?'
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of trying to parse different formats as regex why not just force the user to provide the interval in minutes?

Comment on lines +27 to +31
source: Source object - used to fetch data
start_time: python datetime - the start time of the job
end_time: python datetime - the end time of the job
dry_run: boolean - whether to run the job in dry run mode - i.e. print to console instead of sending
it to source (useful for debugging)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You're missing sink from the docstring. Also I think a better description of what source and sink are is needed

@staticmethod
def _transform(rows: List[Dict[str, Any]], end_time: datetime, instance) -> str:
"""
transform cinder data from source to get total amount of GB per second usage for block-storage per project

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

GB per second sounds like throughput, I think it should be GB seconds


def _load_config(self, config_fp: str) -> None:
"""
Helper function to read theobjectcount config file and extrac source-related config

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is theobjectcount correct?

Suggested change
Helper function to read theobjectcount config file and extrac source-related config
Helper function to read thecount config file and extract source-related config

Comment on lines +1 to +2
# Use Python 3.12 slim image as base
FROM python:3.12-slim

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why was version 3.12 chosen, any reason we couldn't use something newer like 3.14?

RUN pip install --no-cache-dir .

# Create a non-root user
RUN useradd --create-home --shell /bin/bash monitoring

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Don't think we need to create a home dir for the user?

Suggested change
RUN useradd --create-home --shell /bin/bash monitoring
RUN useradd --shell /bin/bash monitoring

requires-python = ">=3.10"
keywords = ["python", "openstack"]
classifiers = [
"Development Status :: 4 - Beta",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Version 1.0.0 is a beta version?

Comment on lines +7 to +11
User with read access to the relevant databases for each OpenStack component you are accounting for - right now:
- cinder
- manila
- nova
- glance

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do the correct SQL procedures need to exist as well? How are these set up/kept?

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.

2 participants