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

feat: adding stateless CNI pipeline test #2914

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

behzad-mir
Copy link
Contributor

Reason for Change:
Adding a pipline test for stateless CNI to check the cns State and the datapath

Notes:

@behzad-mir behzad-mir added the cni Related to CNI. label Aug 12, 2024
@behzad-mir behzad-mir requested a review from a team as a code owner August 12, 2024 17:27
@behzad-mir behzad-mir force-pushed the statelessCNI-pipeline branch 4 times, most recently from 8784e66 to a00c923 Compare August 13, 2024 23:50

- job: ${{ parameters.name }}_windows
displayName: Azure CNI Overlay Test Suite | Windows - (${{ parameters.name }})
dependsOn: ${{ parameters.name }}_linux
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need to modify the depends on after ewmoving linux stages

- job: failedE2ELogs_linux
displayName: "Linux Failure Logs"
dependsOn:
- ${{ parameters.name }}_linux
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removing this

scriptType: "bash"
addSpnToEnvironment: true
inlineScript: |
set -e
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add nodepool after the cluster creation

name: "kubeconfig"
displayName: "Set Kubeconfig"

- ${{ if eq(parameters.os, 'linux') }}:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the test belo need to be run on windows

@behzad-mir behzad-mir force-pushed the statelessCNI-pipeline branch 6 times, most recently from 26e0c9f to 7e98835 Compare August 14, 2024 22:25
Copy link
Contributor

@QxBytes QxBytes left a comment

Choose a reason for hiding this comment

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

Also would like to see a passing pipeline run to be sure

@behzad-mir behzad-mir force-pushed the statelessCNI-pipeline branch 2 times, most recently from bf25b66 to a8a0251 Compare August 22, 2024 22:17
@behzad-mir behzad-mir requested a review from a team as a code owner August 22, 2024 22:17
@behzad-mir behzad-mir force-pushed the statelessCNI-pipeline branch 2 times, most recently from 1359ef1 to 5ba93d5 Compare August 23, 2024 01:55
@behzad-mir behzad-mir force-pushed the statelessCNI-pipeline branch 3 times, most recently from 6a34418 to e47fd99 Compare August 23, 2024 23:10
Comment on lines +81 to +94
- script: |
echo "validate pod IP assignment before CNS restart"
kubectl get pod -owide -A
echo "validate pod state before CNS restarts"
cd test/integration/load
CNI_TYPE=stateless go test -timeout 30m -tags load -run ^TestValidateState$
kubectl rollout restart ds azure-cns -n kube-system
kubectl rollout status ds azure-cns -n kube-system
kubectl get pod -owide -A
echo "validate pods after CNS restart"
CNI_TYPE=stateless go test -timeout 30m -tags load -run ^TestValidateState$
name: "restartCNS_ValidatePodState"
displayName: "Restart CNS and validate pod state"
retryCountOnTaskFailure: 3
Copy link
Contributor

@jpayne3506 jpayne3506 Aug 26, 2024

Choose a reason for hiding this comment

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

This test is contained within make test-load. Should be removed.

clusterType: overlay-byocni-up
clusterName: "statelesswin"
vmSize: Standard_B2ms
k8sVersion: "1.30"
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: this line should/will be removed once #2924 is merged.

@@ -1125,18 +1126,23 @@ func (plugin *NetPlugin) Delete(args *cniSkel.CmdArgs) error {
// network ID is passed in and used only for migration
// otherwise, in stateless, we don't need the network id for deletion
epInfos, err = plugin.nm.GetEndpointState(networkID, args.ContainerID)
// if stateless CNI fail to get the endpoint from CNS for any reason other than Endpoint Not found
if err != nil && !strings.Contains(err.Error(), "endpoint state could not be found in the statefile") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way we can tie this back to the defined error message?

ErrEndpointStateNotFound = errors.New("endpoint state could not be found in the statefile")

for _, v := range cnsResult.Endpoints {
for ifName, ip := range v.IfnameToIPMap {
if ifName == "eth0" {
cnsPodIps[ip.IPv4[0].IP.String()] = v.PodName
Copy link
Contributor

Choose a reason for hiding this comment

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

just wanted to confirm that ip.IPv4 will always have at least 1 item in it?

name: $(BUILD_POOL_NAME_DEFAULT)
jobs:
- job: ${{ parameters.name }}_windows
displayName: Azure CNI Overlay Test Suite | Windows - (${{ parameters.name }})
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we mention in the display name that this is for stateless?

inputs:
kubectlVersion: latest

- task: AzureCLI@1
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we should only use AzureCLI@2 for anything new (also found below in this file)

Copy link
Contributor

@QxBytes QxBytes left a comment

Choose a reason for hiding this comment

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

Also, noticed there's a failure in the pipelines here but it doesn't fail the test, and here . The second failure I noticed occurs on other PRs (which we should look into at some point) but could we check the first is expected behavior?

Copy link

This pull request is stale because it has been open for 2 weeks with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the stale Stale due to inactivity. label Sep 11, 2024
@github-actions github-actions bot removed the stale Stale due to inactivity. label Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cni Related to CNI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants