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

runner service refactor #230

Merged
merged 4 commits into from
Jan 3, 2025
Merged

runner service refactor #230

merged 4 commits into from
Jan 3, 2025

Conversation

ganisback
Copy link
Collaborator

@ganisback ganisback commented Jan 3, 2025

What is this feature?

refactor runner service events receiving with k8s informer

Why do we need this feature?

improve the performance for runner

Who is this feature for?

csghub user

Which issue(s) does this PR fix?:

Fixes #

Special notes for your reviewer:

MR Summary:

The summary is added by @codegpt.

This Merge Request introduces significant performance improvements and new features to the runner service, particularly focusing on Kubernetes (K8s) service management and Argo workflow handling. Key changes include:

  1. Refactoring for Performance: The runner service has been refactored to improve performance. This includes enhancements in handling Kubernetes services and Argo workflows more efficiently.

  2. Kubernetes Service Management Enhancements: The service component now supports operations like running, stopping, purging, and updating Kubernetes services. It also includes the ability to generate Kubernetes services dynamically based on requests, manage persistent volume claims, and retrieve service pods with their status.

  3. Argo Workflow Handling Improvements: The workflow component has been enhanced to support creating, updating, deleting, and retrieving Argo workflows. It also includes functionality to handle workflow events and perform accounting for resource usage.

  4. Informer Integration: Informers have been integrated for both Kubernetes services and Argo workflows to watch for changes and handle them accordingly. This allows for real-time updates and management of services and workflows.

  5. API Endpoints for Service and Workflow Management: New API endpoints have been introduced to manage Kubernetes services and Argo workflows, including operations like creating, stopping, updating, and getting the status of services and workflows.

  6. Error Handling and Logging: Improved error handling and logging mechanisms have been implemented to ensure robustness and ease of debugging.

  7. Configuration and Dependency Updates: Updates to configurations and dependencies to support the new features and improvements.

This MR aims to enhance the scalability, efficiency, and manageability of Kubernetes services and Argo workflows within the runner service, providing a more robust and performant platform for users.

@ganisback ganisback changed the title performance tunning for runner service runner service refactor Jan 3, 2025
@starship-github
Copy link

Linter Issue Report

During the code review, a list issues were found. These issues could affect the code quality, maintainability, and consistency. Below is the detailed Linter issue report:

builder/store/database/knative_service.go

Lint Issue: undefined: DB

  • Location: Line 12, Column 6
  • Code Snippet:
    type knativeServiceImpl struct {
        db *DB
    }
  • Suggestion: It seems like the DB type is not defined within this file or imported from another package. Please ensure that you have defined the DB type or imported the package where DB is defined. If DB is supposed to be part of an external package, include the correct import statement at the top of your file.

Lint Issue: undefined: times

  • Location: Line 50, Column 2
  • Code Snippet:
    type KnativeService struct {
        ID             int64                  `bun:",pk,autoincrement" json:"id"`
        ...
        OrderDetailID  int64                  `bun:",\" json:\"order_detail_id\""`
        times
    }
  • Suggestion: The times field appears to be undefined. If it's meant to be a struct or a type representing time-related data, please define it or import the necessary package. If times is a typo or an incomplete code snippet, either remove it or replace it with the intended code.

Lint Issue: undefined: defaultDB

  • Location: Line 25, Column 7
  • Code Snippet:
    func NewKnativeServiceStore() KnativeServiceStore {
        return &knativeServiceImpl{
            db: defaultDB,
        }
    }
  • Suggestion: The variable defaultDB is not defined in this scope. Ensure that defaultDB is correctly defined and accessible in this context. If it's defined in another package, make sure to import that package. Alternatively, if defaultDB is meant to be a global variable or a constant, verify that it has been declared and initialized properly before use.
builder/store/database/migrations/20241225124808_create_table_knative_service.go

Lint Issue: undefined: times

  • Location: Line 27, Column 2
  • Code Snippet:
    \tOrderDetailID  int64                  `bun:\",\" json:\"order_detail_id\"`\n\ttimes\n}\n
  • Suggestion: It seems like times is intended to be a field or a struct but is not defined anywhere in the code. If times is meant to be a struct containing timestamp fields (like created_at, updated_at), ensure that it is correctly defined and imported. Alternatively, if times is not needed, consider removing this line.

Lint Issue: undefined: Migrations

  • Location: Line 31, Column 2
  • Code Snippet:
    \nfunc init() {\n\tMigrations.MustRegister(func(ctx context.Context, db *bun.DB) error {\n
  • Suggestion: The Migrations object is not defined within this file or imported from external packages. Ensure that you have a package that defines Migrations and that it is correctly imported. If it's a custom implementation, verify that the package path is correct and accessible.

Lint Issue: undefined: createTables

  • Location: Line 32, Column 10
  • Code Snippet:
    \t\terr := createTables(ctx, db, KnativeService{})\n
  • Suggestion: The function createTables is not defined or imported. If it's a custom function intended to create database tables, ensure that it is correctly implemented and imported. If it's part of an external package, check the import statements for accuracy.

Lint Issue: undefined: dropTables

  • Location: Line 52, Column 10
  • Code Snippet:
    \t\treturn dropTables(ctx, db, KnativeService{})\n\t})\n}\n
  • Suggestion: Similar to createTables, the dropTables function is missing. Ensure that if it's a custom function for dropping database tables, it is correctly defined and imported. Verify the import paths and the existence of the function in the codebase.

Please make the suggested changes to improve the code quality.

@starship-github
Copy link

Possible Issues And Suggestions:

  • Line 250 in runner/component/service.go

    • Comments:
      • Using 'continue' inside a loop without checking a specific condition may lead to infinite loops if not carefully managed.
  • Line 68 in builder/store/database/knative_service.go

    • Comments:
      • The 'On("CONFLICT(name, cluster_id) DO UPDATE")' clause in 'Add' method might not work as expected without specifying the 'SET' part.
    • Suggestions:
      // Ensure to specify what happens on conflict, for example:
      _, err := s.db.Operator.Core.NewInsert().Model(service).On("CONFLICT(name, cluster_id) DO UPDATE SET column = EXCLUDED.column").Exec(ctx)
      
  • Line 40 in builder/store/database/migrations/20241225124808_create_table_knative_service.go

    • Comments:
      • The migration script attempts to create an index after altering the table but does not specify what to do in case of failure, potentially leaving the database in an inconsistent state.
    • Suggestions:
      // Consider wrapping table alterations and index creations in a transaction to ensure atomicity and rollback on failure.
      tx, err := db.BeginTx(ctx, nil)
      if err != nil {
          return err
      }
      // Perform table alterations and index creations within the transaction
      if err := tx.Commit(); err != nil {
          return err
      }
      
  • builder/deploy/deployer.go

    • Comments:
      • Directly returning 'true' without checking the response code in 'Exist' function might lead to incorrect assumptions about service existence.

MR Evaluation:

This feature is still under test, evaluation are given by AI and might be inaccurate.

After evaluation, the code changes in the Merge Request get score: 90.

Analysis for the evaluation score:
  • The code change may not include corresponding unit tests.
  • The code change may not include corresponding integration testing.
Tips

CodeReview Commands (invoked as MR or PR comments)

  • @codegpt /review to trigger an code review.
  • @codegpt /evaluate to trigger code evaluation process.
  • @codegpt /describe to regenerate the summary of the MR.
  • @codegpt /secscan to scan security vulnerabilities for the MR or the Repository.
  • @codegpt /help to get help.

CodeReview Discussion Chat

There are 2 ways to chat with Starship CodeReview:

  • Review comments: Directly reply to a review comment made by StarShip.
    Example:
    • @codegpt How to fix this bug?
  • Files and specific lines of code (under the "Files changed" tab):
    Tag @codegpt in a new review comment at the desired location with your query.
    Examples:
    • @codegpt generate unit testing code for this code snippet.

Note: Be mindful of the bot's finite context window.
It's strongly recommended to break down tasks such as reading entire modules into smaller chunks.
For a focused discussion, use review comments to chat about specific files and their changes, instead of using the MR/PR comments.

CodeReview Documentation and Community

  • Visit our Documentation
    for detailed information on how to use Starship CodeReview.

About Us:

Visit the OpenCSG StarShip website for the Dashboard and detailed information on CodeReview, CodeGen, and other StarShip modules.

@Yiling-J Yiling-J self-requested a review January 3, 2025 03:51
@ganisback ganisback merged commit 5bcc387 into main Jan 3, 2025
6 checks passed
@ganisback ganisback deleted the refactor-runner-service branch January 3, 2025 04:05
@starship-github
Copy link

The StarShip CodeReviewer was triggered but terminated because it encountered an issue: The MR state is not opened.

Tips

CodeReview Commands (invoked as MR or PR comments)

  • @codegpt /review to trigger an code review.
  • @codegpt /evaluate to trigger code evaluation process.
  • @codegpt /describe to regenerate the summary of the MR.
  • @codegpt /secscan to scan security vulnerabilities for the MR or the Repository.
  • @codegpt /help to get help.

CodeReview Discussion Chat

There are 2 ways to chat with Starship CodeReview:

  • Review comments: Directly reply to a review comment made by StarShip.
    Example:
    • @codegpt How to fix this bug?
  • Files and specific lines of code (under the "Files changed" tab):
    Tag @codegpt in a new review comment at the desired location with your query.
    Examples:
    • @codegpt generate unit testing code for this code snippet.

Note: Be mindful of the bot's finite context window.
It's strongly recommended to break down tasks such as reading entire modules into smaller chunks.
For a focused discussion, use review comments to chat about specific files and their changes, instead of using the MR/PR comments.

CodeReview Documentation and Community

  • Visit our Documentation
    for detailed information on how to use Starship CodeReview.

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