We should be able to share the promhttp handler between the validation and server paths. Consider moving startSelfMonitoring to galley/pkg/server/monitoring.go?

Now there is no metrics available in server, so keep the logics for validation only move it once server have added metrics part?

We'll be adding metrics shortly. Since you're already moving this code around we might as well put it the common place now :)

Done~ Move to galley/pkg/server/galleymonitor.go

We could simplify this a bit by hooking PersistantFlags.StringVar() directly to the server and validation args. e.g.

func GetRootCmd(...) {
    var (
        serverArgs = server.DefaultArgs()
        validationARgs = validadtion.DefaultArgs()
    )

    rootCmd := &cobra.Command{ ... }

    rootCmd.PersistentFlags().BoolVar(&serverArgs.server, "server", "Run galley server mode", server.Args.server)
    // etc
}

This would also keep the server.DefaultArgs() and default flag values in sync.

Done~

Can we reverse the polarity of no-server and no-validation` to avoid double negatives? I think the command line usage would read a bit better, e.g.

galley --no-server=true --no-validation=false
// vs.
galley --enable-server=false --enable-validation

Done~

Should we propagate the galleyStop channel to server.RunServer and validation.RunValidation?

in server.RunServer and validation.RunValidation they will call os.Exit(-1) when they encounter errors. So I think we can use this channel to exit if none of them has errors.

I would declare livenessProbeOptions and readinessProbeOptions as value types instead of pointers and move it to within the scope of GetRootCmd (line ~56). This would eliminate lines 104-105 and the use of global variables.

Done

Missing description for insecure flag

done~

Can we align this with how mixer uses the readiness probe package? Specifically, the probe package itself seems to already handle updating the health file. We only need to invoke SetAvailable(<status>) based on server and validation readiness.

Mixer have not used this part actually because they have not defined liveness and readiness probe options in their helm/docker file. the security part is relying on this mechanism now: https://github.com/istio/istio/blob/master/security/pkg/probe/controller.go , so we are now under the same way with it.

currently I have used the same logic to detect the liveness and readiness for galley:

func (c *ProbeCheckController) checkLiveness() error {
	content := []byte(`ok`)
	if err := ioutil.WriteFile(c.livenessProbeOptions.Path, content, 0644); err != nil {
		log.Errorf("Health check update of %q failed: %v", c.livenessProbeOptions.Path, err)
		return err

	}
	return nil
}


func (c *ProbeCheckController) checkReadiness() error {
	content := []byte(`ok`)
	if err := ioutil.WriteFile(c.readinessProbeOptions.Path, content, 0644); err != nil {
		log.Errorf("Health check update of %q failed: %v", c.readinessProbeOptions.Path, err)
		return err

	}
	return nil
}

So in case there is error, the probe package itself will get the error and remove the heath file.

So is above logic valid to check the galley liveness and readiness? (Just copy the logic from validation code)?

See istio#7986 (release-1.0 branch). This change should also be merged into master. This decouples the config reconciliation loop from the health check updates. To do this, I used the istio.io/pkg/probe package instead of writing the file directly. We should use the same mechanism for the galley service health checks.

Done~~ Thanks for the clue 👍

FYI, you'll need to run make format and make lint to help fix the formatting and linter issues before the PR can be merged.

sure, Thanks~