Skip to content

Conversation

@jshr-w
Copy link
Contributor

@jshr-w jshr-w commented Oct 11, 2023

Reason for Change:

PR moves Hubble Connectivity tests to Cilium Nightly Pipeline by enabling Hubble Metrics.

Issue Fixed:

Requirements:

Notes:

matmerr and others added 2 commits October 11, 2023 09:51
* cilium configmap

* update hubble configs and add metrics test

* update pipeline yaml

* separate cilium+hubble config
@jshr-w jshr-w added cni Related to CNI. ci Infra or tooling. labels Oct 11, 2023
@jshr-w jshr-w changed the base branch from master to shjayaraman/hubble-ci October 11, 2023 21:11
@Azure Azure deleted a comment from azure-pipelines bot Oct 11, 2023
@jshr-w jshr-w changed the title ci: move hubble connectivity tests to nightly pipeline [Draft] ci: move hubble connectivity tests to nightly pipeline Oct 11, 2023
@jshr-w jshr-w marked this pull request as ready for review October 11, 2023 21:28
@jshr-w jshr-w requested a review from a team as a code owner October 11, 2023 21:28
@jshr-w jshr-w requested a review from bohuini October 11, 2023 21:28
@jshr-w jshr-w marked this pull request as draft October 11, 2023 21:29
@jshr-w jshr-w removed the request for review from bohuini October 11, 2023 21:29
@jshr-w jshr-w changed the base branch from shjayaraman/hubble-ci to master October 17, 2023 16:30
@jshr-w jshr-w changed the title [Draft] ci: move hubble connectivity tests to nightly pipeline ci: move hubble connectivity tests to nightly pipeline Oct 17, 2023
@jshr-w jshr-w self-assigned this Oct 17, 2023
@jshr-w jshr-w marked this pull request as ready for review October 17, 2023 16:36
@jshr-w jshr-w requested a review from matmerr October 17, 2023 16:37
matmerr
matmerr previously approved these changes Oct 17, 2023
Copy link
Member

@matmerr matmerr left a comment

Choose a reason for hiding this comment

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

good stuff, thanks @jshr-w!

@jshr-w jshr-w requested a review from jpayne3506 October 17, 2023 16:50
echo "deploy Cilium ConfigMap"
kubectl apply -f cilium/configmap.yaml
kubectl apply -f test/integration/manifests/cilium/cilium${FILE_PATH}-config.yaml
kubectl apply -f test/integration/manifests/cilium/hubble/hubble-peer-svc.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

This is overwriting the current cilium-config configMap which is being set on the line above. Did the tests for nightly version of cilium pass?

Are we only trying to use this configmap (and test hubble) for the nightly pipeline?

Copy link
Member

Choose a reason for hiding this comment

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

this actually a service targeting cilium pods, the configmap below is applied below after the Azilium stages have completed

if preferred, @jshr-w, can you move the service installation to the test stage below? I don't have a strong preference as this is a cilium component, albeit not used yet

Copy link
Contributor

@jpayne3506 jpayne3506 Oct 17, 2023

Choose a reason for hiding this comment

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

My bad, misread it as I was reviewing. Is there any reasoning for overwriting the configMap further down instead of here?

flag.StringVar(&kubeconfigPath, "kubeconfig", getDefaultKubeconfigPath(), "Path to the kubeconfig file")
flag.Parse()

config, err := getClientConfig(kubeconfigPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We have the kubernetes package that provide k8s api calls. https://github.com/Azure/azure-container-networking/tree/master/test/internal/kubernetes.

The function that could replace this would be

func MustGetRestConfig() *rest.Config {
config, err := clientcmd.BuildConfigFromFlags("", *Kubeconfig)
if err != nil {
panic(err)
}
return config
}

Copy link
Member

Choose a reason for hiding this comment

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

true, I forgot these were here :)

unzip \
vim \
wget
wget
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: whitespace. You can change this to be automatically taken care of in your IDE

Comment on lines 160 to 162
kubectl rollout restart ds cilium -n kube-system
echo "wait 3 minutes for pods to be ready after restart"
sleep 180s
Copy link
Contributor

Choose a reason for hiding this comment

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

What pods need to be ready?

Copy link
Member

Choose a reason for hiding this comment

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

the Cilium pods, which are being restarted on L160

in lieu of static sleep, it could be a kubectl wait, ex:
kubectl wait --for=condition=ready pod -l k8s-app=cilium --timeout=3m

imo this suite could benefit by using kubectl wait in several places instead of sleeps, including
kubectl wait --for=delete pod/ciliumidentity/etc, but that's out of scope for this pr :)

Copy link
Contributor

@jpayne3506 jpayne3506 Oct 17, 2023

Choose a reason for hiding this comment

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

Would kubectl rollout status ds -n kube-system cilium --timeout=3m be just as effective for this case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Infra or tooling. cni Related to CNI.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants