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

feat: passive runner deletion #294

Merged
merged 15 commits into from
Jun 17, 2024

Conversation

yanksyoon
Copy link
Collaborator

@yanksyoon yanksyoon commented Jun 6, 2024

Applicable spec: N/A

Rejected:

  • Alert logging: The alert logging is for debugging purposes and should be setup via grafana-dashboard with datasource as Loki.

Overview

  1. Refactor the module to be step-down ordering to follow IS-Charms contributing guide.
  2. Refactor _get_ssh_connection function to return a single working SSH connection instance. (There is no need to loop over every possible network connections and try them for each business logic).
  3. Create a general _health_check function that encapsulates the business logic of health checking for both startup and running health checks.
  4. Log and alert when there is an unexpected health check result to allow more insights into the state of Openstack GitHub runners.

Rationale

To gain more insights into interactions with Openstack.

Juju Events Changes

None.

Module Changes

  • openstack_manager.py
    • refactored to step-down function ordering
    • added _health_check function
    • refactored _get_ssh_connection function

Library Changes

None.

Checklist

Testing has been one on PS6
image

Returns:
An SSH connection to OpenStack server instance.
"""
server: Server | None = conn.get_server(name_or_id=server_name)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Refresh server on error (e.g. network not yet attached)

@staticmethod
def _get_key_path(name: str) -> Path:
"""Get the filepath for storing private SSH of a runner.
def reconcile(self, quantity: int) -> int:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Step down ordering of methods.

@yanksyoon yanksyoon changed the title [WIP] status alert log feat: passive runner deletion Jun 14, 2024
]

@staticmethod
def _health_check(conn: OpenstackConnection, server_name: str, startup: bool = False) -> bool:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Main change


@staticmethod
def _ssh_health_check(server: Server) -> bool:
def _ssh_health_check(conn: OpenstackConnection, server_name: str, startup: bool) -> bool:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Main change


@staticmethod
@retry(tries=3, delay=5, max_delay=60, backoff=2, local_logger=logger)
def _get_ssh_connection(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Improved _get_ssh_connection function

Copy link
Collaborator Author

@yanksyoon yanksyoon left a comment

Choose a reason for hiding this comment

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

Comments to assist in PR review

Copy link
Collaborator

@cbartz cbartz left a comment

Choose a reason for hiding this comment

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

Minor comments and a question. Thanks!

src/openstack_cloud/openstack_manager.py Show resolved Hide resolved
tests/unit/test_openstack_manager.py Outdated Show resolved Hide resolved
tests/unit/test_openstack_manager.py Outdated Show resolved Hide resolved
@yanksyoon yanksyoon marked this pull request as ready for review June 14, 2024 09:35
@yanksyoon yanksyoon requested a review from a team as a code owner June 14, 2024 09:35
Copy link
Contributor

@amandahla amandahla left a comment

Choose a reason for hiding this comment

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

LGTM

@yanksyoon yanksyoon merged commit 72ebea6 into feat/openstack-integration Jun 17, 2024
3 of 9 checks passed
@yanksyoon yanksyoon deleted the chore/status_alert_log branch June 17, 2024 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants