Update the file locking module dependency #772

prkalle · 2024-05-24T19:33:21Z

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

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|

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 .
[email protected]'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 · 2024-06-07T16:00:05Z

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

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.
In testing done, can you report some test results pertaining to this change when run on windows?
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)

Signed-off-by: Prem Kumar Kalle <[email protected]>

prkalle · 2024-06-12T07:28:12Z

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

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.

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|

vuil

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

prkalle · 2024-06-12T18:58:35Z

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)

prkalle requested a review from a team as a code owner May 24, 2024 19:33

vmwclabot added the cla-not-required label May 24, 2024

prkalle force-pushed the update/lock_lib branch from a50f8c0 to 883429b Compare May 24, 2024 19:44

Update the file locking module dependency

8ccca60

Signed-off-by: Prem Kumar Kalle <[email protected]>

prkalle force-pushed the update/lock_lib branch from 883429b to 8ccca60 Compare June 12, 2024 07:19

vuil approved these changes Jun 12, 2024

View reviewed changes

prkalle merged commit 9539aed into vmware-tanzu:main Jun 12, 2024
7 checks passed

marckhouzam added this to the v1.4.0 milestone Jun 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the file locking module dependency #772

Update the file locking module dependency #772

prkalle commented May 24, 2024 •

edited

Loading

vuil commented Jun 7, 2024

prkalle commented Jun 12, 2024 •

edited

Loading

vuil left a comment

prkalle commented Jun 12, 2024 •

edited

Loading

Update the file locking module dependency #772

Update the file locking module dependency #772

Conversation

prkalle commented May 24, 2024 • edited Loading

What this PR does / why we need it

Which issue(s) this PR fixes

Describe testing done for PR

Release note

Additional information

Special notes for your reviewer

vuil commented Jun 7, 2024

prkalle commented Jun 12, 2024 • edited Loading

vuil left a comment

Choose a reason for hiding this comment

prkalle commented Jun 12, 2024 • edited Loading

prkalle commented May 24, 2024 •

edited

Loading

prkalle commented Jun 12, 2024 •

edited

Loading

prkalle commented Jun 12, 2024 •

edited

Loading