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

Scenario manager #121

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

Raguideau
Copy link
Contributor

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have read the CONTRIBUTING document.
  • Each of my commits messages include Signed-off-by: Author Name <[email protected]> to accept the DCO.

@Raguideau Raguideau requested a review from linouxis9 as a code owner April 17, 2024 14:32
Copy link
Member

@linouxis9 linouxis9 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your PR, it indeed something we need a lot!
One thing that comes to mind, is that by isolating a Scenario per UE, it might be harder to manage things like task rates (eg. Scenario has 10 000 UEs registered, and wants to use some of them to do 100 PDU Session creation per second, so it must be able to dynamically tweak the rate of all UE to reach 100 TX/s).
Could we avoid having an ueTaskExecuter pe UE, bring back the code of the ueTaskExecuter into something like a scenarioExecutor which I guess scenarioManager is kind of, that manages all UEs requests and gNodeBs?
I've offered a few comments on the code with the design as-is. Thanks a lot, it's quite great!


time.Sleep(time.Second * 1)
gnbId = fmt.Sprintf("%06x", base)
Copy link
Member

Choose a reason for hiding this comment

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

should %06X, see: #107

return gnb
}

func genereateGnbId(i int, gnbId string) string {
Copy link
Member

Choose a reason for hiding this comment

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

generate

log.Info("------------------------------------ Scenario finished ------------------------------------")
}

func initManager(ueScenarios []UEScenario, timeBetweenRegistration int, timeout int) *ScenarioManager {
Copy link
Member

Choose a reason for hiding this comment

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

please add line breaks to group together similar things in your code, it makes the code a bit more readable :D
ex:

	log.Debug("Init manager with timeBetweenRegistration = ", timeBetweenRegistration)
	s := &ScenarioManager{}

	s.ues = make(map[int]*ueTasksCtx)

	s.taskExecuterWg = &sync.WaitGroup{}
	s.gnbWg = &sync.WaitGroup{}

	s.reportChan = make(chan report, 10)
	s.sigStop = make(chan os.Signal, 1)
	signal.Notify(s.sigStop, os.Interrupt)

	s.registrationQueue = make(chan order, len(ueScenarios))
	go startOrderRateController(s.registrationQueue, timeBetweenRegistration)

	if timeout > 0 {
		time.AfterFunc(time.Duration(timeout)*time.Millisecond, func() {
			log.Debug("Scenario Timeout, sending interrupt signal")
			s.sigStop <- os.Interrupt
		})
	}

select {
case <-s.sigStop:
s.stop = true
ueId = len(ueScenarios) + 1 // exit loop
Copy link
Member

Choose a reason for hiding this comment

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

smart :D but just return would work
do we actually need this select / check at this step though?

case report := <-s.reportChan:
s.handleReport(report)
if err := s.manageNextTask(report.ueId, s.ueScenarios[report.ueId-1]); err != nil {
if !s.ueScenarios[report.ueId-1].Hang {
Copy link
Member

Choose a reason for hiding this comment

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

Hang?

}
}

func (s *ScenarioManager) cleanup() {
Copy link
Member

Choose a reason for hiding this comment

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

you should reorder the functions in this file in the order they are first seen / or when used in the flow of the lifecycle of this file (cleanup should be probably more towards the bottom)

log.Errorf("[UE-%d] Reported error: %s", report.ueId, report.reason)
return
}
log.Infof("[UE-%d] Reported succes: %s", report.ueId, report.reason)
Copy link
Member

Choose a reason for hiding this comment

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

Very cool to finally have the beginnings of reports! :D

time.Sleep(2 * time.Second)
}

func (s *ScenarioManager) setupUeTaskExecuter(ueId int, ueScenario UEScenario) error {
Copy link
Member

Choose a reason for hiding this comment

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

Executor?

Signed-off-by: Raguideau <[email protected]>
Signed-off-by: Raguideau <[email protected]>
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.

2 participants