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

[DocDB] yb-admin not accepting correct number of arguments for create_database_snapshot #23332

Closed
1 task done
yamen-haddad opened this issue Jul 30, 2024 · 0 comments
Closed
1 task done
Assignees
Labels
area/docdb YugabyteDB core features kind/bug This issue is a bug priority/medium Medium priority issue

Comments

@yamen-haddad
Copy link
Member

yamen-haddad commented Jul 30, 2024

Jira Link: DB-12259

Description

After adding a Time To Live to the snapshots created by the user. The default TTL for snapshots before deleting them is 24 hours or the flag default_snapshot_retention_hours. However, users can override this default behaviour when creating a snapshot using yb-admin by providing the desired retention_duration_hours as a second argument to the create_database_snapshot. So if the user wants to leave the snapshot for 72 hours, he can issue:

./bin/yb-admin --master_addresses $MASTERS create_database_snapshot ysql.db1 72

However, yb_admin CLI is not accepting that the user set the second argument and returns Invalid argument error by mistake.
Additionally, if the user wants to retain the snapshot forever, the instructions say:
(set a <= 0 value to retain the snapshot forever. If not specified then takes the default value controlled by gflag default_retention_hours)
However, if the user tries to set a negative value, the negative sign will be parsed as an option for yb-admin. Ex:

$ ./build/latest/bin/yb-admin --master_addresses 127.0.0.1:7100,127.0.0.2:7100,127.0.0.3:7100 create_database_snapshot ysql.db1 '-9'
ERROR: unknown command line flag '9'

This is confusing and the user can simply set the default_retention_hours to 0 in case he wants to retain the snapshot forever.

Issue Type

kind/bug

Warning: Please confirm that this issue does not contain any sensitive information

  • I confirm this issue does not contain any sensitive information.
@yamen-haddad yamen-haddad added area/docdb YugabyteDB core features status/awaiting-triage Issue awaiting triage labels Jul 30, 2024
@yamen-haddad yamen-haddad self-assigned this Jul 30, 2024
@yugabyte-ci yugabyte-ci added kind/bug This issue is a bug priority/medium Medium priority issue labels Jul 30, 2024
yamen-haddad added a commit that referenced this issue Jul 31, 2024
Summary:
After adding a Time To Live to the snapshots created by the user. The default TTL for snapshots before deleting them is 24 hours or the flag `default_snapshot_retention_hours`. However, users can override this default behaviour when creating a snapshot using `yb-admin` by providing the desired `retention_duration_hours` as a second argument to the `create_database_snapshot`. So if the user wants to leave the snapshot for 72 hours, he can issue:

```
./bin/yb-admin --master_addresses $MASTERS create_database_snapshot ysql.db1 72
```
However, yb_admin CLI is not accepting that the user set the second argument and returns `Invalid argument` error by mistake.
Additionally, if the user wants to retain the snapshot forever, the instructions say:
`(set a <= 0 value to retain the snapshot forever. If not specified then takes the default value controlled by gflag default_retention_hours)`
However, if the user tries to set a negative value, the negative sign will be parsed as an option for yb-admin. Ex:
```
$ ./build/latest/bin/yb-admin --master_addresses 127.0.0.1:7100,127.0.0.2:7100,127.0.0.3:7100 create_database_snapshot ysql.db1 '-9'
ERROR: unknown command line flag '9'
```
The diff fixes these issues by:
1- Accepting 1 or 2 arguments for `create_database_snapshot`
2- A simpler message to the user if he wants to retain the snapshot forever: `set retention_duration_hours to 0 to retain the snapshot forever.` Instead of telling the user to set it to negative value while that is not possible.
Jira: DB-12259

Test Plan:
Tested manually, by running the yb-admin command `create_database_snapshot` with and without the `retention_duration_hours` option set. The command was failing before this diff.

Before this diff:
```

$ ./build/latest/bin/yb-admin --master_addresses 127.0.0.1:7100,127.0.0.2:7100,127.0.0.3:7100 create_database_snapshot ysql.db1 72
Error running create_database_snapshot: Invalid argument (yb/tools/yb-admin_cli.cc:91): Invalid arguments for operation
Usage: ./build/latest/bin/yb-admin create_database_snapshot [ysql.]<database_name> [retention_duration_hours] (set a <= 0 value to retain the snapshot forever. If not specified then takes the default value controlled by gflag default_retention_hours)
```
After this diff it works as desired.

Reviewers: zdrudi

Reviewed By: zdrudi

Subscribers: ybase, slingam

Differential Revision: https://phorge.dev.yugabyte.com/D36936
jasonyb pushed a commit that referenced this issue Aug 2, 2024
Summary:
 66890cb [PLAT-14781] Add runtime config for download metrics as pdf
 b2f24f2 [#9797] YSQL: Stubs for ysql major version upgrade RPCs
 5891faa [#23332] Docdb: Fix yb-admin to handle snapshot_retention correctly
 0614c52 [PLAT-14756] Releases API RBAC cleanup
 2cfb999 [docs] Release notes for 2024.1.1.0-b137 (#23185)
 d868a03 [PLAT-13495] Do not run dual nic steps on systemd upgrade
 10bc447 [PLAT-14798] Adding internal load balancer to devspace
 7296df8 [docs] [2.23] Add pg_cron extension docs (#22546)
 79902ae [PLAT-14678] - feat : Export All Metrics to pdf
 8a0b95c [#23322] YSQL: pg_partman: fix logic of checking existing table
 Excluded: 63f471a [#18822] YSQL: Framework to skip redundant sec index updates and fkey checks when relevant columns not modified
 3040472 [PLAT-14783] [PGParity] After Edit RR cores scale up on PG Parity enabled cluster, the RR node does not have PGP gflags
 e052089 [PLAT-14774] Per process tserver metrics is not working if YSQL is disabled
 0c664a1 [#22370] docdb: Cost Based Optimizer changes to take into account backward scans improvement
 a060877 [PLAT-13712]fix plan info for Azure VMs
 291dd40 Remove 6sense domains from CSP headers (#23354)
 75cb273 [#23330] docdb: fixed static columns handling for CQL operations

Test Plan: Jenkins: rebase: pg15-cherrypicks

Reviewers: jason, tfoucher

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D36984
@rthallamko3 rthallamko3 removed the status/awaiting-triage Issue awaiting triage label Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docdb YugabyteDB core features kind/bug This issue is a bug priority/medium Medium priority issue
Projects
None yet
Development

No branches or pull requests

3 participants