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

[installer/openvsx] Explicitly add statusfield of statefulset #8511

Closed
wants to merge 1 commit into from

Conversation

ArthurSens
Copy link
Contributor

Description

While trying to spin up preview environments on Harvester, we were getting failures applying the gitpod config as shown here:
image

It is a strange error since the status field of a stateful set should not be set before applying it, but should be populated and managed by the Kubernetes API.

If we decide to apply a statefulset with the status field though, there are some required fields that need to be set.

I couldn't find a way to make the installer not render the openvsx-proxy statefulset without the status field, but managed to make CI pass if I add the required fields. I'm opening this PR to ask for feedback, I'd rather prevent the status field from rendering altogether

Related Issue(s)

Fixes #

How to test

Create a fresh new environment from this PR

Release Notes

installer: Add required fields to openvsx statefulset manifests

Signed-off-by: ArthurSens <arthursens2005@gmail.com>
@mrsimonemms
Copy link
Contributor

This PR seems to be a sensible solution to the problem. It appears that the Kubernetes StatefulSet struct doesn't have omitempty on the Replicas, which it does on others (see Deployment). This means that it adds replicas: 0 when generating the YAML.

We want this fix in the Installer as it's likely that it'll affect other users - as it's an edge case, investigation and diagnosis will be time-consuming. Therefore, this is not something that we should post-process and treat as a Gitpod fix.

The imported Helm charts are not a problem as they manually write the YAML as a Golang template - we're converting the struct into YAML and so are bound by the Golang rules.

Other options we looked at:

  1. create a custom struct with the correct omitempty status
  2. post-process the YAML inside the Installer

Both of these seemed to be an awful lot of effort for (hopefully) one small issue. In the event that we find other things like this, we should consider one of the above two options. @gitpod-io/engineering-self-hosted

@mrsimonemms mrsimonemms self-requested a review March 1, 2022 16:37
Copy link
Contributor

@mrsimonemms mrsimonemms left a comment

Choose a reason for hiding this comment

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

This is a sensible approach to a misconfigured struct

@ArthurSens should we submit a bug report/PR to the Kubernetes maintainers?

@meysholdt
Copy link
Member

meysholdt commented Mar 1, 2022

/werft run no-preview=true

👍 started the job as gitpod-build-as-status.3

@codecov
Copy link

codecov bot commented Mar 1, 2022

Codecov Report

Merging #8511 (53be504) into main (a21f568) will decrease coverage by 7.51%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             main   #8511      +/-   ##
=========================================
- Coverage   15.06%   7.55%   -7.52%     
=========================================
  Files          51      31      -20     
  Lines        4899    2171    -2728     
=========================================
- Hits          738     164     -574     
+ Misses       4089    2004    -2085     
+ Partials       72       3      -69     
Flag Coverage Δ
components-gitpod-cli-app 11.17% <ø> (ø)
components-local-app-app-darwin-amd64 ?
components-local-app-app-darwin-arm64 ?
components-local-app-app-linux-amd64 ?
components-local-app-app-linux-arm64 ?
components-local-app-app-windows-386 ?
components-local-app-app-windows-amd64 ?
components-local-app-app-windows-arm64 ?
components-ws-daemon-app ?
components-ws-daemon-lib ?
install-installer-raw-app 4.49% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
components/ws-daemon/pkg/internal/session/store.go
components/ws-daemon/pkg/cpulimit/cpulimit.go
components/ws-daemon/pkg/cpulimit/dispatch.go
...onents/ws-daemon/pkg/internal/session/workspace.go
components/ws-daemon/pkg/quota/size.go
components/ws-daemon/pkg/content/service.go
...mponents/ws-daemon/pkg/daemon/cgroup_customizer.go
components/ws-daemon/pkg/daemon/markunmount.go
components/local-app/pkg/auth/auth.go
components/ws-daemon/pkg/content/initializer.go
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a21f568...53be504. Read the comment docs.

@ArthurSens
Copy link
Contributor Author

ArthurSens commented Mar 1, 2022

This is a sensible approach to a misconfigured struct

@ArthurSens should we submit a bug report/PR to the Kubernetes maintainers?

Still feels awkward that this hasn't been discovered yet, kubernetes client-go is one of the most used Go libraries in the world... I believe there is something off in our set up 🤔

@meysholdt the build failed with a valid failure, I think we need to address that instead of forcing the green lights 😬
image

@iQQBot
Copy link
Contributor

iQQBot commented Mar 1, 2022

Why? status is omitempty
image

@ArthurSens
Copy link
Contributor Author

ArthurSens commented Mar 1, 2022

Yeah, but the installer is rendering it anyways 🤔

If possible, I'd very much rather not render status at all

@akosyakov akosyakov requested a review from iQQBot March 2, 2022 08:08
@mrsimonemms
Copy link
Contributor

Why? status is omitempty

@iQQBot It does have omitempty, but it's not a pointer. A non-pointer struct defaults to an empty object. And because status doesn't have omitempty, that gets put to the default int32 value, which is 0

@meysholdt
Copy link
Member

meysholdt commented Mar 2, 2022

/werft run with-vm=true

👍 started the job as gitpod-build-as-status.4

@ArthurSens
Copy link
Contributor Author

ArthurSens commented Mar 2, 2022

Alrighty, this PR fixes installations on previews on Harvester, but breaks on core-dev 😕.

The reason is an incompatibility on Kubernetes versions, where AvailableReplicas does not exist in core-dev while being a required field in our k3s setup. Kinda expected since AvailableReplicas is a fairly new field added to Kubernetes API.

Although we're not going forward with this PR, rendering StatefulSetStatus isn't a good thing and we probably want to get rid of this to avoid possible incompatibilities with self-hosted users on more recent K8s distributions.

@ArthurSens ArthurSens closed this Mar 2, 2022
@ArthurSens
Copy link
Contributor Author

Related issue #8529

@ArthurSens ArthurSens deleted the as/status branch March 2, 2022 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants