-
Notifications
You must be signed in to change notification settings - Fork 25
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
fix: Fix authtoken ticker #114
Conversation
@@ -66,10 +66,7 @@ func main() { | |||
// Stat returns file info. It will return | |||
// an error if there is no file. | |||
_, err := os.Stat(tokenFilePath) | |||
if err != nil { | |||
klog.Error(err.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no more logging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why retry here at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why retry here at all?
The refresh token will take time until the file got to appear. This way will keep the member-agent trying for a couple of time until the file is ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no more logging?
already handled in like 71-74
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why retry here at all?
The refresh token will take time until the file got to appear. This way will keep the member-agent trying for a couple of time until the file is ready.
There is no guarantee that this retry will succeed, right? We can just exit and let the k8s handle the retry too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can, but since the token is written to pod(ephemeral) storage, every time we deploy the member-agent container will initially fail to boot, potentially trigging alerts on deployment. How about increasing backoff
?
@@ -66,10 +66,7 @@ func main() { | |||
// Stat returns file info. It will return | |||
// an error if there is no file. | |||
_, err := os.Stat(tokenFilePath) | |||
if err != nil { | |||
klog.Error(err.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why retry here at all?
The refresh token will take time until the file got to appear. This way will keep the member-agent trying for a couple of time until the file is ready.
There is no guarantee that this retry will succeed, right? We can just exit and let the k8s handle the retry too.
Codecov Report
@@ Coverage Diff @@
## main #114 +/- ##
==========================================
+ Coverage 76.45% 76.55% +0.10%
==========================================
Files 5 5
Lines 671 674 +3
==========================================
+ Hits 513 516 +3
Misses 148 148
Partials 10 10
Continue to review full report at Codecov.
|
Description of your changes
I have:
make reviewable
to ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer