Skip to content

Separate attacher flags into attacher only flags and global shared flags #652

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mauriciopoppe
Copy link
Member

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind design

What this PR does / why we need it:
Split attacher flags into attacher only flags and global flags.

We're moving some of the flags into its own package so that the flags can be imported by external-attacher and by other repositories. We're doing this to prepare external-attacher for its integration with the CSI Sidecar monorepo.

Testing:

  • Within this repo there's no op besides checking that it builds fine.
  • In the CSI Sidecar monorepo we should be able to import the new cmd/config package and reuse the exported functions and types. The CSI Sidecar monorepo will use the function RegisterAttacherFlagsWithPrefix so that the attacher flags are imported with the attacher- prefix.

In the CSI Sidecar monorepo the change to adopt this is mauriciopoppe/csi-sidecars-aio-poc#11.

After compiling the CSI Sidecar monorepo binary the output for its help is:

+ ./bin/csi-sidecars --help
Usage of ./bin/csi-sidecars:
      --add_dir_header                                    If true, adds the file directory to the header of the log messages
      --alsologtostderr                                   log to standard error as well as files (no effect when -logtostderr=true)
      --attacher-default-fstype string                    The default filesystem type of the volume to use.
      --attacher-max-entries int                          Max entries per each page in volume lister call, 0 means no limit.
      --attacher-max-grpc-log-length int                  The maximum amount of characters logged for every grpc responses. Defaults to no limit (default -1)
      --attacher-reconcile-sync duration                  Resync interval of the VolumeAttachment reconciler. (default 1m0s)
      --attacher-retry-interval-max duration              Maximum retry interval of failed create volume or deletion. (default 5m0s)
      --attacher-retry-interval-start duration            Initial retry interval of failed create volume or deletion. It doubles with each failure, up to retry-interval-max. (default 1s)
      --attacher-timeout duration                         Timeout for waiting for attaching or detaching the volume. (default 15s)
      --attacher-worker-threads int                       Number of worker threads per sidecar (default 10)

For more info please read https://docs.google.com/document/d/1AKqJeAlBL8PkH8D9zABCZ82Bk1N46EygKPvVh5p4-qU/edit?tab=t.0.

Does this PR introduce a user-facing change?:

NONE

/cc @jsafrane @xing-yang @mowangdk @ConnorJC3

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label May 31, 2025
@k8s-ci-robot k8s-ci-robot requested a review from xing-yang May 31, 2025 21:53
@k8s-ci-robot k8s-ci-robot added the kind/design Categorizes issue or PR as related to design. label May 31, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mauriciopoppe
Once this PR has been reviewed and has the lgtm label, please assign msau42 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 31, 2025
@mowangdk
Copy link
Contributor

mowangdk commented Jun 2, 2025

So is this just a demo PR for the csi aio integration or will it be a former PR in the future? I think maybe I can close #620 and we can refine this PR. they are basically the same

@mauriciopoppe
Copy link
Member Author

Thanks for creating #620! In this PR I tried to do as minimum as possible to move only the flags that'd be reused from two places (this sidecar and the monorepo). Yeah, we can refine this PR only if you're ok with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/design Categorizes issue or PR as related to design. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants