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

Uses viper for reading configurations for controller/watcher #1409

Closed
wants to merge 5 commits into from

Conversation

sm43
Copy link
Contributor

@sm43 sm43 commented Sep 4, 2023

(Still need some work to make it work)

here are the changes which would be required if we go ahead with this

  • this mounts the configmap as volume and if configmap changes it takes some time (noticeable time) to actually get it reflected in the pod
    why?
The kubelet checks whether the mounted ConfigMap is fresh on every periodic sync. However, it uses its 
local TTL-based cache for getting the current value of the ConfigMap. As a result, the total delay from the 
moment when the ConfigMap is updated to the moment when new keys are projected to the pod can 
be as long as kubelet sync period (1 minute by default) + TTL of ConfigMaps cache (1 minute by default) in kubelet.

WORST WORST CASE: 60-90s (although that would be rare)

A hacky way to to fix is.. updating pac pod with an annotation, which triggers kubelet to do it immediately.

  • Operator will need changes to update configmap, as there will be only one field so a little work there

Submitter Checklist

  • 📝 A good commit message is important for other reviewers to understand the context of your change. Please refer to How to Write a Git Commit Message for more details how to write beautiful commit messages. We rather have the commit message in the PR body and the commit message instead of an external website.
  • ♽ Run make test before submitting a PR (ie: with pre-commit, no need to waste CPU cycle on CI. (or even better install pre-commit and do pre-commit install in the root of this repo).
  • ✨ We heavily rely on linters to get our code clean and consistent, please ensure that you have run make lint before submitting a PR. The markdownlint error can get usually fixed by running make fix-markdownlint (make sure it's installed first)
  • 📖 If you are adding a user facing feature or make a change of the behavior, please verify that you have documented it
  • 🧪 100% coverage is not a target but most of the time we would rather have a unit test if you make a code change.
  • 🎁 If that's something that is possible to do please ensure to check if we can add a e2e test.
  • 🔎 If there is a flakiness in the CI tests then don't necessary ignore it, better get the flakyness fixed before merging or if that's not possible there is a good reason to bypass it. (token rate limitation may be a good reason to skip).

@codecov
Copy link

codecov bot commented Sep 4, 2023

Codecov Report

Patch coverage: 49.54% and project coverage change: -0.10% ⚠️

Comparison is base (3efd1a3) 61.65% compared to head (abfb890) 61.55%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1409      +/-   ##
==========================================
- Coverage   61.65%   61.55%   -0.10%     
==========================================
  Files         136      136              
  Lines       10205    10299      +94     
==========================================
+ Hits         6292     6340      +48     
- Misses       3408     3455      +47     
+ Partials      505      504       -1     
Files Changed Coverage Δ
pkg/reconciler/controller.go 29.62% <0.00%> (+2.51%) ⬆️
pkg/params/settings/config.go 53.57% <46.39%> (-5.49%) ⬇️
pkg/adapter/adapter.go 51.91% <100.00%> (+2.18%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@chmouel
Copy link
Member

chmouel commented Sep 5, 2023

awesome thanks, i can live with that tradeoff which is much more efficient than having to reload all config everytime and bring a performance boost... if people are asking we can add a /reload or something to PaC (may be with network ACL)

@sm43 sm43 force-pushed the use-viper-for-config branch 2 times, most recently from 748e357 to e2550c2 Compare September 8, 2023 13:46
@sm43 sm43 changed the title DNM: Use viper for config Uses viper for reading configurations for controller/watcher Sep 8, 2023
@sm43
Copy link
Contributor Author

sm43 commented Sep 8, 2023

@chmouel it would be nice if I can get access to cancel github workflow when not required to run 🙃

Also we export functions for Operator for validation/defaulting
now that we are using viper it wouldn't be straight forward just calling a func

they have a map from TektonConfig CR, they will have to convert into viper and pass to PAC func
so may be give them a heads up for this change.

@@ -71,6 +71,7 @@ jobs:

# restart controller
kubectl -n pipelines-as-code delete pod -l app.kubernetes.io/name=controller
Copy link
Contributor Author

Choose a reason for hiding this comment

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

update..

 kubectl patch configmap -n pipelines-as-code -p "{\"data\":{\"bitbucket-cloud-check-source-ip\": \"false\"}}" \
          --type merge pipelines-as-code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO:

  • cleanup
  • cli changes

Copy link
Member

Choose a reason for hiding this comment

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

what are the cli changes that would be needed?

@chmouel
Copy link
Member

chmouel commented Sep 14, 2023

@piyush-garg do you think that would break the operator integration?

now we mount configurations configmap for both componenet and use viper
to read the configurations.

NOTE: after updating the configmap it takes a little time to reflect in pod
well that's because kubelet sync them on certain interval so if you don't see
immediate change, then wait :P
reflecting change in configuration in pod take time so we can add a wait
or restart pods, we already do controller so now watcher too.
@sm43 sm43 force-pushed the use-viper-for-config branch from e2550c2 to 463efd9 Compare September 22, 2023 08:28
@sm43
Copy link
Contributor Author

sm43 commented Mar 26, 2024

closing for now, incomplete need some work

@sm43 sm43 closed this Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants