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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't useExperimentalRetryJoin #1029

Merged
merged 1 commit into from
Apr 9, 2021
Merged

Don't useExperimentalRetryJoin #1029

merged 1 commit into from
Apr 9, 2021

Conversation

nader-ziada
Copy link
Contributor

@nader-ziada nader-ziada commented Nov 10, 2020 •
edited
Loading

/kind feature

What type of PR is this?

What this PR does / why we need it:

  • trying out the e2e tests without using useExperimentalRetryJoin

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 #998

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

Remove the setting for using useExperimentalRetryJoin

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 10, 2020
@k8s-ci-robot k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Nov 10, 2020
@nader-ziada
Copy link
Contributor Author

/hold
for running tests a few times

@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 10, 2020
@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-full

@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-capi-e2e

@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-conformance-with-ci-artifacts
/test pull-cluster-api-provider-azure-conformance-v1alpha3

@CecileRobertMichon
Copy link
Contributor

The tests have been pretty flaky with the retry lately so I wouldn't be surprised if we see flakes without it too...

@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e

looks like it was a flake since pull-cluster-api-provider-azure-e2e-full passed

@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e
/test pull-cluster-api-provider-azure-e2e-full
/test pull-cluster-api-provider-azure-capi-e2e

@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-full
/test pull-cluster-api-provider-azure-capi-e2e

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2020
@CecileRobertMichon
Copy link
Contributor

Let's hold until #991 merges so we can validate with the KCP flake improvements

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2020
@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-full
/test pull-cluster-api-provider-azure-capi-e2e

@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-full

@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-full
/test pull-cluster-api-provider-azure-capi-e2e

1 similar comment
@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-full
/test pull-cluster-api-provider-azure-capi-e2e

@CecileRobertMichon
Copy link
Contributor

/test pull-cluster-api-provider-azure-capi-e2e

1 similar comment
@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-capi-e2e

@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-full

@nader-ziada nader-ziada force-pushed the dont-use-retry-join branch from 7a024de to 5a8aa7e Compare April 1, 2021 21:43
@nader-ziada
Copy link
Contributor Author

Pass 8 after rebase
/test pull-cluster-api-provider-azure-e2e-full
/test pull-cluster-api-provider-azure-capi-e2e

@nader-ziada
Copy link
Contributor Author

/retest

@nader-ziada
Don't useExperimentalRetryJoin
66b7f3a 
now that we are using k9s 1.19.3 as default, we should not need this
@nader-ziada nader-ziada force-pushed the dont-use-retry-join branch from 5a8aa7e to 66b7f3a Compare April 2, 2021 03:29
@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-coverage

@nader-ziada
Copy link
Contributor Author

Pass 9
/test pull-cluster-api-provider-azure-e2e-full
/test pull-cluster-api-provider-azure-capi-e2e

@nader-ziada
Copy link
Contributor Author

capi is actually passing, its the GPU test that fails which should not be related

@nader-ziada
Copy link
Contributor Author

Pass 10
/test pull-cluster-api-provider-azure-e2e-full
/test pull-cluster-api-provider-azure-capi-e2e

@nader-ziada
Copy link
Contributor Author

Pass 10 - everything passed

Pass 11
/test pull-cluster-api-provider-azure-e2e-full
/test pull-cluster-api-provider-azure-capi-e2e

@nader-ziada
Copy link
Contributor Author

Pass 12
/test pull-cluster-api-provider-azure-e2e-full
/test pull-cluster-api-provider-azure-capi-e2e

@nader-ziada
Copy link
Contributor Author

the last few runs, only the GPU tests failed sometimes which is not related to this

@nader-ziada
Copy link
Contributor Author

/hold cancel

@shysank @CecileRobertMichon I think this is ready to merge now, wdyt?

@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 Apr 7, 2021
@CecileRobertMichon
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 8, 2021
@shysank
Copy link
Contributor

shysank commented Apr 8, 2021

One small clarification: useExperimentalRetryJoin is not needed for k8s > 1.19. I understand the part where it is removed from test templates. For other templates, is it expected for the user to know about including this setting when using k8s < 1.19?

@shysank
Copy link
Contributor

shysank commented Apr 8, 2021

lgtm with one clarification

@CecileRobertMichon
Copy link
Contributor

One small clarification: useExperimentalRetryJoin is not needed for k8s > 1.19. I understand the part where it is removed from test templates. For other templates, is it expected for the user to know about including this setting when using k8s < 1.19?

useExperimentalRetryJoin is not needed for k8s >= 1.19

Given that 1.18 & 1.17 would be out of the support skew for v1alpha4 (and < 1.17 won't even work - kubernetes-sigs/cluster-api#4423 (comment)), it should be safe to remove it from the example templates. It would be good to add a note in the CAPI bootstrap kubeadm docs about turning on the retries for k8s clusters < 1.19.

kubernetes-sigs/cluster-api#4444 is tracking defining the policy for k8s version support.

@shysank
Copy link
Contributor

shysank commented Apr 8, 2021

/lgtm

@CecileRobertMichon
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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 Apr 8, 2021
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Apr 8, 2021 •
edited
Loading

@nader-ziada: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-cluster-api-provider-azure-e2e-full 66b7f3a link /test pull-cluster-api-provider-azure-e2e-full

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e

@k8s-ci-robot k8s-ci-robot merged commit 37fe934 into kubernetes-sigs:master Apr 9, 2021
@k8s-ci-robot k8s-ci-robot added this to the v0.5.0 milestone Apr 9, 2021
@jackfrancis jackfrancis mentioned this pull request Apr 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awesomenix awesomenix Awaiting requested review from awesomenix

@CecileRobertMichon CecileRobertMichon Awaiting requested review from CecileRobertMichon

Assignees

@CecileRobertMichon CecileRobertMichon

@shysank shysank

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/azure Issues or PRs related to azure provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v0.5.0
Development

Successfully merging this pull request may close these issues.

Test if experimentalRetryJoin still needed for K8s 1.19+
4 participants
@nader-ziada @CecileRobertMichon @shysank @k8s-ci-robot