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

fix: health check failing for BGPPeer #2269

Merged
merged 1 commit into from Feb 7, 2024
Merged

fix: health check failing for BGPPeer #2269

merged 1 commit into from Feb 7, 2024

Conversation

budimanjojo
Copy link
Contributor

This fix this error:

health check failed after 29m30.045208087s: timeout waiting for: [BGPPeer/networking-system/main status: 'Unknown': conversion webhook for metallb.io/v1beta2, Kind=BGPPeer failed: Post "https://webhook-service.networking-system.svc/convert?timeout=30s": service "webhook-service" not found]

Thanks for sending a pull request! A few things before we get started:

  1. If this is your first time, please read the contributing guide
  2. For non-trivial pull requests, please file an
    issue
    first, and get
    agreement that the change is a good idea, and a general guideline
    for how it should be implemented, before sending code. Large PRs
    that weren't first discussed and agreed upon in an issue won't be
    accepted.
  3. If the PR fixes a particular bug, please include the words "Fixed
    #" in the PR text, so that the bug auto-closes when
    the PR is merged.

@budimanjojo
Copy link
Contributor Author

I don't know what's wrong. I used invoke generatemanifests in my machine to fix the error and it just complains about yq: error: argument files: can't open 'select(.kind == "CustomResourceDefinition")': [Errno 2] No such file or directory: 'select(.kind == "CustomResourceDefinition") and delete the whole crds.yaml file.

This is just a one liner change.

@fedepaol
Copy link
Member

fedepaol commented Feb 4, 2024

I think there are two yq versions, the one we are consuming is https://github.com/mikefarah/yq
We should probably change inv to make sure the right yq is downloaded locally and used.
In general, the problem of your PR is that it changes the helm CRD directly, while what we do is to generate them starting fro mwhat we have under /config.
The right fix would be to change the sed used in inv.

@budimanjojo
Copy link
Contributor Author

@fedepaol thanks for the yq-go hint. I have edited the file patch in ./config directory and ran inv command to generate the crds instead. Not sure if I should commit the generated one too or just commit the change in ./config directory so I just commit them all.

@fedepaol
Copy link
Member

fedepaol commented Feb 7, 2024

This is not good either, because the service in the kustomize configuration is named webhook-service (which exactly why this bug was introduced).
So, we either align the wh service name OR we extend the sed command when generating the charts. I think the former is cleaner and metallb-* is just a better name

The CRDs conversion webhook name refers to a non existing service, and
this is because we added automatic conversion from generated CRDs to
chart's CRD, so here:

- we align the name of the conversion webhook in the kustomized based
  env to the one in the charts (it's better anyway)
- we remove the "cert-service-name" parameter on the controller since
  now it uses the default

Signed-off-by: budimanjojo <budimanjojo@gmail.com>

Co-authored-by: Federico Paolinelli <fpaoline@redhat.com>
@fedepaol
Copy link
Member

fedepaol commented Feb 7, 2024

@budimanjojo I took the liberty to adjust your commit, to speed up the release.
Thanks again!

@budimanjojo
Copy link
Contributor Author

@fedepaol thank you!

@fedepaol
Copy link
Member

fedepaol commented Feb 7, 2024

Operator lane not affected by these changes, merging.

@fedepaol fedepaol merged commit b3d8a0f into metallb:main Feb 7, 2024
35 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants