[Discuss] Don't panic if server fails to create scheduler

[nanne007]

Please answer these questions before submitting your issue. Thanks!

1. What did you do?
   If possible, provide a recipe for reproducing the error.

From this PR, If server cannot create scheduler, it just panic. However, the reasons of failing to create scheduler is not just come from wrong configurations.
In real case, maybe schedulers itself raise some error, or maybe pd-ctl send an invalid config due to some kind of bugs. The online server cannot work normally again in these cases, because there is no way to delete the wrong config in the etcd kv before PD crashs.

Also, Should we check the disabled scheduler, if it's configured incorrectly, and delete it too?

	for _, schedulerCfg := range scheduleCfg.Schedulers {
		if schedulerCfg.Disable {
			scheduleCfg.Schedulers[k] = schedulerCfg
			k++
			log.Info("skip create scheduler", zap.String("scheduler-type", schedulerCfg.Type))
			continue
		}
		s, err := schedule.CreateScheduler(schedulerCfg.Type, c.opController, schedulerCfg.Args...)
		if err != nil {
			log.Fatal("can not create scheduler", zap.String("scheduler-type", schedulerCfg.Type), zap.Error(err))
		}
		log.Info("create scheduler", zap.String("scheduler-name", s.GetName()))
		if err = c.addScheduler(s, schedulerCfg.Args...); err != nil {
			log.Error("can not add scheduler", zap.String("scheduler-name", s.GetName()), zap.Error(err))
		}

		// Only records the valid scheduler config.
		if err == nil {
			scheduleCfg.Schedulers[k] = schedulerCfg
			k++
		}
	}

2. What did you expect to see?

3. What did you see instead?

4. What version of PD are you using (pd-server -V)?

[rleungx]

@Connor1996 PTAL.

[Connor1996]

seems that even though pd-ctl sends an invalid config, PD won't persist the config.

// AddScheduler adds a scheduler.
func (h *Handler) AddScheduler(name string, args ...string) error {
	c, err := h.getCoordinator()
	if err != nil {
		return err
	}
	s, err := schedule.CreateScheduler(name, c.limiter, args...)
	if err != nil {
		return err
	}
	log.Infof("create scheduler %s", s.GetName())
	if err = c.addScheduler(s, args...); err != nil {
		log.Errorf("can not add scheduler %v: %v", s.GetName(), err)
	} else if err = h.opt.persist(c.cluster.kv); err != nil {
		log.Errorf("can not persist scheduler config: %v", err)
	}
	return err
}

But for the disabled schedulers, seems they should be checked. /cc @nolouch

[nanne007]

In fact, I have a running cluster whose schedule config is a total mess.
It is the result of many times of pd restart and leader switch due to bugs or upgrading.
Because I have done some patches personally, It's hard to distinguish which part make the damage.

What I'm trying to point out is that we need a way to recover from/discard invalid config, instead of panic assuming the underlining etcd data always work perfectly.

[Connor1996]

The panic is to notify users that there may be some typo of the config. If we just discard the invalid config, the PD's behavior may be not as same as what users expect.
I prefer not discarding config here. Do you have any better idea?

[nolouch]

@lerencao We really need a recover way. @Connor1996 How about adding an option let PD in recovery mode and ignore some panics that similar this situation?

[nanne007]

Errorred spell from config file may be checked before ahead. And in real case, I believe most people just use default schedulers, and use pd-ctl to change schedulers dynamically.

[Connor1996]

@lerencao check before ahead seems reasonable and neat

[Connor1996]

Do you want to have a try? @lerencao