Skip to content

feat(1-1-restore): add unpin-agent-cpu flag #4380

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

Merged

Conversation

VAveryanov8
Copy link
Collaborator

This adds support of unpin-agent-cpu flag to 1-1-restore command.
Unpinning agent CPU should help with agent download speed.

Refs: #4375


Please make sure that:

  • Code is split to commits that address a single change
  • Commit messages are informative
  • Commit titles have module prefix
  • Commit titles have issue nr. suffix

@VAveryanov8 VAveryanov8 marked this pull request as ready for review May 12, 2025 09:35
@VAveryanov8 VAveryanov8 requested a review from Copilot May 12, 2025 09:35
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for the unpin-agent-cpu flag to the 1-1-restore command to potentially improve agent download speed. Key changes include:

  • Adding a new boolean flag (UnpinAgentCPU) in the Target model and corresponding command structures.
  • Implementing CPU pinning/unpinning logic in the restore workflow and its helper functions.
  • Documenting the new flag in both YAML configuration files and command usage partials.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
pkg/service/one2onerestore/worker.go Implements CPU pinning logic with deferred re-pinning behavior.
pkg/service/one2onerestore/model.go Adds the UnpinAgentCPU field to the Target struct.
pkg/command/one2onerestore/cmd.go Introduces a new flag and propagates its value into task props.
pkg/command/one2onerestore/res.yaml Documents the new flag in the command resource configuration.
docs/source/sctool/partials/sctool_restore_1-1-restore_update.yaml Updates command option documentation with the new flag.
docs/source/sctool/partials/sctool_restore_1-1-restore.yaml Mirrors the update in a separate YAML partial for restore.
Comments suppressed due to low confidence (1)

pkg/service/one2onerestore/worker.go:82

  • Consider using the original or a derived context instead of context.Background() in this deferred call for consistent cancellation handling and logging.
defer func() { if err := w.pinAgentCPU(context.Background(), workload, true); err != nil { w.logger.Error(ctx, "Can't pin agent to CPU", "error", err) } }()

@VAveryanov8 VAveryanov8 self-assigned this May 12, 2025
Copy link
Collaborator

@Michal-Leszczynski Michal-Leszczynski left a comment

Choose a reason for hiding this comment

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

Maybe there won't be a need for this flag to be configurable (e.g. it will always be better to unpin agent for the 1-1-restore), but it is useful for benchmarking (it can always be removed before release if necessary).

FYI @mikliapko it is set to false by default, so it needs to be manually set during the 1-1-restore benchamarks.

@VAveryanov8 VAveryanov8 force-pushed the 1-1-restore-feature-branch branch from bde0cfb to fca8d54 Compare May 28, 2025 08:52
This adds support of unpin-agent-cpu flag to 1-1-restore command.
Unpinning agent CPU should help with agent download speed.

Refs: #4375
@VAveryanov8 VAveryanov8 force-pushed the va/1-1-restore-unpin-agent-cpu branch from d6a3395 to 788fc0f Compare May 28, 2025 14:11
@VAveryanov8 VAveryanov8 merged commit a22c931 into 1-1-restore-feature-branch May 29, 2025
41 checks passed
@VAveryanov8 VAveryanov8 deleted the va/1-1-restore-unpin-agent-cpu branch May 29, 2025 13:19
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