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

Align the message with master plugin restoration logic #1190

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kichankwon
Copy link

@kichankwon kichankwon commented Nov 26, 2023

thin_entrypoint prints that it allows 45 seconds for its restoration, but it waits until "masterConfigFilePath" exists.
So, adjust the message to align with this logic.

@s1061123
Copy link
Member

Thank you for the PR. Good catch that mismatch between comment and actual code. But I would like to know why you fix to 45sec wait, not wait forever. If we cannot find master config, then your change makes the pod quit and launch it again...

How about to change to wait forever until the master config found?

@kichankwon
Copy link
Author

Thank you for the PR. Good catch that mismatch between comment and actual code. But I would like to know why you fix to 45sec wait, not wait forever. If we cannot find master config, then your change makes the pod quit and launch it again...

How about to change to wait forever until the master config found?

I fixed to 45 seconds wait because..

  • I thought there's a mistake in the message or logic
  • And I found that old shell script(images/entrypoint.sh) waits 45 seconds just before it got removed
  • So, I decided to fix the logic

But, I agree with you. So, I'm going to fix the message not the logic.

@kichankwon kichankwon changed the title Align master plugin restoration logic with message Align the message with master plugin restoration logic Dec 13, 2023
@coveralls
Copy link

Coverage Status

coverage: 63.211% (-0.3%) from 63.468%
when pulling 4f7dce6 on kichankwon:set_timeout
into acfbd42 on k8snetworkplumbingwg:master.

@kichankwon
Copy link
Author

@s1061123 Could you check this PR?

@kichankwon
Copy link
Author

@s1061123 Can I ping you for a review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants