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

Project update #9

Closed
wants to merge 1 commit into from
Closed

Project update #9

wants to merge 1 commit into from

Conversation

michalschott
Copy link

Tried to run this against kubernetes 1.12 but it complained about using custom transport.

Also included few tweaks.

Not sure if this project is still maintained?

@coveralls
Copy link

coveralls commented Aug 5, 2019

Coverage Status

Coverage increased (+1.4%) to 43.584% when pulling 6b77f4c on michalschott:upstream_pr into 18e7710 on mikkeloscar:master.

Copy link
Collaborator

@szuecs szuecs left a comment

Choose a reason for hiding this comment

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

This change will cause problems with DNS changes.
The custom transport is not a nice to have.

Use defaultTaintNodeNotReadyName if not-ready-taint-name parameter is not specified.

Build container from scratch.

aws sdk update
@michalschott
Copy link
Author

@szuecs I've reverted the code in main.go and just disabled custom transport usage.

@szuecs
Copy link
Collaborator

szuecs commented Aug 6, 2019

The custom transport is required to make it work.
Please show relevant logs, that show why you want to drop it. We might need to find another work around to get our transport into client-go.

There’s a client-go issue, that has to be solved before we can change this custom transport. Basically the fix is to rollout H2 within kubernetes client go and apiserver. Until it’s not done, we can not change this because of reliability.

@@ -1,7 +1,5 @@
FROM registry.opensource.zalan.do/stups/alpine:latest
MAINTAINER Team Teapot @ Zalando SE <[email protected]>
FROM scratch
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change?
It doesn’t help in your issue and it’s not possible to investigate from inside containers anymore.

Copy link
Author

Choose a reason for hiding this comment

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

I'll revert this once we agree on final solution, cherry-picked and squashed in rush

@michalschott
Copy link
Author

michalschott commented Aug 6, 2019

kube-node-ready-5ff4bddfbf-577j6 kube-node-ready time="2019-08-05T12:24:57Z" level=fatal msg="using a custom transport with TLS certificate options or the insecure flag is not allowed"

That's the reason I've dropped custom transport completely.

The custom transport is required to make it work.

Can you explain why? I'm open for better solution, altho this fix is currently working in my clusters w/o any problems so far.

@szuecs
Copy link
Collaborator

szuecs commented Aug 9, 2019

You will rarely see the problem, but dns is only looked up once in Go. If the ip changes you have to delete the pod manually, which at our scale can’t do. golang/go#23427 is the underlying Go issue and there’s also a kubernetes related client go issue kubernetes/client-go#374 (comment) which leads to kubernetes/kubernetes#65012
So you have to check if you can get rid of the fatal log without removing the whole custom transport as long H2 is not completely rolled out in k8s.

@michalschott michalschott deleted the upstream_pr branch July 29, 2020 08:22
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.

3 participants