search

vmware-tanzu / tanzu-cli

The Tanzu Core CLI project provides the core functionality of the Tanzu CLI. The CLI is based on a plugin architecture where CLI command functionality can be delivered through independently developed plugin binaries
Apache License 2.0
33 stars 21 forks source link

Update the file locking module dependency #772

Closed prkalle closed 2 months ago

prkalle commented 3 months ago

What this PR does / why we need it

This PR updates the file locking module dependency

Which issue(s) this PR fixes

Fixes #

Describe testing done for PR

Unit tests and CI checks passed

Tested manually the lock is auto-released on process exit

❯ make build
build darwin-amd64 CLI with version: v1.3.0-dev
mkdir -p bin
cp /Users/pkalle/projects/tanzu-cli/artifacts/darwin/amd64/cli/core/v1.3.0-dev/tanzu-cli-darwin_amd64 ./bin/tanzu

❯ sqlite3 -batch ~/.config/tanzu-cli-telemetry/cli_metrics.db "select * from tanzu_cli_operations;"
cli_version|os_name|os_arch|plugin_name|plugin_version|command|cli_id|command_start_ts|command_end_ts|target|name_arg|endpoint|flags|exit_status|is_internal|error
v1.3.0|darwin|amd64|appsv2|v0.1.0-beta.2-dev-411403a5|app|8fd081d4-b2d2-413e-b461-5ad917b6635c|1718167006626|1718167009479|global||||0|1|
❯ ./bin/tanzu context list
  NAME                                   ISACTIVE  TYPE             PROJECT            SPACE
  1_ARM_ESO_MAIN_FRESH-staging-d03c5c97  false     tanzu
  TAP_pre-integration-staging-d03c5c97   false     tanzu            jay-project        space3
  TAP_staging-staging-d03c5c97           false     tanzu
  Tap-SaaS-Beta3                         false     tanzu
  mytmc-ctx                              false     mission-control  n/a                n/a
  prem-test-context                      false     tanzu
  tap-saas-ga-1                          true      tanzu            longevity-project  cli-dev-test
  tkg-mgmt-vc                            false     kubernetes       n/a                n/a
  tt-test-selfmg                         false     mission-control  n/a                n/a
  ucp                                    false     tanzu

[i] Use '--wide' to view additional columns.

❯ sqlite3 -batch ~/.config/tanzu-cli-telemetry/cli_metrics.db "select * from tanzu_cli_operations;"
cli_version|os_name|os_arch|plugin_name|plugin_version|command|cli_id|command_start_ts|command_end_ts|target|name_arg|endpoint|flags|exit_status|is_internal|error
v1.3.0|darwin|amd64|appsv2|v0.1.0-beta.2-dev-411403a5|app|8fd081d4-b2d2-413e-b461-5ad917b6635c|1718167006626|1718167009479|global||||0|1|

Later compiled the code removing the os.Exit(2) and ran the CLI, and this time it successfully updated the metrics which confirms the earlier exit without unlocking auto-released the lock. I will try to add the automated testing for this in separate PR.

❯ make  build
build darwin-amd64 CLI with version: v1.3.0-dev
mkdir -p bin
cp /Users/pkalle/projects/tanzu-cli/artifacts/darwin/amd64/cli/core/v1.3.0-dev/tanzu-cli-darwin_amd64 ./bin/tanzu
❯ ./bin/tanzu context list
  NAME                                   ISACTIVE  TYPE             PROJECT            SPACE
  1_ARM_ESO_MAIN_FRESH-staging-d03c5c97  false     tanzu
  TAP_pre-integration-staging-d03c5c97   false     tanzu            jay-project        space3
  TAP_staging-staging-d03c5c97           false     tanzu
  Tap-SaaS-Beta3                         false     tanzu
  mytmc-ctx                              false     mission-control  n/a                n/a
  prem-test-context                      false     tanzu
  tap-saas-ga-1                          true      tanzu            longevity-project  cli-dev-test
  tkg-mgmt-vc                            false     kubernetes       n/a                n/a
  tt-test-selfmg                         false     mission-control  n/a                n/a
  ucp                                    false     tanzu

[i] Use '--wide' to view additional columns.
❯ sqlite3 -batch ~/.config/tanzu-cli-telemetry/cli_metrics.db "select * from tanzu_cli_operations;"
cli_version|os_name|os_arch|plugin_name|plugin_version|command|cli_id|command_start_ts|command_end_ts|target|name_arg|endpoint|flags|exit_status|is_internal|error
v1.3.0|darwin|amd64|appsv2|v0.1.0-beta.2-dev-411403a5|app|8fd081d4-b2d2-413e-b461-5ad917b6635c|1718167006626|1718167009479|global||||0|1|
v1.3.0-dev|darwin|amd64|||context list|8fd081d4-b2d2-413e-b461-5ad917b6635c|1718176704393|1718176704476|||||0|1|

Verified it works on Windows as well.


H:\windows-cli-test\cli\v1.3.0-dev>tanzu-cli-windows_amd64.exe version
version: v1.3.0-dev
buildDate: 2024-06-12
sha: 8ccca608
arch: amd64

H:\windows-cli-test\cli\v1.3.0-dev>tanzu-cli-windows_amd64.exe context list
  NAME                          ISACTIVE  TYPE   PROJECT  SPACE
  TAP_staging-staging-d03c5c97  true      tanzu

[i] Use '--wide' to view additional columns.

H:\windows-cli-test\cli\v1.3.0-dev>
H:\windows-cli-test\cli\v1.3.0-dev>tanzu-cli-windows_amd64.exe context use TANZU_CLI_SHOW_TELEMETRY_CONSOLE_LOGS
[x] : context TANZU_CLI_SHOW_TELEMETRY_CONSOLE_LOGS not found

H:\windows-cli-test\cli\v1.3.0-dev>tanzu-cli-windows_amd64.exe context use TAP_staging-staging-d03c5c97
[i] Successfully activated context 'TAP_staging-staging-d03c5c97' (Type: tanzu)

H:\windows-cli-test\cli\v1.3.0-dev>tanzu-cli-windows_amd64.exe context delete TAP_staging-staging-d03c5c97
Deleting the context entry from the config will remove it from the list of tracked contexts. You will need to use `tanzu context create` to re-create this context. Are you sure you want to continue? [y/N]: N

//Later copied the sqlite db file to other machine and validated the telemetry entries are updated in the DB which confirms the locking/unlocking is working as expected on windows.

❯ scp sc-dbc2163.eng.vmware.com:/mts/home4/pkalle/windows-cli-test/cli/cli_metrics.db .
pkalle@sc-dbc2163.eng.vmware.com's password:
cli_metrics.db                                                                                                                                                                    100%   12KB 166.9KB/s   00:00
❯ sqlite3 -batch ./cli_metrics.db "select * from tanzu_cli_operations;"
cli_version|os_name|os_arch|plugin_name|plugin_version|command|cli_id|command_start_ts|command_end_ts|target|name_arg|endpoint|flags|exit_status|is_internal|error
v1.3.0-dev|windows|amd64|||context list|22ee360d-13b1-4b42-8b85-9f8e69822d04|1718217033724|1718217033769|||||0|0|
v1.3.0-dev|windows|amd64|||context list|22ee360d-13b1-4b42-8b85-9f8e69822d04|1718217649175|1718217649218|||||0|0|
v1.3.0-dev|windows|amd64|||context list|22ee360d-13b1-4b42-8b85-9f8e69822d04|1718217843421|1718217843465|||||0|0|
v1.3.0-dev|windows|amd64|||context use|22ee360d-13b1-4b42-8b85-9f8e69822d04|1718217856357|1718217856393||be05294ec015003b552b205540ba9b245fc7329259bd223a1d7813494a8984e7|||1|0|
v1.3.0-dev|windows|amd64|||context use|22ee360d-13b1-4b42-8b85-9f8e69822d04|1718217867032|1718217867075||a1467bf1ce39871ed82c8a5197e7b6cebcd6c96ff724c957ef86323753f1b3b6|||0|0|
v1.3.0-dev|windows|amd64|||context delete|22ee360d-13b1-4b42-8b85-9f8e69822d04|1718217888264|1718217890725||a1467bf1ce39871ed82c8a5197e7b6cebcd6c96ff724c957ef86323753f1b3b6|||0|0|

Release note

Additional information

Special notes for your reviewer

vuil commented 3 months ago

