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 for CI: adding chart museum ingress #5337

Merged
merged 10 commits into from Sep 14, 2022
Merged

Conversation

castelblanque
Copy link
Collaborator

Description of the change

CI E2E runs are constantly failing due to problems with the port-forward done in the chart-museum part.
Whenever a chart is needed to be pushed to chart-museum, there is a port-forward done to the service.

That solution is weak and error prone, this PR removes that and adds an ingress setup for Chart museum so that there is no port-forward used anymore.
Ingress is only used from the scripts to push charts. From the E2E tests, the same internal service locator is used (http://chartmuseum.${CHARTMUSEUM_NS}.svc.cluster.local:8080/).

Benefits

It should bring more reliability.

Possible drawbacks

N/A

Applicable issues

N/A

Rafa Castelblanque added 5 commits September 12, 2022 11:47
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
@netlify
Copy link

netlify bot commented Sep 12, 2022

Deploy Preview for kubeapps-dev canceled.

Name Link
🔨 Latest commit 037354c
🔍 Latest deploy log https://app.netlify.com/sites/kubeapps-dev/deploys/6320abc9e36ead0008c5bcc0

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
@@ -107,9 +109,6 @@ spec:
pathType: ImplementationSpecific
EOF

# Add Ingress IP to hosts file
sudo echo "${DEX_IP} ${CHARTMUSEUM_HOSTNAME}" >> /etc/hosts
Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm, so it was a kind of a race condition because of the hostname not being set timely? Thanks for addressing it!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not really. That line was a test I did, but I didn't remember that the whole script must be run with sudo, which is not a good option. Finally went with sending the Host header and going to the IP directly.
However, it still fails and I can't replicate it locally. Whenever I have time I'll continue debugging in CircleCI directly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Feel free to jump in if you see the cause of the current error.

Rafa Castelblanque added 3 commits September 13, 2022 17:35
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
@castelblanque castelblanque marked this pull request as ready for review September 13, 2022 16:05
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Copy link
Contributor

@antgamdia antgamdia left a comment

Choose a reason for hiding this comment

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

Great, thanks! Let's see if it brings more reliability to the CI runs 🤞

Comment on lines +89 to +92
nginx.ingress.kubernetes.io/connection-proxy-header: keep-alive
nginx.ingress.kubernetes.io/proxy-read-timeout: "600"
nginx.ingress.kubernetes.io/proxy-buffer-size: "8k"
nginx.ingress.kubernetes.io/proxy-buffers: "4.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure we really need these annotations here, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are using those annotations in the other Ingress definition we use, so I just copy-pasted. Happy to remove them.

path: /
pathType: ImplementationSpecific
EOF
sleep 20
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this manual sleep here? Maybe it's for giving some time to the ingress controller to become available? If so, why not just sth like: kubectl wait --namespace ingress-nginx --for=condition=ready pod --selector=app.kubernetes.io/component=controller --timeout=120s

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That sleep is the key that was missing. Before that, Ingress wasn't ready by the time it was used some instructions after. I thought about adding the kubectl wait, but that is for the nginx controller itself which is ready way before (when the cluster is installed).
It seems that it takes some time for the route to be processed by the Nginx controller?

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.

None yet

3 participants