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

Silence errors for namespace listing #725

Merged
merged 1 commit into from
Dec 5, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions chart/openfaas/templates/gateway-dep.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,8 @@ spec:
value: "{{ .Values.faasnetes.livenessProbe.timeoutSeconds }}"
- name: liveness_probe_period_seconds
value: "{{ .Values.faasnetes.livenessProbe.periodSeconds }}"
- name: cluster_role
value: "{{ .Values.clusterRole }}"
volumeMounts:
- mountPath: /tmp
name: faas-netes-temp-volume
Expand Down
8 changes: 4 additions & 4 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ func main() {
runController(setup)
} else {
log.Println("Starting operator")
runOperator(setup)
runOperator(setup, config)
}
}

Expand Down Expand Up @@ -178,14 +178,14 @@ func runController(setup serverSetup) {
InfoHandler: handlers.MakeInfoHandler(version.BuildVersion(), version.GitCommit),
SecretHandler: handlers.MakeSecretHandler(config.DefaultFunctionNamespace, kubeClient),
LogHandler: logs.NewLogHandlerFunc(k8s.NewLogRequestor(kubeClient, config.DefaultFunctionNamespace), config.FaaSConfig.WriteTimeout),
ListNamespaceHandler: handlers.MakeNamespacesLister(config.DefaultFunctionNamespace, kubeClient),
ListNamespaceHandler: handlers.MakeNamespacesLister(config.DefaultFunctionNamespace, config.ClusterRole, kubeClient),
}

faasProvider.Serve(&bootstrapHandlers, &config.FaaSConfig)
}

// runOperator runs the CRD Operator
func runOperator(setup serverSetup) {
func runOperator(setup serverSetup, cfg config.BootstrapConfig) {
// pull out the required config and clients fromthe setup, this is largely a
// leftover from refactoring the setup to a shared step and keeping the function
// signature readable
Expand Down Expand Up @@ -218,7 +218,7 @@ func runOperator(setup serverSetup) {
factory,
)

srv := server.New(faasClient, kubeClient, endpointsInformer, deploymentInformer)
srv := server.New(faasClient, kubeClient, endpointsInformer, deploymentInformer, cfg.ClusterRole)

go faasInformerFactory.Start(stopCh)
go kubeInformerFactory.Start(stopCh)
Expand Down
17 changes: 10 additions & 7 deletions pkg/handlers/namespaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,26 +19,29 @@ import (
)

// MakeNamespacesLister builds a list of namespaces with an "openfaas" tag, or the default name
func MakeNamespacesLister(defaultNamespace string, clientset kubernetes.Interface) http.HandlerFunc {
func MakeNamespacesLister(defaultNamespace string, clusterRole bool, clientset kubernetes.Interface) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
log.Println("Query namespaces")
// log.Println("List namespaces")
Copy link
Member

Choose a reason for hiding this comment

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

Just delete it

Suggested change
// log.Println("List namespaces")

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I am still undecided about whether we should delete all happy-path logging and focus on the Prometheus metrics gathered from the gateway.


if r.Body != nil {
defer r.Body.Close()
}

res := ListNamespaces(defaultNamespace, clientset)
namespaces := []string{}
if clusterRole {
namespaces = ListNamespaces(defaultNamespace, clientset)
} else {
namespaces = append(namespaces, defaultNamespace)
}
Comment on lines +30 to +35
Copy link
Member

Choose a reason for hiding this comment

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

We could slightly simplify this using

Suggested change
namespaces := []string{}
if clusterRole {
namespaces = ListNamespaces(defaultNamespace, clientset)
} else {
namespaces = append(namespaces, defaultNamespace)
}
namespaces := []string{defaultNamespace}
if clusterRole {
namespaces = ListNamespaces(defaultNamespace, clientset)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Very open to a PR.


out, err := json.Marshal(res)
out, err := json.Marshal(namespaces)
if err != nil {
glog.Errorf("Failed to list namespaces: %s", err.Error())
w.WriteHeader(http.StatusInternalServerError)
w.Write([]byte("Failed to list namespaces"))
http.Error(w, "Failed to list namespaces", http.StatusInternalServerError)
return
}

w.Header().Set("Content-Type", "application/json")

w.WriteHeader(http.StatusOK)
w.Write(out)
}
Expand Down
6 changes: 4 additions & 2 deletions pkg/handlers/replicas.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"io/ioutil"
"log"
"net/http"
"time"

"github.com/gorilla/mux"
"github.com/openfaas/faas-provider/types"
Expand Down Expand Up @@ -102,6 +103,7 @@ func MakeReplicaReader(defaultNamespace string, clientset *kubernetes.Clientset)
lookupNamespace = namespace
}

s := time.Now()
Copy link
Member

Choose a reason for hiding this comment

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

This might be more valuable as an actual prometheus metric. Having it in the logs is nice, but it means a person is going to make judgments on a few values as opposed to being able to collect and aggregate real data into graphs etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. I think it is in there already 👍 This is another example of a log message we should probably delete.

function, err := getService(lookupNamespace, functionName, clientset)
if err != nil {
log.Printf("Unable to fetch service: %s %s\n", functionName, namespace)
Expand All @@ -113,8 +115,8 @@ func MakeReplicaReader(defaultNamespace string, clientset *kubernetes.Clientset)
w.WriteHeader(http.StatusNotFound)
return
}

log.Printf("Read replicas - %s %s, %d/%d\n", functionName, lookupNamespace, function.AvailableReplicas, function.Replicas)
d := time.Since(s)
log.Printf("Replicas: %s.%s, (%d/%d)\t%dms\n", functionName, lookupNamespace, function.AvailableReplicas, function.Replicas, d.Milliseconds())

functionBytes, err := json.Marshal(function)
if err != nil {
Expand Down
5 changes: 3 additions & 2 deletions pkg/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ const defaultWriteTimeout = 8
func New(client clientset.Interface,
kube kubernetes.Interface,
endpointsInformer coreinformer.EndpointsInformer,
deploymentsInformer appsinformer.DeploymentInformer) *Server {
deploymentsInformer appsinformer.DeploymentInformer,
clusterRole bool) *Server {

functionNamespace := "openfaas-fn"
if namespace, exists := os.LookupEnv("function_namespace"); exists {
Expand Down Expand Up @@ -92,8 +93,8 @@ func New(client clientset.Interface,
HealthHandler: makeHealthHandler(),
InfoHandler: makeInfoHandler(),
SecretHandler: handlers.MakeSecretHandler(functionNamespace, kube),
ListNamespaceHandler: handlers.MakeNamespacesLister(functionNamespace, kube),
LogHandler: logs.NewLogHandlerFunc(faasnetesk8s.NewLogRequestor(kube, functionNamespace), bootstrapConfig.WriteTimeout),
ListNamespaceHandler: handlers.MakeNamespacesLister(functionNamespace, clusterRole, kube),
}

if pprof == "true" {
Expand Down