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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃弮 Migrate rest of e2e tests to use clusterctl framework #1798

Merged
merged 2 commits into from Jul 17, 2020
Merged

馃弮 Migrate rest of e2e tests to use clusterctl framework #1798

merged 2 commits into from Jul 17, 2020

Conversation

gab-satchi
Copy link
Member

@gab-satchi gab-satchi commented Jul 7, 2020 •
edited

What this PR does / why we need it:

  • Migrates remaining tests to use the clusterctl testing framework.
  • Removes parallel option for Ginkgo tests. Tests were competing for resources and hitting the 5 EIP resource limit
  • Consolidate some test cases so fewer clusters are created.

In an effort to speed up the total time, some of the tests are combined so a new cluster isn't required per test:

  • Cluster name validations and provisioning extra AWS resources creates one cluster with the following tests:
    • name with more than 22 characters
    • name with '.'
    • name that starts with 'sg-'
    • Provisioning LoadBalancer dynamically and deleting on cluster deletion
    • Provisioning volumes dynamically and retain on cluster deletion
  • multiple workload clusters in different namespaces with machine failures also tests:
    • Delete infra node directly from infra provider
    • MachineDeployment will replace a deleted Machine

/hold for kubernetes/test-infra#17590 to merge

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 7, 2020
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 7, 2020
@gab-satchi
Copy link
Member Author

/test pull-cluster-api-provider-aws-test

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2020
@detiber detiber added this to the v0.5.x milestone Jul 13, 2020
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2020
@gab-satchi gab-satchi changed the title 馃弮 [WIP] Migrate rest of e2e tests to use clusterctl framework 馃弮 Migrate rest of e2e tests to use clusterctl framework Jul 14, 2020
@ncdc ncdc modified the milestones: v0.5.x, v0.5.5 Jul 15, 2020
@gab-satchi
Copy link
Member Author

/hold cancel
/test pull-cluster-api-provider-aws-e2e-new

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2020
@gab-satchi
Copy link
Member Author

gab-satchi commented Jul 15, 2020 •
edited

It looks like the artifacts folder is already set as an environment variable:

time hack/tools/bin/ginkgo -trace -progress -v -tags=e2e -focus="functional tests"  ./test/e2e_new/... -- -config-path="/home/prow/go/src/sigs.k8s.io/cluster-api-provider-aws/test/e2e_new/data/e2e_conf.yaml" -artifacts-folder="/logs/artifacts"

but the e2e_conf.yaml specifically looks under <repo_root>/_artifacts/templates/cluster-template-conformance-ci-artifacts.yaml

@gab-satchi
Copy link
Member Author

/test pull-cluster-api-provider-aws-e2e-new

@randomvariable
Copy link
Member

should be able to rebase now

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2020
Update e2e tests to use clusterctl framework
aa5698d 
- Adds volumes and multi-workload tests
- Adds md recovery test.
- Adds infra node deletion and VPC limit tests
- Adds invalid subnet and az tests
- Adds more cluster name tests
- Adds multi az test and test with '.' in name
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2020
@gab-satchi
Copy link
Member Author

/test pull-cluster-api-provider-aws-e2e-new

@randomvariable
Copy link
Member

/lgtm
/hold
pending e2e pass

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 16, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 16, 2020
@randomvariable
Copy link
Member

Maybe need a little longer here:

/home/prow/go/src/sigs.k8s.io/cluster-api-provider-aws/test/e2e_new/aws_test.go:72
  Cluster name validations and provisioning extra AWS resources
  /home/prow/go/src/sigs.k8s.io/cluster-api-provider-aws/test/e2e_new/aws_test.go:85
    Should create a cluster, AWS load balancer, and volume [It]
    /home/prow/go/src/sigs.k8s.io/cluster-api-provider-aws/test/e2e_new/aws_test.go:102
    Timed out after 600.000s.
      
    Expected
        <bool>: false
    to be true
    /home/prow/go/src/sigs.k8s.io/cluster-api-provider-aws/test/e2e_new/aws_test.go:889
    Full Stack Trace
    sigs.k8s.io/cluster-api-provider-aws/test/e2e_new.waitForStatefulSetRunning(0x27d0c1c, 0x11, 0x27bea92, 0x7, 0x2, 0xc0001ab890, 0x27cc1f0, 0xf, 0x27c8de2, 0xd, ...)
    	/home/prow/go/src/sigs.k8s.io/cluster-api-provider-aws/test/e2e_new/aws_test.go:889 +0x205
    sigs.k8s.io/cluster-api-provider-aws/test/e2e_new.createStatefulSet(0x27d0c1c, 0x11, 0x27bea92, 0x7, 0x2, 0xc0001ab890, 0x27cc1f0, 0xf, 0x27c8de2, 0xd, ...)
    	/home/prow/go/src/sigs.k8s.io/cluster-api-provider-aws/test/e2e_new/aws_test.go:487 +0x379
    sigs.k8s.io/cluster-api-provider-aws/test/e2e_new.glob..func1.2.1()
    	/home/prow/go/src/sigs.k8s.io/cluster-api-provider-aws/test/e2e_new/aws_test.go:120 +0x406
    github.com/onsi/ginkgo/internal/leafnodes.(*runner).runSync(0xc000428f60, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,

@gab-satchi
Copy link
Member Author

Ya I just saw the flake locally as well. It looks like it fails to schedule the pod as it waits for the worker node to become ready

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 16, 2020
@gab-satchi
Copy link
Member Author

gab-satchi commented Jul 16, 2020 •
edited

There's also a point of failure with zone-affinity. If the control plane is created in one of the zones other than the first one, it tends to create the Volume in that zone as well. When a worker node is created in the first zone, it's unable to run the stateful set. Adding a label to force storage classes to be created in the first AZ. This assumes AWS zones are returned consistently. If it's not reliable, I can move the volume test into one of the other tests that restricts the AZ used.

/test pull-cluster-api-provider-aws-e2e-new

@randomvariable
Copy link
Member

DescribeAvailabilityZones does appear to be deterministic, a surprise when it comes to the AWS API.

@randomvariable
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 17, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: randomvariable

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2020
@randomvariable
Copy link
Member

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 17, 2020
@k8s-ci-robot k8s-ci-robot merged commit 8c0c3db into kubernetes-sigs:master Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@detiber detiber detiber left review comments

@randomvariable randomvariable randomvariable left review comments

@ashish-amarnath ashish-amarnath Awaiting requested review from ashish-amarnath

@justinsb justinsb Awaiting requested review from justinsb

Assignees

@randomvariable randomvariable

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v0.5.5
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@gab-satchi @randomvariable @k8s-ci-robot @detiber @ncdc