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

Update external cloud provider flavor to use CRS and add test #1216

Merged
merged 2 commits into from
Mar 16, 2021
Merged

Update external cloud provider flavor to use CRS and add test #1216

merged 2 commits into from
Mar 16, 2021

Conversation

CecileRobertMichon
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon commented Mar 5, 2021 •
edited
Loading

What type of PR is this?
/kind feature

What this PR does / why we need it:
Updates the external cloud provider flavor:

  • removed the manual step of deploying the cloud controller manager and node controller manager
  • updated the version of the external cloud provider to v0.7.2
  • added the --cluster-name to ccm args (fixes an issue where external ccm creates new LBs named "kubernetes")
  • adds e2e test to the full test suite for external cloud provider (3 control planes for now to work around When running a workload with a single control plane node the load balancers take 15 mins to provision #857)
  • adds section about testing azure-cloud-provider in k8s developer docs

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 #

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:

Update external cloud provider flavor to use CRS and add test

@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. labels Mar 5, 2021
@k8s-ci-robot k8s-ci-robot requested review from cpanato and juan-lee March 5, 2021 22:35
@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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 5, 2021
@CecileRobertMichon
Copy link
Contributor Author

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

@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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 5, 2021
@CecileRobertMichon
Copy link
Contributor Author

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

1 similar comment
@CecileRobertMichon
Copy link
Contributor Author

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

@CecileRobertMichon CecileRobertMichon mentioned this pull request Mar 10, 2021
Merged
@CecileRobertMichon
Copy link
Contributor Author

kubernetes/test-infra#21298 should merge first

CecileRobertMichon
CecileRobertMichon commented Mar 10, 2021
View reviewed changes
test/e2e/azure_test.go
cluster = result.Cluster

// TODO: fix and enable
//Context("Creating an accessible load balancer", func() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for now disabled the test as something is causing it to fail. will investigate but didn't want to hold this PR since it's still an incremental improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kubernetes-sigs/cloud-provider-azure#551

@CecileRobertMichon CecileRobertMichon mentioned this pull request Mar 11, 2021
Merged
@CecileRobertMichon
Copy link
Contributor Author

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

@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 Mar 11, 2021
@devigned
Copy link
Contributor

/retest

@CecileRobertMichon
Copy link
Contributor Author

this is ready for review

devigned
devigned reviewed Mar 15, 2021
View reviewed changes
Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

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

Looks good. Just a question about usage.

docs/topics/kubernetes-developers.md Outdated Show resolved Hide resolved
@CecileRobertMichon
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-full
/hold for ^

@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 Mar 15, 2021
@CecileRobertMichon
Copy link
Contributor Author

CecileRobertMichon commented Mar 15, 2021 •
edited
Loading

ERROR: (AuthenticationFailed) Authentication failed.

😕

/retest

@CecileRobertMichon
Copy link
Contributor Author

/retest

auth failures should be resolved

@CecileRobertMichon
Copy link
Contributor Author

I think I messed up the regex in kubernetes/test-infra#21298 all tests are getting skipped now 😕

@CecileRobertMichon
Copy link
Contributor Author

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

@CecileRobertMichon
Copy link
Contributor Author

CecileRobertMichon commented Mar 16, 2021 •
edited
Loading

I think I messed up the regex in kubernetes/test-infra#21298 all tests are getting skipped now 😕

Actually, this was caused by the recent bump to ginkgo 1.15. I have reverted the update in the above commit and tests are now running as expected.

@CecileRobertMichon
Copy link
Contributor Author

/retest

@CecileRobertMichon
Copy link
Contributor Author

/hold cancel

@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 Mar 16, 2021
@CecileRobertMichon CecileRobertMichon mentioned this pull request Mar 16, 2021
Closed
@nader-ziada
Copy link
Contributor

/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 Mar 16, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nader-ziada

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 Mar 16, 2021
@k8s-ci-robot k8s-ci-robot merged commit 289f521 into kubernetes-sigs:master Mar 16, 2021
@k8s-ci-robot k8s-ci-robot added this to the v0.5.0 milestone Mar 16, 2021
@CecileRobertMichon CecileRobertMichon mentioned this pull request Mar 16, 2021
Merged
2 tasks
@CecileRobertMichon CecileRobertMichon mentioned this pull request Apr 5, 2021
Closed
@devigned devigned mentioned this pull request May 11, 2021
Closed
@CecileRobertMichon CecileRobertMichon deleted the update-external-cp branch February 17, 2023 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devigned devigned devigned left review comments

@cpanato cpanato Awaiting requested review from cpanato

@juan-lee juan-lee Awaiting requested review from juan-lee

Assignees

@nader-ziada nader-ziada

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v0.5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@CecileRobertMichon @devigned @nader-ziada @k8s-ci-robot