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

Set CF_INSTANCE_INDEX env variable with downward API #3483

Conversation

pbusko
Copy link
Contributor

@pbusko pbusko commented Sep 18, 2024

Is there a related GitHub Issue?

no

What is this change about?

Removes redundant webhook for setting CF_INSTANCE_INDEX environment variable

https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#pod-index-label
https://kubernetes.io/docs/reference/labels-annotations-taints/#apps-kubernetes.io-pod-index

Does this PR introduce a breaking change?

no

Acceptance Steps

Everything should work as before

Tag your pair, your PM, and/or team

@pbusko pbusko force-pushed the set-index-with-downward-api branch 2 times, most recently from 036ad05 to 67a76aa Compare September 18, 2024 11:43
@danail-branekov
Copy link
Member

danail-branekov commented Sep 18, 2024

E2E tests fail when pushing apps (logs are unfortunately not public). It seems that getting process stats fails:

{"severity":"ERROR","timestamp":"2024-09-18T12:12:47.171799854Z","logger":"handlers.process.get-stats","caller":"errors/errors.go:34","message":"Failed to get process stats from Kubernetes","correlation-id":"8c43cbde-5f9e-4e65-b45b-eb30c48b2334","ProcessGUID":"cf-proc-6e3e54e4-0c05-408f-9917-3c2391ba8ecc-web","error":"CF_INSTANCE_INDEX is not a valid index: strconv.Atoi: parsing \"\": invalid syntax","stacktrace":"code.cloudfoundry.org/korifi/api/errors.LogAndReturn\n\t/workspace/api/errors/errors.go:34\ncode.cloudfoundry.org/korifi/api/handlers.(*Process).getStats\n\t/workspace/api/handlers/process.go:192\ncode.cloudfoundry.org/korifi/api/routing.Handler.ServeHTTP\n\t/workspace/api/routing/handler.go:45\ncode.cloudfoundry.org/korifi/api/middleware.(*cfUser).middleware-fm.(*cfUser).middleware.func1\n\t/workspace/api/middleware/cf_user.go:68\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2220\ncode.cloudfoundry.org/korifi/api/middleware.(*authentication).middleware-fm.(*authentication).middleware.func1\n\t/workspace/api/middleware/authentication.go:60\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2220\ngithub.com/go-chi/chi.(*ChainHandler).ServeHTTP\n\t/go/pkg/mod/github.com/go-chi/chi@v4.1.2+incompatible/chain.go:31\ngithub.com/go-chi/chi.(*Mux).routeHTTP\n\t/go/pkg/mod/github.com/go-chi/chi@v4.1.2+incompatible/mux.go:431\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2220\ngithub.com/go-chi/chi/middleware.StripSlashes.func1\n\t/go/pkg/mod/github.com/go-chi/chi@v4.1.2+incompatible/middleware/strip.go:25\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2220\ncode.cloudfoundry.org/korifi/api/middleware.HTTPLogging.func1\n\t/workspace/api/middleware/http_logging.go:39\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2220\ncode.cloudfoundry.org/korifi/api/middleware.CFCliVersion.func1\n\t/workspace/api/middleware/cli_version.go:49\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2220\nmain.main.Correlation.func3.1\n\t/workspace/api/middleware/correlation.go:25\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2220\ngithub.com/go-chi/chi.(*Mux).ServeHTTP\n\t/go/pkg/mod/github.com/go-chi/chi@v4.1.2+incompatible/mux.go:86\nnet/http.serverHandler.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:3210\nnet/http.(*conn).serve\n\t/usr/local/go/src/net/http/server.go:2092"}

I have not looked into the code yet but it seems that the value is set to an empty string.

According to https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#pod-index-label, we need to enable the PodIndexLabel feature gate, maybe it is not on by default on GKE and EKS?

@danail-branekov
Copy link
Member

danail-branekov commented Sep 18, 2024

Just checked on our GKE cluster, PodIndexLabel is enabled:

❯ kubectl get --raw /metrics | grep kubernetes_feature_enabled | grep PodIndexLabel
kubernetes_feature_enabled{name="PodIndexLabel",stage="BETA"} 1

Apparently this function needs to be reimplemented to handle the downward API way of setting the index

@danail-branekov
Copy link
Member

danail-branekov commented Sep 18, 2024

docker build on controllers fails:

cloudfoundry/korifi-controllers | ------
cloudfoundry/korifi-controllers |  > [builder 12/12] RUN --mount=type=cache,target=/root/.cache/go-build     --mount=type=cache,target=/go/pkg/mod     CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -ldflags "-X code.cloudfoundry.org/korifi/version.Version=v0.12.1-165-gdde7c8be" -o manager controllers/main.go:
cloudfoundry/korifi-controllers | #16 0.327 statefulset-runner/controllers/appworkload_to_stset.go:13:2: no required module provides package code.cloudfoundry.org/korifi/api/actions; to add it:
cloudfoundry/korifi-controllers | #16 0.327 	go get code.cloudfoundry.org/korifi/api/actions
cloudfoundry/korifi-controllers | ------
cloudfoundry/korifi-controllers | Error: failed to solve: process "/bin/sh -c CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -ldflags \"-X code.cloudfoundry.org/korifi/version.Version=${version}\" -o manager controllers/main.go" did not complete successfully: exit code: 1
cloudfoundry/korifi-controllers | error: exit status 1
cloudfoundry/korifi-controllers | finished build (using kubectl buildkit)

The reason is that controllers now reference the actions.LabelPodIndex constant. Maybe referencing api from controllers is not ideal, I would suggest moving the constant under the korifiv1alpha package (maybe in https://github.com/cloudfoundry/korifi/blob/main/controllers/api/v1alpha1/shared_types.go, even though I hate those shared files) and reference it from both api and controllers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Reviewer approved
Development

Successfully merging this pull request may close these issues.

2 participants