Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

Use fslock to acquire lock when reading/writing the tanzu config file for update #2882

Merged
merged 1 commit into from
Jul 13, 2022

Conversation

anujc25
Copy link
Contributor

@anujc25 anujc25 commented Jul 8, 2022

What this PR does / why we need it

  • Use fslock to acquire lock when reading the tanzu config file for update
  • This PR implements wrapper functions AcquireTanzuConfigLock and ReleaseTanzuConfigLock using fslock.

Which issue(s) this PR fixes

Fixes #2357

Describe testing done for PR

  • Added unit tests to read and updated the configuration file in parallel.
  • Verified that the unit test fails without this locking mechanism and it is successful with this change.

Release note

Use fslock to acquire lock when reading/writing the tanzu config file for update

PR Checklist

  • Squash the commits into one or a small number of logical commits
  • Use good commit messages
  • Ensure PR contains terms all contributors can understand and links all contributors can access

Additional information

Special notes for your reviewer

@anujc25 anujc25 requested review from a team as code owners July 8, 2022 20:41
@github-actions
Copy link

github-actions bot commented Jul 8, 2022

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/2882/20220708205027/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@anujc25 anujc25 force-pushed the add-lock-tanzu-config-RW-v2 branch from 7e9b5b1 to c6b8594 Compare July 8, 2022 21:12
@github-actions
Copy link

github-actions bot commented Jul 8, 2022

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/2882/20220708212038/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@anujc25 anujc25 force-pushed the add-lock-tanzu-config-RW-v2 branch from c6b8594 to fbc15fe Compare July 8, 2022 22:34
@github-actions
Copy link

github-actions bot commented Jul 8, 2022

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/2882/20220708224933/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@codecov
Copy link

codecov bot commented Jul 8, 2022

Codecov Report

Merging #2882 (838da0d) into main (f033cd2) will decrease coverage by 0.07%.
The diff coverage is 31.85%.

@@            Coverage Diff             @@
##             main    #2882      +/-   ##
==========================================
- Coverage   43.80%   43.73%   -0.08%     
==========================================
  Files         414      415       +1     
  Lines       41297    41436     +139     
==========================================
+ Hits        18090    18120      +30     
- Misses      21512    21613     +101     
- Partials     1695     1703       +8     
Impacted Files Coverage Δ
pkg/v1/auth/csp/grpc.go 0.00% <0.00%> (ø)
pkg/v1/cli/command/core/config.go 36.46% <0.00%> (-1.43%) ⬇️
pkg/v1/cli/command/core/discovery_source.go 38.34% <0.00%> (-2.80%) ⬇️
pkg/v1/cli/command/core/repo.go 14.28% <0.00%> (-1.69%) ⬇️
pkg/v1/config/clientconfig.go 34.79% <45.45%> (-2.50%) ⬇️
pkg/v1/config/lock.go 64.00% <64.00%> (ø)
pkg/v1/config/discovery.go 74.07% <0.00%> (-16.67%) ⬇️
.../cli/plugin/cluster/get_machinehealthcheck_node.go 9.30% <0.00%> (-1.81%) ⬇️
pkg/v1/tkg/tkgpackageclient/package_update.go 83.57% <0.00%> (-1.43%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f033cd2...838da0d. Read the comment docs.

Copy link
Contributor

@tenczar tenczar left a comment

Choose a reason for hiding this comment

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

LGTM

@anujc25 anujc25 force-pushed the add-lock-tanzu-config-RW-v2 branch from fbc15fe to ba1f9c2 Compare July 12, 2022 19:22
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/2882/20220712193320/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@anujc25 anujc25 force-pushed the add-lock-tanzu-config-RW-v2 branch from ba1f9c2 to eba494c Compare July 12, 2022 20:18
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/2882/20220712202725/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@anujc25 anujc25 force-pushed the add-lock-tanzu-config-RW-v2 branch from eba494c to c015f66 Compare July 12, 2022 20:45
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/2882/20220712205444/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

Copy link
Contributor

@vuil vuil left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for the updates

Comment on lines +88 to +89
config.AcquireTanzuConfigLock()
defer config.ReleaseTanzuConfigLock()
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to do this inside config.GetClientConfig and config.StoreClientConfig functions so the callers don't have to worry about it? If I'm using one of those functions, I would ideally expect it to handle all the locking for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, I don't think it is possible without refactoring how we are calling this functions because these external functions use config.GetClientConfig to get the config, update the config as part of the function and use config.StoreClientConfig to update the config. We would like to have the RW lock for this entire read/write operation.

I am not sure we can reliably add locking mechanism as part of config.GetClientConfig and config.StoreClientConfig individually.

pkg/v1/config/lock.go Outdated Show resolved Hide resolved
Signed-off-by: Anuj Chaudhari <anujc@vmware.com>
@anujc25 anujc25 force-pushed the add-lock-tanzu-config-RW-v2 branch from c015f66 to 838da0d Compare July 13, 2022 17:15
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/2882/20220713173138/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@vuil vuil added ok-to-merge PRs should be labelled with this before merging area/ux UX related area/plugin labels Jul 13, 2022
@anujc25 anujc25 merged commit 74109cf into vmware-tanzu:main Jul 13, 2022
pgandigesang pushed a commit to pgandigesang/tanzu-framework that referenced this pull request Jul 15, 2022
pgandigesang pushed a commit to pgandigesang/tanzu-framework that referenced this pull request Jul 15, 2022
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
area/plugin area/ux UX related cla-not-required ok-to-merge PRs should be labelled with this before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement file locking during CLI config writes
5 participants