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

Add AWSCluster #942

Merged
merged 8 commits into from
Aug 1, 2019
Merged

Add AWSCluster #942

merged 8 commits into from
Aug 1, 2019

Conversation

ncdc
Copy link
Contributor

@ncdc ncdc commented Jul 31, 2019 •
edited
Loading

What this PR does / why we need it:
Replaces the old cluster actuator with a new AWSCluster CRD and controller.

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

Special notes for your reviewer:

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

Release note:

NONE

@ncdc
Update cluster-api dep to master
a97febc 
Signed-off-by: Andy Goldstein <goldsteina@vmware.com>
@ncdc ncdc added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Jul 31, 2019
@ncdc ncdc added this to the v0.4 milestone Jul 31, 2019
@ncdc ncdc requested a review from detiber July 31, 2019 14:34
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 31, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncdc

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 size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 31, 2019
@ncdc ncdc force-pushed the awscluster-controller branch from 87d7eb7 to bc2d7e2 Compare July 31, 2019 14:38
@ncdc
Copy link
Contributor Author

ncdc commented Jul 31, 2019

I'm fixing the broken tests but I'd appreciate some 👀 on this now if anyone has time.
/cc @rudoi @sethp-nr

@k8s-ci-robot k8s-ci-robot requested review from rudoi and sethp-nr July 31, 2019 15:01
detiber
detiber reviewed Jul 31, 2019
View reviewed changes
pkg/cloud/scope/cluster.go Outdated Show resolved Hide resolved
pkg/cloud/scope/cluster.go Outdated Show resolved Hide resolved
pkg/cloud/scope/cluster.go Outdated Show resolved Hide resolved
pkg/controller/awscluster/awscluster_controller.go Outdated Show resolved Hide resolved
pkg/controller/awscluster/awscluster_controller.go Outdated Show resolved Hide resolved
pkg/controller/awscluster/awscluster_controller.go Outdated Show resolved Hide resolved
pkg/controller/awscluster/awscluster_controller.go Outdated Show resolved Hide resolved
pkg/controller/awscluster/awscluster_controller.go Outdated Show resolved Hide resolved
pkg/controller/awscluster/awscluster_controller.go
awsCluster.Status.APIEndpoints = []infrastructurev1alpha2.APIEndpoint{
{
Host: awsCluster.Status.Network.APIServerELB.DNSName,
// TODO(ncdc): should this come from awsCluster.Status.Network.APIServerELB.Listeners[0].Port?
Copy link
Member

Choose a reason for hiding this comment

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

If we can assume only a single listener, then I do not see why not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like our ELB creation code sets up a single listener. Is it possible for a user to BYO ELB?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like the code does get-or-create?

pkg/cloud/services/interfaces.go Outdated Show resolved Hide resolved
@ncdc
Add AWSCluster
f7ca984 
Signed-off-by: Andy Goldstein <goldsteina@vmware.com>
@ncdc ncdc force-pushed the awscluster-controller branch from bc2d7e2 to f7ca984 Compare July 31, 2019 20:31
@ncdc
correct cluster log names
8daeb8b 
Signed-off-by: Andy Goldstein <goldsteina@vmware.com>
@ncdc
Copy link
Contributor Author

ncdc commented Jul 31, 2019

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

@ncdc
code review + remove unused code
bd0c83c 
Signed-off-by: Andy Goldstein <goldsteina@vmware.com>
detiber
detiber reviewed Jul 31, 2019
View reviewed changes
pkg/controller/awscluster/awscluster_controller.go Show resolved Hide resolved
pkg/controller/awsmachine/awsmachine_controller.go Show resolved Hide resolved
pkg/controller/awsmachine/awsmachine_controller.go Show resolved Hide resolved
pkg/cloud/scope/machine.go Outdated Show resolved Hide resolved
pkg/cloud/scope/machine.go Outdated Show resolved Hide resolved
pkg/cloud/scope/machine.go Outdated Show resolved Hide resolved
pkg/cloud/scope/machine.go Outdated Show resolved Hide resolved
pkg/cloud/scope/machine.go Outdated Show resolved Hide resolved
ncdc added 2 commits July 31, 2019 17:04
@ncdc
remove commented out code
16ca9d1 
Signed-off-by: Andy Goldstein <goldsteina@vmware.com>
@ncdc
remove unused mocks. fix integration test. remove unused crd yaml
1ca9f13 
Signed-off-by: Andy Goldstein <goldsteina@vmware.com>
@ncdc ncdc force-pushed the awscluster-controller branch from adae994 to fe5c581 Compare August 1, 2019 01:25
@detiber
Copy link
Member

detiber commented Aug 1, 2019

@ncdc planning to update with the changes from kubernetes-sigs/cluster-api#1217?

@ncdc
Copy link
Contributor Author

ncdc commented Aug 1, 2019

@detiber next on my list!

ncdc added 2 commits August 1, 2019 10:06
@ncdc
try to fix jsonpatch in go.sum
cd889e1 
Signed-off-by: Andy Goldstein <goldsteina@vmware.com>
@ncdc
try to fix integration
242a3ea 
Signed-off-by: Andy Goldstein <goldsteina@vmware.com>
@ncdc ncdc force-pushed the awscluster-controller branch from fe5c581 to 242a3ea Compare August 1, 2019 14:06
@ncdc
Copy link
Contributor Author

ncdc commented Aug 1, 2019

Latest force-push has the same changes from CAPI 1217

@detiber
Copy link
Member

detiber commented Aug 1, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 1, 2019
@k8s-ci-robot k8s-ci-robot merged commit e3b2ce1 into kubernetes-sigs:master Aug 1, 2019
@ncdc ncdc mentioned this pull request Aug 1, 2019
Closed
6 tasks
@sbueringer sbueringer mentioned this pull request Aug 16, 2019
Closed
@ncdc ncdc deleted the awscluster-controller branch August 20, 2019 18:20
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

@davidewatson davidewatson Awaiting requested review from davidewatson

@justinsb justinsb Awaiting requested review from justinsb

@rudoi rudoi Awaiting requested review from rudoi

@sethp-nr sethp-nr Awaiting requested review from sethp-nr

Assignees

@detiber detiber

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. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v0.4.0 (v1alpha2)
Development

Successfully merging this pull request may close these issues.

[v1alpha2] Implement AWSCluster controller
3 participants
@ncdc @k8s-ci-robot @detiber