Thanks, changes look good. But I have some suggestions about testing: (ideally automated, but manually at least)

  1. similar to TestParallelLocking, can we provide one that starts multiple goroutines and verify that multiple of them are able to successfully acquire the lock (make a file modification and quickly get out) and verify that the locking has the desired effect (i.e. sequence of modifications is verified to reflect the locking sequence). If there are other parts of the codebase that indirectly tests what I suggest, it might be okay too.
  2. In testing done, can you report some test results pertaining to this change when run on windows?
  3. It would be good verify if lock is auto-released on process exit. (seems from the implementation of the newly imported package that it should, but it doesn't hurt to verify)
prkalle commented 2 months ago

Thanks, changes look good. But I have some suggestions about testing: (ideally automated, but manually at least)

  1. similar to TestParallelLocking, can we provide one that starts multiple goroutines and verify that multiple of them are able to successfully acquire the lock (make a file modification and quickly get out) and verify that the locking has the desired effect (i.e. sequence of modifications is verified to reflect the locking sequence). If there are other parts of the codebase that indirectly tests what I suggest, it might be okay too.

Thank you. Added a test to do paralled locking and unlocking using goroutines.

  1. It would be good verify if lock is auto-released on process exit. (seems from the implementation of the newly imported package that it should, but it doesn't hurt to verify) Verified manually by adding the os.Exit(2) here after line 94. Then build and ran the the below command, we can see the CLI exit and didn't save the metric. 
❯ make build
build darwin-amd64 CLI with version: v1.3.0-dev
mkdir -p bin
cp /Users/pkalle/projects/tanzu-cli/artifacts/darwin/amd64/cli/core/v1.3.0-dev/tanzu-cli-darwin_amd64 ./bin/tanzu

❯ sqlite3 -batch ~/.config/tanzu-cli-telemetry/cli_metrics.db "select * from tanzu_cli_operations;"
cli_version|os_name|os_arch|plugin_name|plugin_version|command|cli_id|command_start_ts|command_end_ts|target|name_arg|endpoint|flags|exit_status|is_internal|error
v1.3.0|darwin|amd64|appsv2|v0.1.0-beta.2-dev-411403a5|app|8fd081d4-b2d2-413e-b461-5ad917b6635c|1718167006626|1718167009479|global||||0|1|
❯ ./bin/tanzu context list
  NAME                                   ISACTIVE  TYPE             PROJECT            SPACE
  1_ARM_ESO_MAIN_FRESH-staging-d03c5c97  false     tanzu
  TAP_pre-integration-staging-d03c5c97   false     tanzu            jay-project        space3
  TAP_staging-staging-d03c5c97           false     tanzu
  Tap-SaaS-Beta3                         false     tanzu
  mytmc-ctx                              false     mission-control  n/a                n/a
  prem-test-context                      false     tanzu
  tap-saas-ga-1                          true      tanzu            longevity-project  cli-dev-test
  tkg-mgmt-vc                            false     kubernetes       n/a                n/a
  tt-test-selfmg                         false     mission-control  n/a                n/a
  ucp                                    false     tanzu

[i] Use '--wide' to view additional columns.

❯ sqlite3 -batch ~/.config/tanzu-cli-telemetry/cli_metrics.db "select * from tanzu_cli_operations;"
cli_version|os_name|os_arch|plugin_name|plugin_version|command|cli_id|command_start_ts|command_end_ts|target|name_arg|endpoint|flags|exit_status|is_internal|error
v1.3.0|darwin|amd64|appsv2|v0.1.0-beta.2-dev-411403a5|app|8fd081d4-b2d2-413e-b461-5ad917b6635c|1718167006626|1718167009479|global||||0|1|

Later compiled the code removing the os.Exit(2) and ran the CLI, and this time it successfully updated the metrics which confirms the earlier exit without unlocking auto-released the lock. I will try to add the automated testing for this in separate PR.

❯ make  build
build darwin-amd64 CLI with version: v1.3.0-dev
mkdir -p bin
cp /Users/pkalle/projects/tanzu-cli/artifacts/darwin/amd64/cli/core/v1.3.0-dev/tanzu-cli-darwin_amd64 ./bin/tanzu
❯ ./bin/tanzu context list
  NAME                                   ISACTIVE  TYPE             PROJECT            SPACE
  1_ARM_ESO_MAIN_FRESH-staging-d03c5c97  false     tanzu
  TAP_pre-integration-staging-d03c5c97   false     tanzu            jay-project        space3
  TAP_staging-staging-d03c5c97           false     tanzu
  Tap-SaaS-Beta3                         false     tanzu
  mytmc-ctx                              false     mission-control  n/a                n/a
  prem-test-context                      false     tanzu
  tap-saas-ga-1                          true      tanzu            longevity-project  cli-dev-test
  tkg-mgmt-vc                            false     kubernetes       n/a                n/a
  tt-test-selfmg                         false     mission-control  n/a                n/a
  ucp                                    false     tanzu

[i] Use '--wide' to view additional columns.
❯ sqlite3 -batch ~/.config/tanzu-cli-telemetry/cli_metrics.db "select * from tanzu_cli_operations;"
cli_version|os_name|os_arch|plugin_name|plugin_version|command|cli_id|command_start_ts|command_end_ts|target|name_arg|endpoint|flags|exit_status|is_internal|error
v1.3.0|darwin|amd64|appsv2|v0.1.0-beta.2-dev-411403a5|app|8fd081d4-b2d2-413e-b461-5ad917b6635c|1718167006626|1718167009479|global||||0|1|
v1.3.0-dev|darwin|amd64|||context list|8fd081d4-b2d2-413e-b461-5ad917b6635c|1718176704393|1718176704476|||||0|1|
prkalle commented 2 months ago

For future reference, can you paste what you posted in terms of manual testing done into the testing section of the PR description?

Sure, thank you! Updated the PR description with the testing done (both manual testing done for abrupt process exit and windows testing)