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

NET-655 #628

Merged
merged 25 commits into from
Dec 21, 2023
Merged

NET-655 #628

merged 25 commits into from
Dec 21, 2023

Conversation

uGiFarukh
Copy link
Contributor

@uGiFarukh uGiFarukh commented Nov 10, 2023

Describe your changes

  • When MQ connection is broken, a fallback thread is being ran every 30s for calling netclient http config pull.
  • If peer updates are not received or lost, the same fallback thread will run every 30s for calling netclient http config pull.

Related to -> gravitl/netmaker#2670

Provide Issue ticket number if applicable/not in title

Provide link to Netmaker PR if required

Provide testing steps

Checklist before requesting a review

  • My changes affect only 10 files or less.
  • I have performed a self-review of my code and tested it.
  • If it is a new feature, I have added thorough tests, my code is <= 1450 lines.
  • If it is a bugfix, my code is <= 200 lines.
  • My functions are <= 80 lines.
  • I have had my code reviewed by a peer.
  • My unit tests pass locally.
  • Netclient & Netmaker are awesome.

* draft code for NET-655
@uGiFarukh uGiFarukh added enhancement New feature or request help wanted Extra attention is needed labels Nov 10, 2023
@uGiFarukh uGiFarukh self-assigned this Nov 10, 2023
Copy link
Contributor

@mattkasun mattkasun left a comment

Choose a reason for hiding this comment

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

would it not be simpler to start the fallback go routine in the mq connection lost handler and stop it in the mq connection handler

functions/mqhandlers.go Outdated Show resolved Hide resolved
functions/daemon.go Outdated Show resolved Hide resolved
functions/daemon.go Outdated Show resolved Hide resolved
functions/daemon.go Outdated Show resolved Hide resolved
functions/mqhandlers.go Outdated Show resolved Hide resolved
functions/daemon.go Show resolved Hide resolved
* Added ticker for MQ Fallback Main thread.
* Added wait groups so that threads close properly.
* Added atomic sync for thread status boolean.
functions/daemon.go Outdated Show resolved Hide resolved
functions/mqhandlers.go Outdated Show resolved Hide resolved
functions/daemon.go Outdated Show resolved Hide resolved
* Fixed variables scoping
* Added fallback pull function
* fixed server version empty string
@uGiFarukh uGiFarukh marked this pull request as ready for review November 16, 2023 11:37
@uGiFarukh uGiFarukh changed the title NET-655 draft NET-655 Nov 16, 2023
@uGiFarukh uGiFarukh mentioned this pull request Nov 16, 2023
8 tasks
Copy link
Contributor

@mattkasun mattkasun left a comment

Choose a reason for hiding this comment

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

I think this is way over complicated with starting stopping the fallback function all over the place:

just need to start fallback go routine and let it run


func MQFallback(ctx contex.Context, wg *sync.WaitGroup) {
	defer wg.Done()
	for {
		select {
		case <-ctx.Done():
			fallbackTicker.Stop()
			return
		case <-fallbackTicker.C:
			if Mqclient == nil || !Mqclient.IsConnected() {
				response, err := pull(false)
				if err != nil {
					slog.Error("pull failed", "error", err)
				} else {
					MQFallbackPull(response)
				}
			}
		}
	}
}

* fallback mechanism optimization
* further optimization of mq fallback goroutine.
functions/mqhandlers.go Outdated Show resolved Hide resolved
* reset interface if new node.
* fallback goroutine context and waitgroup changed.
* mq connection re-attempt after successful pull.
* reset interface network change scenario
@abhishek9686 abhishek9686 merged commit daa7a59 into develop Dec 21, 2023
7 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants