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

🌱 Adding to the test framework the equivalent to kubectl create -f. #9731

Conversation

adilGhaffarDev
Copy link
Contributor

@adilGhaffarDev adilGhaffarDev commented Nov 16, 2023

What this PR does / why we need it:
Adding runtime controller create for testing.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

@k8s-ci-robot k8s-ci-robot added this to the v1.4 milestone Nov 16, 2023
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area PR is missing an area label labels Nov 16, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign cecilerobertmichon for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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 size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 16, 2023
@adilGhaffarDev
Copy link
Contributor Author

/test ?

@k8s-ci-robot
Copy link
Contributor

@adilGhaffarDev: The following commands are available to trigger required jobs:

  • /test pull-cluster-api-build-release-1-4
  • /test pull-cluster-api-e2e-full-release-1-4
  • /test pull-cluster-api-e2e-release-1-4
  • /test pull-cluster-api-e2e-workload-upgrade-1-26-1-27-release-1-4
  • /test pull-cluster-api-test-mink8s-release-1-4
  • /test pull-cluster-api-test-release-1-4
  • /test pull-cluster-api-verify-release-1-4

The following commands are available to trigger optional jobs:

  • /test pull-cluster-api-apidiff-release-1-4
  • /test pull-cluster-api-e2e-informing-ipv6-release-1-4
  • /test pull-cluster-api-e2e-informing-release-1-4

Use /test all to run the following jobs that were automatically triggered:

  • pull-cluster-api-apidiff-release-1-4
  • pull-cluster-api-build-release-1-4
  • pull-cluster-api-e2e-informing-ipv6-release-1-4
  • pull-cluster-api-e2e-informing-release-1-4
  • pull-cluster-api-e2e-release-1-4
  • pull-cluster-api-test-mink8s-release-1-4
  • pull-cluster-api-test-release-1-4
  • pull-cluster-api-verify-release-1-4

In response to this:

/test ?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@adilGhaffarDev
Copy link
Contributor Author

/test pull-cluster-api-e2e-full-release-1-4

2 similar comments
@adilGhaffarDev
Copy link
Contributor Author

/test pull-cluster-api-e2e-full-release-1-4

@adilGhaffarDev
Copy link
Contributor Author

/test pull-cluster-api-e2e-full-release-1-4

@furkatgofurov7
Copy link
Member

/pull-cluster-api-e2e-release-1-4

@adilGhaffarDev
Copy link
Contributor Author

/test pull-cluster-api-e2e-full-release-1-4

@adilGhaffarDev
Copy link
Contributor Author

/test pull-cluster-api-e2e-release-1-4

Comment on lines 266 to 267
for _, o := range objs {
if err := p.GetClient().Create(ctx, &o); err != nil {
Copy link
Member

@furkatgofurov7 furkatgofurov7 Nov 17, 2023

Choose a reason for hiding this comment

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

Suggested change
for _, o := range objs {
if err := p.GetClient().Create(ctx, &o); err != nil {
for o := range objs {
if err := p.GetClient().Create(ctx, &objs[o]); err != nil {

or reassign the iteration var

for _, o := range objs { 
   o := o
   if err := p.GetClient().Create(ctx, &o); err != nil {

Signed-off-by: muhammad adil ghaffar <[email protected]>
@adilGhaffarDev
Copy link
Contributor Author

/test pull-cluster-api-e2e-full-release-1-4

@adilGhaffarDev
Copy link
Contributor Author

4/4 pull-cluster-api-e2e-full-release-1-4 passed (last passed: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/9731/pull-cluster-api-e2e-full-release-1-4/1725501937413001216)
/test pull-cluster-api-e2e-full-release-1-4

@furkatgofurov7
Copy link
Member

@adilGhaffarDev can you please remove WIP, if this is ready for review? Thanks!
The last run passed (5/5), re-running it again

/test pull-cluster-api-e2e-full-release-1-4
/area e2e-testing

@k8s-ci-robot k8s-ci-robot added area/e2e-testing Issues or PRs related to e2e testing and removed do-not-merge/needs-area PR is missing an area label labels Nov 17, 2023
@adilGhaffarDev adilGhaffarDev changed the title wip: 🌱 Adding runtime controller create. 🌱 Adding runtime controller create. Nov 19, 2023
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 19, 2023
@adilGhaffarDev
Copy link
Contributor Author

5/6 passed, last failure was not at Applying the cluster template yaml to the cluster it failed at Running Post-upgrade steps against the management cluster, (https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/9731/pull-cluster-api-e2e-full-release-1-4/1725625498903515136). This failure is not related to this PR. I will keep on investigating it.
running once more :
/test pull-cluster-api-e2e-full-release-1-4

@adilGhaffarDev
Copy link
Contributor Author

5/7 passed last failure was also related to ownerGraph issue which is in Running Post-upgrade steps against the management cluster Looking into it.
/test pull-cluster-api-e2e-full-release-1-4

@@ -472,7 +472,7 @@ func createWorkloadClusterAndWait(ctx context.Context, input createWorkloadClust
Expect(workloadClusterTemplate).ToNot(BeNil(), "Failed to get the cluster template")

Eventually(func() error {
return input.Proxy.Apply(ctx, workloadClusterTemplate)
return input.Proxy.Create(ctx, workloadClusterTemplate)
}, 10*time.Second).Should(Succeed(), "Failed to apply the cluster template")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}, 10*time.Second).Should(Succeed(), "Failed to apply the cluster template")
}, 10*time.Second).Should(Succeed(), "Failed to create the cluster template")

@fabriziopandini
Copy link
Member

Q: what is the reason to use create instead of apply?

@fabriziopandini fabriziopandini changed the title 🌱 Adding runtime controller create. 🌱 Adding to the test framework the equivalent to kubectl create -f. Nov 20, 2023
@adilGhaffarDev
Copy link
Contributor Author

Q: what is the reason to use create instead of apply?

As @killianmuldoon mentioned here #9696, it will help debug the flake we see in upgrade tests more specifically the one we are getting at INFO: Applying the cluster template yaml to the cluster. Here is a further explanation of 3 flakes that we are getting in clusterctl upgrade: #9688 (comment)

Why did we add this to 1.4 instead of the main first?
Because on 16th Nov, Thursday v1.4 was failing continuously on clusterctl upgrade. I added this to run some tests and get some logs to investigate, but after running several tests on this PR I don't see any failure at INFO: Applying the cluster template yaml to the cluster. I will run a few more tests just to confirm.

@sbueringer
Copy link
Member

sbueringer commented Nov 20, 2023

This is the same PR as https://github.com/kubernetes-sigs/cluster-api/pull/9736/files just for release-1.4, right?

If yes, let's please hold this PR and continue with the other one. If we merged 9736 we should then cherry-pick back from there.

To be clear, it's totally fine to have this PR to get data, but we should merge against main first and then backport instead of merging against release-1.4 first.

@chrischdi
Copy link
Member

And please add [release-1.4] to the title of the PR :-)

@adilGhaffarDev
Copy link
Contributor Author

/hold

@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 Nov 27, 2023
@adilGhaffarDev
Copy link
Contributor Author

/close because we merged #9737 which serves the same purpose.

@sbueringer
Copy link
Member

/close

@k8s-ci-robot
Copy link
Contributor

@sbueringer: Closed this PR.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/e2e-testing Issues or PRs related to e2e testing cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants