Skip to content

Add externalInfraCluster #2124

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
Merged
merged 1 commit into from
Dec 4, 2020
Merged

Add externalInfraCluster #2124

merged 1 commit into from
Dec 4, 2020

Conversation

enxebre
Copy link
Member

@enxebre enxebre commented Nov 26, 2020 •
edited
Loading

This decouples the machine controller from the Infrastructure resource.

What this PR does / why we need it:

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

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 26, 2020
@enxebre enxebre mentioned this pull request Nov 26, 2020
Closed
richardcase
richardcase reviewed Nov 30, 2020
View reviewed changes
Copy link
Member

@richardcase richardcase left a comment

Choose a reason for hiding this comment

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

A very interesting change and generally looks good. A couple of comments and the checks appear to be failing due to linting issues.

controllers/awsmachine_controller.go Outdated
if err != nil {
return ctrl.Result{}, err
}
if !machineScope.IsExternalInfraCluster() {
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we will need to do something similar for machine pools? If so, then we probably want to move the getInfraCluster to somewhere where it can be shared.

@enxebre enxebre force-pushed the decouple-external-infra branch from f02e985 to 8803072 Compare December 2, 2020 11:08
@enxebre
Copy link
Member Author

enxebre commented Dec 2, 2020

/retest

@randomvariable
Copy link
Member

randomvariable commented Dec 2, 2020 •
edited
Loading

You're hitting this dreaded thing, which I've hit before (about to open an issue about refactoring):

missing call(s) to *mock_services.MockEC2MachineInterface.GetInstanceSecurityGroups(is anything) /home/prow/go/src/sigs.k8s.io/cluster-api-provider-aws/controllers/awsmachine_controller_test.go

Fiddle with the caller counts, though it might be worth trying to figure out why there's an extra call or not.

Otherwise agree with rich here.

@enxebre enxebre force-pushed the decouple-external-infra branch from 8803072 to 90308af Compare December 2, 2020 15:42
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2020
@enxebre enxebre force-pushed the decouple-external-infra branch from 90308af to f8fbba4 Compare December 2, 2020 15:43
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2020
@enxebre enxebre force-pushed the decouple-external-infra branch from f8fbba4 to 585b7f5 Compare December 3, 2020 09:22
randomvariable
randomvariable suggested changes Dec 3, 2020
View reviewed changes
Copy link
Member

@randomvariable randomvariable left a comment

Choose a reason for hiding this comment

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

Some small logging suggestions to make it easier to debug because of controller runtime, otherwise lgtm.

@enxebre
Add externalInfraCluster
5437038 
This decouples the machine controller from the Infrastructure resource.
@enxebre enxebre force-pushed the decouple-external-infra branch from 585b7f5 to 5437038 Compare December 4, 2020 11:10
@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 Dec 4, 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 Dec 4, 2020
@k8s-ci-robot k8s-ci-robot merged commit 576f0cd into kubernetes-sigs:master Dec 4, 2020
@enxebre enxebre mentioned this pull request Dec 11, 2020
Merged
This was referenced Jan 20, 2021
Closed
Closed
enxebre added a commit to enxebre/hypershift that referenced this pull request Jun 1, 2021
@enxebre
Drop ExternalInfraCluster CRD in favour of AWSCluster
c5f4ec8 
This PR drops the ExternalInfraCluster CRD in favour of AWSCluster.

Originally we added support for externally managed infra in CAPA via the ExternalInfraCluster CRD kubernetes-sigs/cluster-api-provider-aws#2124 and we used that commit of CAPA in hypershift.

Later on we decided to revert that approach upstream and reuse the existing ${infra}Cluster CRDs with an annotation to support externally managed infrastructure kubernetes-sigs/cluster-api#4135

This PR bring latest CAPI/CAPA with one additional patch on top
kubernetes-sigs/cluster-api#4709
kubernetes-sigs/cluster-api-provider-aws#2453
to avoid running webhooks.

As a follow up we need to rebuild the images from the main branch once those patches are merged or otherwise enable webhooks.
enxebre added a commit to enxebre/hypershift that referenced this pull request Jun 1, 2021
@enxebre
Drop ExternalInfraCluster CRD in favour of AWSCluster
9e928c1 
This PR drops the ExternalInfraCluster CRD in favour of AWSCluster.

Originally we added support for externally managed infra in CAPA via the ExternalInfraCluster CRD kubernetes-sigs/cluster-api-provider-aws#2124 and we used that commit of CAPA in hypershift.

Later on we decided to revert that approach upstream and reuse the existing ${infra}Cluster CRDs with an annotation to support externally managed infrastructure kubernetes-sigs/cluster-api#4135

This PR bring latest CAPI/CAPA with one additional patch on top
kubernetes-sigs/cluster-api#4709
kubernetes-sigs/cluster-api-provider-aws#2453
to avoid running webhooks.

As a follow up we need to rebuild the images from the main branch once those patches are merged or otherwise enable webhooks.
@enxebre enxebre mentioned this pull request Jun 1, 2021
Merged
enxebre added a commit to enxebre/hypershift that referenced this pull request Jun 2, 2021
@enxebre
Drop ExternalInfraCluster CRD in favour of AWSCluster
c579830 
This PR drops the ExternalInfraCluster CRD in favour of AWSCluster.

Originally we added support for externally managed infra in CAPA via the ExternalInfraCluster CRD kubernetes-sigs/cluster-api-provider-aws#2124 and we used that commit of CAPA in hypershift.

Later on we decided to revert that approach upstream and reuse the existing ${infra}Cluster CRDs with an annotation to support externally managed infrastructure kubernetes-sigs/cluster-api#4135

This PR bring latest CAPI/CAPA.

As a follow up we need to rebuild the images and consume them from quay.io/hypershift.
enxebre added a commit to enxebre/hypershift that referenced this pull request Jun 2, 2021
@enxebre
Drop ExternalInfraCluster CRD in favour of AWSCluster
8f6e5fa 
This PR drops the ExternalInfraCluster CRD in favour of AWSCluster.

Originally we added support for externally managed infra in CAPA via the ExternalInfraCluster CRD kubernetes-sigs/cluster-api-provider-aws#2124 and we used that commit of CAPA in hypershift.

Later on we decided to revert that approach upstream and reuse the existing ${infra}Cluster CRDs with an annotation to support externally managed infrastructure kubernetes-sigs/cluster-api#4135

This PR bring latest CAPI/CAPA.

As a follow up we need to rebuild the images and consume them from quay.io/hypershift.
enxebre added a commit to enxebre/hypershift that referenced this pull request Jun 2, 2021
@enxebre
Drop ExternalInfraCluster CRD in favour of AWSCluster
0ef36e1 
This PR drops the ExternalInfraCluster CRD in favour of AWSCluster.

Originally we added support for externally managed infra in CAPA via the ExternalInfraCluster CRD kubernetes-sigs/cluster-api-provider-aws#2124 and we used that commit of CAPA in hypershift.

Later on we decided to revert that approach upstream and reuse the existing ${infra}Cluster CRDs with an annotation to support externally managed infrastructure kubernetes-sigs/cluster-api#4135

This PR bring latest CAPI/CAPA.

As a follow up we need to rebuild the images and consume them from quay.io/hypershift.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardcase richardcase richardcase left review comments

@randomvariable randomvariable randomvariable approved these changes

@ncdc ncdc Awaiting requested review from ncdc

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Decouple Infrastructure resource from machine controller
4 participants
@enxebre @randomvariable @k8s-ci-robot @richardcase