Implementing PR feedback

plugin/cmd/kube-scheduler/app/server.go

@@ -101,7 +101,6 @@ func (s *SchedulerServer) createConfig(configFactory *factory.ConfigFactory) (*s
 		if err != nil {
 			return nil, fmt.Errorf("Unable to read policy config: %v", err)
 		}
-		//err = json.Unmarshal(configData, &policy)
 		err = latestschedulerapi.Codec.DecodeInto(configData, &policy)
 		if err != nil {
 			return nil, fmt.Errorf("Invalid configuration: %v", err)

plugin/pkg/scheduler/api/latest/latest.go

@@ -23,8 +23,7 @@ import (
 // Version is the string that represents the current external default version.
 const Version = "v1"
 
-// OldestVersion is the string that represents the oldest server version supported,
-// for client code that wants to hardcode the lowest common denominator.
+// OldestVersion is the string that represents the oldest server version supported.
 const OldestVersion = "v1"
 
 // Versions is the list of versions that are recognized in code. The order provided
@@ -33,8 +32,7 @@ const OldestVersion = "v1"
 // with a set of versions to choose.
 var Versions = []string{"v1"}
 
-// Codec is the default codec for serializing output that should use
-// the latest supported version.  Use this Codec when writing to
-// disk, a data store that is not dynamically versioned, or in tests.
+// Codec is the default codec for serializing input that should use
+// the latest supported version.  Use this Codec when reading from the file.
 // This codec can decode any object that Kubernetes is aware of.
 var Codec = v1.Codec

plugin/pkg/scheduler/api/types.go

@@ -20,54 +20,84 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 )
 
-// Where possible, json tags match the cli argument names.
-// Top level config objects and all values required for proper functioning are not "omitempty".  Any truly optional piece of config is allowed to be omitted.
-
 type Policy struct {
 	api.TypeMeta `json:",inline"`
-	Predicates   []PredicatePolicy `json:"predicates"`
-	Priorities   []PriorityPolicy  `json:"priorities"`
+	// Holds the information to configure the fit predicate functions
+	Predicates []PredicatePolicy `json:"predicates"`
+	// Holds the information to configure the priority functions
+	Priorities []PriorityPolicy `json:"priorities"`
 }
 
 type PredicatePolicy struct {
-	Name     string             `json:"name"`
+	// Identifier of the predicate policy
+	// For a custom predicate, the name can be user-defined
+	// For the Kubernetes provided predicates, the name is the identifier of the pre-defined predicate
+	Name string `json:"name"`
+	// Holds the parameters to configure the given predicate
 	Argument *PredicateArgument `json:"argument"`
 }
 
 type PriorityPolicy struct {
-	Name     string            `json:"name"`
-	Weight   int               `json:"weight"`
+	// Identifier of the priority policy
+	// For a custom priority, the name can be user-defined
+	// For the Kubernetes provided priority functions, the name is the identifier of the pre-defined priority function
+	Name string `json:"name"`
+	// The numeric multiplier for the minion scores that the priority function generates
+	Weight int `json:"weight"`
+	// Holds the parameters to configure the given priority function
 	Argument *PriorityArgument `json:"argument"`
 }
 
-// PredicateArgument represents the arguments that the different types of predicates take.
-// Only one of its members may be specified.
+// Represents the arguments that the different types of predicates take
+// Only one of its members may be specified
 type PredicateArgument struct {
+	// The predicate that provides affinity for pods belonging to a service
+	// It uses a label to identify minions that belong to the same "group"
 	ServiceAffinity *ServiceAffinity `json:"serviceAffinity"`
-	LabelsPresence  *LabelsPresence  `json:"labelsPresence"`
+	// The predicate that checks whether a particular minion has a certain label
+	// defined or not, regardless of value
+	LabelsPresence *LabelsPresence `json:"labelsPresence"`
 }
 
-// PriorityArgument represents the arguments that the different types of priorities take.
-// Only one of its members may be specified.
+// Represents the arguments that the different types of priorities take.
+// Only one of its members may be specified
 type PriorityArgument struct {
+	// The priority function that ensures a good spread (anti-affinity) for pods belonging to a service
+	// It uses a label to identify minions that belong to the same "group"
 	ServiceAntiAffinity *ServiceAntiAffinity `json:"serviceAntiAffinity"`
-	LabelPreference     *LabelPreference     `json:"labelPreference"`
+	// The priority function that checks whether a particular minion has a certain label
+	// defined or not, regardless of value
+	LabelPreference *LabelPreference `json:"labelPreference"`
 }
 
+// Holds the parameters that are used to configure the corresponding predicate
 type ServiceAffinity struct {
+	// The list of labels that identify minion "groups"
+	// All of the labels should match for the minion to be considered a fit for hosting the pod
 	Labels []string `json:"labels"`
 }
 
+// Holds the parameters that are used to configure the corresponding predicate
 type LabelsPresence struct {
-	Labels   []string `json:"labels"`
-	Presence bool     `json:"presence"`
+	// The list of labels that identify minion "groups"
+	// All of the labels should be either present (or absent) for the minion to be considered a fit for hosting the pod
+	Labels []string `json:"labels"`
+	// The boolean flag that indicates whether the labels should be present or absent from the minion
+	Presence bool `json:"presence"`
 }
 
+// Holds the parameters that are used to configure the corresponding priority function
 type ServiceAntiAffinity struct {
+	// Used to identify minion "groups"
 	Label string `json:"label"`
 }
 
+// Holds the parameters that are used to configure the corresponding priority function
 type LabelPreference struct {
-	Label    string `json:"label"`
-	Presence bool   `json:"presence"`
+	// Used to identify minion "groups"
+	Label string `json:"label"`
+	// This is a boolean flag
+	// If true, higher priority is given to minions that have the label
+	// If false, higher priority is given to minions that do not have the label
+	Presence bool `json:"presence"`
 }

plugin/pkg/scheduler/api/v1/types.go

@@ -20,86 +20,83 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta3"
 )
 
-// Where possible, json tags match the cli argument names.
-// Top level config objects and all values required for proper functioning are not "omitempty".  Any truly optional piece of config is allowed to be omitted.
-
 type Policy struct {
 	v1beta3.TypeMeta `json:",inline"`
-	// Predicates holds the information to configure the fit predicate functions
+	// Holds the information to configure the fit predicate functions
 	Predicates []PredicatePolicy `json:"predicates"`
-	// Priorities holds the information to configure the priority functions
+	// Holds the information to configure the priority functions
 	Priorities []PriorityPolicy `json:"priorities"`
 }
 
 type PredicatePolicy struct {
-	// Name is the identifier of the predicate policy
+	// Identifier of the predicate policy
 	// For a custom predicate, the name can be user-defined
 	// For the Kubernetes provided predicates, the name is the identifier of the pre-defined predicate
 	Name string `json:"name"`
-	// Argument holds the parameters to configure the given predicate
+	// Holds the parameters to configure the given predicate
 	Argument *PredicateArgument `json:"argument"`
 }
 
 type PriorityPolicy struct {
-	// Name is the identifier of the priority policy
+	// Identifier of the priority policy
 	// For a custom priority, the name can be user-defined
 	// For the Kubernetes provided priority functions, the name is the identifier of the pre-defined priority function
 	Name string `json:"name"`
-	// Weight is the numeric multiplier for the minion scores that the priority function generates
+	// The numeric multiplier for the minion scores that the priority function generates
 	Weight int `json:"weight"`
-	// Argument holds the parameters to configure the given priority function
+	// Holds the parameters to configure the given priority function
 	Argument *PriorityArgument `json:"argument"`
 }
 
-// PredicateArgument represents the arguments that the different types of predicates take
+// Represents the arguments that the different types of predicates take
 // Only one of its members may be specified
 type PredicateArgument struct {
-	// ServiceAffinity is the predicate that provides affinity for pods belonging to a service
+	// The predicate that provides affinity for pods belonging to a service
 	// It uses a label to identify minions that belong to the same "group"
 	ServiceAffinity *ServiceAffinity `json:"serviceAffinity"`
-	// LabelsPresence is the predicate that checks whether a particular minion has a certain label
+	// The predicate that checks whether a particular minion has a certain label
 	// defined or not, regardless of value
 	LabelsPresence *LabelsPresence `json:"labelsPresence"`
 }
 
-// PriorityArgument represents the arguments that the different types of priorities take.
+// Represents the arguments that the different types of priorities take.
 // Only one of its members may be specified
 type PriorityArgument struct {
-	// ServiceAntiAffinity is the priority function that ensures a good spread (anti-affinity) for pods belonging to a service
+	// The priority function that ensures a good spread (anti-affinity) for pods belonging to a service
 	// It uses a label to identify minions that belong to the same "group"
 	ServiceAntiAffinity *ServiceAntiAffinity `json:"serviceAntiAffinity"`
-	// LabelPreference is the priority function that checks whether a particular minion has a certain label
+	// The priority function that checks whether a particular minion has a certain label
 	// defined or not, regardless of value
 	LabelPreference *LabelPreference `json:"labelPreference"`
 }
 
-// ServiceAffinity holds the parameters that are used to configure the corresponding predicate
+// Holds the parameters that are used to configure the corresponding predicate
 type ServiceAffinity struct {
-	// Labels is the list of labels that identify minion "groups"
+	// The list of labels that identify minion "groups"
 	// All of the labels should match for the minion to be considered a fit for hosting the pod
 	Labels []string `json:"labels"`
 }
 
-// LabelsPresence holds the parameters that are used to configure the corresponding predicate
+// Holds the parameters that are used to configure the corresponding predicate
 type LabelsPresence struct {
-	// Labels is the list of labels that identify minion "groups"
+	// The list of labels that identify minion "groups"
 	// All of the labels should be either present (or absent) for the minion to be considered a fit for hosting the pod
 	Labels []string `json:"labels"`
-	// Presence is the boolean flag that indicates whether the labels should be present or absent from the minion
+	// The boolean flag that indicates whether the labels should be present or absent from the minion
 	Presence bool `json:"presence"`
 }
 
-// ServiceAntiAffinity holds the parameters that are used to configure the corresponding priority function
+// Holds the parameters that are used to configure the corresponding priority function
 type ServiceAntiAffinity struct {
-	// Label is used to identify minion "groups"
+	// Used to identify minion "groups"
 	Label string `json:"label"`
 }
 
-// LabelPreference holds the parameters that are used to configure the corresponding priority function
+// Holds the parameters that are used to configure the corresponding priority function
 type LabelPreference struct {
-	// Label is used to identify minion "groups"
+	// Used to identify minion "groups"
 	Label string `json:"label"`
-	// Presence is a boolean flag
+	// This is a boolean flag
 	// If true, higher priority is given to minions that have the label
 	// If false, higher priority is given to minions that do not have the label
 	Presence bool `json:"presence"`

plugin/pkg/scheduler/factory/factory.go

@@ -54,7 +54,7 @@ type ConfigFactory struct {
 	ServiceLister *cache.StoreToServiceLister
 }
 
-// NewConfigFactory initializes the factory.
+// Initializes the factory.
 func NewConfigFactory(client *client.Client) *ConfigFactory {
 	return &ConfigFactory{
 		Client:        client,
@@ -70,7 +70,7 @@ func (f *ConfigFactory) Create() (*scheduler.Config, error) {
 	return f.CreateFromProvider(DefaultProvider)
 }
 
-// CreateFromProvider creates a scheduler from the name of a registered algorithm provider.
+// Creates a scheduler from the name of a registered algorithm provider.
 func (f *ConfigFactory) CreateFromProvider(providerName string) (*scheduler.Config, error) {
 	glog.V(2).Infof("creating scheduler from algorithm provider '%v'", providerName)
 	provider, err := GetAlgorithmProvider(providerName)
@@ -81,14 +81,14 @@ func (f *ConfigFactory) CreateFromProvider(providerName string) (*scheduler.Conf
 	return f.CreateFromKeys(provider.FitPredicateKeys, provider.PriorityFunctionKeys)
 }
 
-// CreateFromConfig creates a scheduler from the configuration file
+// Creates a scheduler from the configuration file
 func (f *ConfigFactory) CreateFromConfig(policy schedulerapi.Policy) (*scheduler.Config, error) {
 	glog.V(2).Infof("creating scheduler from configuration: %v", policy)
 
 	predicateKeys := util.NewStringSet()
 	for _, predicate := range policy.Predicates {
 		glog.V(2).Infof("Registering predicate: %s", predicate.Name)
-		predicateKeys.Insert(RegisterCustomPredicate(predicate))
+		predicateKeys.Insert(RegisterCustomFitPredicate(predicate))
 	}
 
 	priorityKeys := util.NewStringSet()
@@ -100,7 +100,7 @@ func (f *ConfigFactory) CreateFromConfig(policy schedulerapi.Policy) (*scheduler
 	return f.CreateFromKeys(predicateKeys, priorityKeys)
 }
 
-// CreateFromKeys creates a scheduler from a set of registered fit predicate keys and priority keys.
+// Creates a scheduler from a set of registered fit predicate keys and priority keys.
 func (f *ConfigFactory) CreateFromKeys(predicateKeys, priorityKeys util.StringSet) (*scheduler.Config, error) {
 	glog.V(2).Infof("creating scheduler with fit predicates '%v' and priority functions '%v", predicateKeys, priorityKeys)
 	predicateFuncs, err := getFitPredicateFunctions(predicateKeys)
@@ -160,7 +160,7 @@ func (f *ConfigFactory) CreateFromKeys(predicateKeys, priorityKeys util.StringSe
 	}, nil
 }
 
-// createUnassignedPodLW returns a cache.ListWatch that finds all pods that need to be
+// Returns a cache.ListWatch that finds all pods that need to be
 // scheduled.
 func (factory *ConfigFactory) createUnassignedPodLW() *cache.ListWatch {
 	return cache.NewListWatchFromClient(factory.Client, "pods", api.NamespaceAll, labels.Set{"DesiredState.Host": ""}.AsSelector())
@@ -174,7 +174,7 @@ func parseSelectorOrDie(s string) labels.Selector {
 	return selector
 }
 
-// createAssignedPodLW returns a cache.ListWatch that finds all pods that are
+// Returns a cache.ListWatch that finds all pods that are
 // already scheduled.
 // TODO: return a ListerWatcher interface instead?
 func (factory *ConfigFactory) createAssignedPodLW() *cache.ListWatch {
@@ -186,7 +186,7 @@ func (factory *ConfigFactory) createMinionLW() *cache.ListWatch {
 	return cache.NewListWatchFromClient(factory.Client, "minions", api.NamespaceAll, parseSelectorOrDie(""))
 }
 
-// pollMinions lists all minions and filter out unhealthy ones, then returns
+// Lists all minions and filter out unhealthy ones, then returns
 // an enumerator for cache.Poller.
 func (factory *ConfigFactory) pollMinions() (cache.Enumerator, error) {
 	allNodes := &api.NodeList{}
@@ -221,7 +221,7 @@ func (factory *ConfigFactory) pollMinions() (cache.Enumerator, error) {
 	return &nodeEnumerator{nodes}, nil
 }
 
-// createServiceLW returns a cache.ListWatch that gets all changes to services.
+// Returns a cache.ListWatch that gets all changes to services.
 func (factory *ConfigFactory) createServiceLW() *cache.ListWatch {
 	return cache.NewListWatchFromClient(factory.Client, "services", api.NamespaceAll, parseSelectorOrDie(""))
 }
@@ -251,7 +251,7 @@ func (factory *ConfigFactory) makeDefaultErrorFunc(backoff *podBackoff, podQueue
 	}
 }
 
-// nodeEnumerator allows a cache.Poller to enumerate items in an api.NodeList
+// Allows a cache.Poller to enumerate items in an api.NodeList
 type nodeEnumerator struct {
 	*api.NodeList
 }

plugin/pkg/scheduler/factory/plugins.go

@@ -46,7 +46,7 @@ type AlgorithmProviderConfig struct {
 	PriorityFunctionKeys util.StringSet
 }
 
-// RegisterFitPredicate registers a fit predicate with the algorithm registry. Returns the name,
+// Registers a fit predicate with the algorithm registry. Returns the name,
 // with which the predicate was registered.
 func RegisterFitPredicate(name string, predicate algorithm.FitPredicate) string {
 	schedulerFactoryMutex.Lock()
@@ -56,9 +56,9 @@ func RegisterFitPredicate(name string, predicate algorithm.FitPredicate) string
 	return name
 }
 
-// RegisterCustomPredicate registers a custom fit predicate with the algorithm registry.
+// Registers a custom fit predicate with the algorithm registry.
 // Returns the name, with which the predicate was registered.
-func RegisterCustomPredicate(policy schedulerapi.PredicatePolicy) string {
+func RegisterCustomFitPredicate(policy schedulerapi.PredicatePolicy) string {
 	var predicate algorithm.FitPredicate
 	var ok bool
 
@@ -83,7 +83,7 @@ func RegisterCustomPredicate(policy schedulerapi.PredicatePolicy) string {
 	return RegisterFitPredicate(policy.Name, predicate)
 }
 
-// IsFitPredicateRegistered check is useful for testing providers.
+// This check is useful for testing providers.
 func IsFitPredicateRegistered(name string) bool {
 	schedulerFactoryMutex.Lock()
 	defer schedulerFactoryMutex.Unlock()
@@ -91,7 +91,7 @@ func IsFitPredicateRegistered(name string) bool {
 	return ok
 }
 
-// RegisterPriorityFunction registers a priority function with the algorithm registry. Returns the name,
+// Registers a priority function with the algorithm registry. Returns the name,
 // with which the function was registered.
 func RegisterPriorityFunction(name string, function algorithm.PriorityFunction, weight int) string {
 	schedulerFactoryMutex.Lock()
@@ -101,7 +101,7 @@ func RegisterPriorityFunction(name string, function algorithm.PriorityFunction,
 	return name
 }
 
-// RegisterCustomPriority registers a custom priority function with the algorithm registry.
+// Registers a custom priority function with the algorithm registry.
 // Returns the name, with which the priority function was registered.
 func RegisterCustomPriorityFunction(policy schedulerapi.PriorityPolicy) string {
 	var priority algorithm.PriorityFunction
@@ -127,7 +127,7 @@ func RegisterCustomPriorityFunction(policy schedulerapi.PriorityPolicy) string {
 	return RegisterPriorityFunction(policy.Name, priority, policy.Weight)
 }
 
-// IsPriorityFunctionRegistered check is useful for testing providers.
+// This check is useful for testing providers.
 func IsPriorityFunctionRegistered(name string) bool {
 	schedulerFactoryMutex.Lock()
 	defer schedulerFactoryMutex.Unlock()
@@ -135,7 +135,7 @@ func IsPriorityFunctionRegistered(name string) bool {
 	return ok
 }
 
-// SetPriorityFunctionWeight sets the weight of an already registered priority function.
+// Sets the weight of an already registered priority function.
 func SetPriorityFunctionWeight(name string, weight int) {
 	schedulerFactoryMutex.Lock()
 	defer schedulerFactoryMutex.Unlock()
@@ -148,7 +148,7 @@ func SetPriorityFunctionWeight(name string, weight int) {
 	priorityFunctionMap[name] = config
 }
 
-// RegisterAlgorithmProvider registers a new algorithm provider with the algorithm registry. This should
+// Registers a new algorithm provider with the algorithm registry. This should
 // be called from the init function in a provider plugin.
 func RegisterAlgorithmProvider(name string, predicateKeys, priorityKeys util.StringSet) string {
 	schedulerFactoryMutex.Lock()
@@ -161,7 +161,7 @@ func RegisterAlgorithmProvider(name string, predicateKeys, priorityKeys util.Str
 	return name
 }
 
-// GetAlgorithmProvider should not be used to modify providers. It is publicly visible for testing.
+// This function should not be used to modify providers. It is publicly visible for testing.
 func GetAlgorithmProvider(name string) (*AlgorithmProviderConfig, error) {
 	schedulerFactoryMutex.Lock()
 	defer schedulerFactoryMutex.Unlock()