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

[BUG] Issues in startEtcd w.r.t select clause #12

Open
unmarshall opened this issue Aug 4, 2023 · 0 comments
Open

[BUG] Issues in startEtcd w.r.t select clause #12

unmarshall opened this issue Aug 4, 2023 · 0 comments
Labels
kind/bug Bug lifecycle/stale Nobody worked on this for 6 months (will further age)
Milestone

Comments

@unmarshall
Copy link
Contributor

unmarshall commented Aug 4, 2023

Describe the bug:

In startEtcd method the current select clause can exit pretty soon as the default value of waitReadyTimeout is 0. It was initially assumed that a value of 0 would signify wait forever but that is now how time.After with a 0 duration would behave. This would result in an uninitialised etcd being assigned here.

In addition in cases where there is a timeout defined or there is a msg on etcd.Server.StopNotify() channel, it currently only logs the error and continues which could result in a stopped etcd being used. This is incorrect. One should return an error in these cases.

Expected behavior:
The assignment of etcd should be done only after a msg has been received on etcd.Server.ReadyNotify() channel. Also an error should be returned back to the caller when either a timeout has happened (if a non-negative and non-zero timeout has been defined) or etcd.Server.StopNotify() has received a msg.

A better way to wait for forever (if no timeout has been defined) is to do the following:

var timeoutCh <-chan time.Time
if a.waitReadyTimeout > 0 {
   ticker := time.NewTicker(a.waitReadyTimeout)
   defer ticker.Close()
   timeoutCh = ticker.C
}

select {
	case <-etcd.Server.ReadyNotify():
		a.logger.Info("etcd server is now ready to serve client requests")
	case <-etcd.Server.StopNotify():
		return fmt.Errorf("etcd server has been aborted, received notification on StopNotify channel")
	case <-timeoutCh:
		return fmt.Errorf("timeout after %s seconds waiting for ReadyNotify signal, aborting start of etcd", a.waitReadyTimeout.String())
	}
}

Trick: Read from a nil channel blocks forever.

Please also write unit tests for app.go which are currently missing. This should have been caught as part of unit tests.

@unmarshall unmarshall added the kind/bug Bug label Aug 4, 2023
@shreyas-s-rao shreyas-s-rao added this to the v0.2.0 milestone Aug 12, 2023
@gardener-robot gardener-robot added the lifecycle/stale Nobody worked on this for 6 months (will further age) label Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Bug lifecycle/stale Nobody worked on this for 6 months (will further age)
Projects
None yet
Development

No branches or pull requests

3 participants