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

Implement flags grouping for karmada-agent #1389

Merged
merged 1 commit into from
Mar 15, 2022

Conversation

lonelyCZ
Copy link
Member

@lonelyCZ lonelyCZ commented Feb 22, 2022

Signed-off-by: lonelyCZ 531187475@qq.com

What type of PR is this?
/kind feature

What this PR does / why we need it:
It make flags of command clearer.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
I'm not sure the place of group.go should be, and leave it here for the time being.

/cc @RainbowMango

Does this PR introduce a user-facing change?:

`karmada-agent`: The klog flags now have been grouped for better readability.

@karmada-bot karmada-bot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 22, 2022
@lonelyCZ
Copy link
Member Author

The result is below

[root@master67 karmada]# ./karmada-agent -h
The karmada agent runs the cluster registration agent

Usage: 
  karmada-agent [flags]

Commands:
  completion            generate the autocompletion script for the specified shell
  help                  Help about any command
  version               Print the version information.

General Flags:
      --controllers strings                           A list of controllers to enable. '*' enables all on-by-default controllers, 'foo' enables the controller named 'foo', '-foo' disables the controller named 'foo'. All controllers: clusterStatus, execution, serviceExport, workStatus. (default [*])
      --leader-elect                                  Start a leader election client and gain leadership before executing the main loop. Enable this when running replicated components for high availability. (default true)
      --leader-elect-resource-namespace string        The namespace of resource object that is used for locking during leader election. (default "karmada-system")
      --karmada-kubeconfig string                     Path to karmada control plane kubeconfig file.
      --karmada-context string                        Name of the cluster context in karmada control plane kubeconfig file.
      --cluster-name string                           Name of member cluster that the agent serves for.
      --cluster-namespace string                      Namespace in the control plane where member cluster secrets are stored. (default "karmada-cluster")
      --cluster-status-update-frequency duration      Specifies how often karmada-agent posts cluster status to karmada-apiserver. Note: be cautious when changing the constant, it must work with ClusterMonitorGracePeriod in karmada-controller-manager. (default 10s)
      --cluster-lease-duration duration               Specifies the expiration period of a cluster lease. (default 40s)
      --cluster-lease-renew-interval-fraction float   Specifies the cluster lease renew interval fraction. (default 0.25)
      --cluster-api-qps float32                       QPS to use while talking with cluster kube-apiserver. Doesn't cover events and node heartbeat apis which rate limiting is controlled by a different set of flags. (default 40)
      --cluster-api-burst int                         Burst to use while talking with cluster kube-apiserver. Doesn't cover events and node heartbeat apis which rate limiting is controlled by a different set of flags. (default 60)
      --kube-api-qps float32                          QPS to use while talking with karmada-apiserver. Doesn't cover events and node heartbeat apis which rate limiting is controlled by a different set of flags. (default 40)
      --kube-api-burst int                            Burst to use while talking with karmada-apiserver. Doesn't cover events and node heartbeat apis which rate limiting is controlled by a different set of flags. (default 60)
      --cluster-cache-sync-timeout duration           Timeout period waiting for cluster cache to sync. (default 30s)
      --cluster-api-endpoint string                   APIEndpoint of the cluster.
      --proxy-server-address string                   Address of the proxy server that is used to proxy to the cluster.
      --resync-period duration                        Base frequency the informers are resynced.

Logs Flags:
      --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
      --log_backtrace_at traceLocation   when logging hits line file:N, emit a stack trace (default :0)
      --log_dir string                   If non-empty, write log files in this directory
      --log_file string                  If non-empty, use this log file
      --log_file_max_size uint           Defines the maximum size a log file can grow to. Unit is megabytes. If the value is 0, the maximum file size is unlimited. (default 1800)
      --logtostderr                      log to standard error instead of files (default true)
      --one_output                       If true, only write logs to their native severity level (vs also writing to each lower severity level)
      --skip_headers                     If true, avoid header prefixes in the log messages
      --skip_log_headers                 If true, avoid headers when opening log files
      --stderrthreshold severity         logs at or above this threshold go to stderr (default 2)
  -v, --v Level                          number for the log level verbosity
      --vmodule moduleSpec               comma-separated list of pattern=N settings for file-filtered logging

Global Flags:
  -h, --help                           help for this command
      --log-flush-frequency duration   Maximum number of seconds between log flushes (default 5s)

Use 'karmada-agent [command] --help' for more information about a command.
[root@master67 karmada]# ./karmada-agent version -h
Print the version information.

Usage: 
  karmada-agent version [flags]

Examples: 
  # Print karmada-agent command version
  karmada-agent version

Global Flags:
  -h, --help                           help for this command
      --log-flush-frequency duration   Maximum number of seconds between log flushes (default 5s)

Use 'karmada-agent version [command] --help' for more information about a command.

@RainbowMango
Copy link
Member

I'll look at it soon. Just a feeling that we might don't need so many changes.

@RainbowMango
Copy link
Member

But the result is exactly what we needed. Thanks!

@lonelyCZ
Copy link
Member Author

I'll look at it soon. Just a feeling that we might don't need so many changes.

Yes, cobra.Command didn't implement custom flags groups, so we need to customize Usage template.

The pkg/util/cmdutils/group.go is a shared Usage template, the other components only call it.

@RainbowMango
Copy link
Member

I am busy preparing the incoming release, I'm not sure I can get back to this PR this week. Just a reminder.

@lonelyCZ
Copy link
Member Author

There's no hurry. I am also working on other task #1367 this week.

@RainbowMango
Copy link
Member

/assign

I'm back to this issue now.

@RainbowMango
Copy link
Member

Hi @lonelyCZ ,

I'm hesitant to copy more code from other projects, my concern is the code we lifted might become the technical debt and it's hard to sync bugfix from the original source. That's the reason why we are going to record these things at #1453. I'm not saying we can't lift code anymore, just more cautious.

I tried to do similar things for karmada-controller-manager, see this commit, it can group the flags as we expected, but there is one issue that I don't know how to handle:
The Available Commands can't be shown out.

Available Commands:
  completion  generate the autocompletion script for the specified shell
  help        Help about any command
  version     Print the version information.

(The commands are still working but the information missing.)

I'll continue investigating it further once I have time.

@lonelyCZ
Copy link
Member Author

Got it, I also research it later.

@RainbowMango
Copy link
Member

Thanks @lonelyCZ

I find a solution and filed a draft PR #1468.
Seems we can customize the SetUsageAndHelpFunc function and I'll try to put it to a common package that other components can share it.

@lonelyCZ
Copy link
Member Author

Hi, @RainbowMango , Do we need to implement group for other components as #1468 in other PR? One pr for all components or one pr for one components?

@RainbowMango
Copy link
Member

Yes, one PR one component is good. And I think this PR could be the next.

@karmada-bot karmada-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 15, 2022
@lonelyCZ
Copy link
Member Author

Yes, one PR one component is good. And I think this PR could be the next.

Right, I have updated it.

@lonelyCZ
Copy link
Member Author

Could I submit a issue to list tasks for other components and invite people who are interested in this to participate? @RainbowMango

@RainbowMango
Copy link
Member

Could I submit a issue to list tasks for other components and invite people who are interested in this to participate?

Yeah! That's more professional. You can let this PR as an example.

// (https://github.com/kubernetes-sigs/controller-runtime/blob/v0.11.1/pkg/client/config/config.go#L39),
// and update the flag usage.
genericFlagSet.AddGoFlagSet(flag.CommandLine)
genericFlagSet.Lookup("kubeconfig").Usage = "Path to karmada control plane kubeconfig file."
Copy link
Member

Choose a reason for hiding this comment

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

For karmada-agent, no need to update the Usage. This is different with karmada-controller-manager.
For karmada-agent

  • --karmada-kubeconfig --> point to karmada control plane.
  • --kubeconfig --> point to the cluster where running karmada-agent

Remove this line and the comments on above should be ok.

Signed-off-by: lonelyCZ <531187475@qq.com>
Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 15, 2022
@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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

The pull request process is described 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

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 15, 2022
@karmada-bot karmada-bot merged commit 82c2fdd into karmada-io:master Mar 15, 2022
@lonelyCZ lonelyCZ deleted the group-flag branch July 11, 2022 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants