Implement BYO infra API changes by csrwng · Pull Request #65 · openshift/hypershift

csrwng · 2021-02-24T03:24:30Z

Implements BYO infra changes in API (as described in #38)
Updates 'create cluster' command to either take an infra description json (output of 'create infra' command) or invoke 'create infra' itself.
Removes use of management cluster infra in setting config for cluster/nodepools.

Still missing:

generate cloud config file for kcm
testing
destroy commands for infra and iam
update docs

openshift-ci-robot · 2021-02-24T03:24:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csrwng

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [csrwng]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sjenning · 2021-02-24T14:56:57Z

+		NodePoolReplicas:      2,
+		Render:                false,
+		Region:                "us-east-1",
+		WorkerInstanceProfile: "hypershift-worker-profile",


should probably set all field here, even if they have no default just to make it explicit that we do not default it. I see InfraID and InfrastructureJSON missing

sjenning · 2021-02-24T14:57:59Z

one nit, but lgtm

enxebre · 2021-02-24T15:03:26Z

+	AMI string `json:"ami,omitempty"`
+	// SecurityGroups is the set of security groups to associate with nodepool machines
+	// +optional
+	SecurityGroups []string `json:"securityGroups,omitempty"`


what does this expect? and ID?
I think we should consolidate with Subnet and let both either support AWSResourceReference or only ID? Happy with this being done in a follow up though

Yup, right now it's expecting an ID. Can definitely switch to AWSResourceReference

enxebre · 2021-02-24T15:06:25Z

+
+	// AvailabilityZone is the default availability zone for the cluster
+	// +optional
+	AvailabilityZone string `json:"availabilityZone,omitempty"`


have we considered Region and AvailabilityZone as part of NodePoolDefaults? what do they mean above that level?

@enxebre I don't think we will support multi-region clusters, so I think of Region as a cluster-level thing.
AvailabilityZone yes is more of a nodepool level thing. However, the cluster cloud config (in the control plane) forces me to specify a zone (even though all it does with the zone is derive a region from it). I can definitely move it to the nodepool default and get the default from there for control plane purposes.

enxebre · 2021-02-24T15:08:30Z

+	Platform PlatformSpec `json:"platform"`
+
+	// InfraID is used to identify the cluster in cloud platforms
+	InfraID string `json:"infraID,omitempty"`


Just for the record I'm not a fan of the UX exposing the infraID in the API. I think we could infer it from the VPC ID and let that be the only contract.
Discussed with @csrwng to go as in this PR for now.

Should definitely do a follow up. Right now specifying the infra ID allows me to avoid doing an AWS call from the hypershift operator to discover it. I can pass it directly to the control plane where it's needed for the cloud config.

csrwng · 2021-02-25T02:54:40Z

Addressed comments, connected more of the control plane pieces

ironcladlou · 2021-02-25T14:17:51Z

 	SSHKey           []byte
 	NodePoolReplicas int
+
+	// AWS-specific options


Nit: what about extracting the AWS-isms (including AWSCredentials) into its own struct?

ironcladlou · 2021-02-25T15:20:47Z

@csrwng I think you want to rebase to get #67 and run make api — looks like you got some stuff generated in the wrong place due to the bug fixed by that patch

ironcladlou · 2021-03-01T18:46:03Z

@csrwng I'm a little bummed out this is further expanding the template debt by introducing more branching logic and even a new template function. If we're going to keep using these templates and even adding more of them, can we at least get to a point where the templates are 100% static string expansions with all data transformation done outside the templates in the code that sets up the input bound to them?

sjenning · 2021-03-01T18:50:43Z

As of 024d958, this worked for me 👍 Just commenting on the functionality.

Having an issue with the guest cluster's ingresscontroller not being created, but I'm not certain that has anything to do with this PR atm.

csrwng · 2021-03-01T20:31:19Z

@csrwng I'm a little bummed out this is further expanding the template debt by introducing more branching logic and even a new template function. If we're going to keep using these templates and even adding more of them, can we at least get to a point where the templates are 100% static string expansions with all data transformation done outside the templates in the code that sets up the input bound to them?

@ironcladlou I agree, continuing to use templates is less than ideal, but also not creating functional clusters is even less ideal. My goal with this PR is to get us to the point where we have a fully functional cluster again. At that point, we can update the e2e to validate that things are fully functional (at the very least validating that all cluster operators are reporting Available). Having that in place, then we can start refactoring the code to remove templates. Before that, I don't feel that we have much to validate that we're not breaking things.

ironcladlou · 2021-03-01T21:12:59Z

@csrwng

@ironcladlou I agree, continuing to use templates is less than ideal, but also not creating functional clusters is even less ideal. My goal with this PR is to get us to the point where we have a fully functional cluster again. At that point, we can update the e2e to validate that things are fully functional (at the very least validating that all cluster operators are reporting Available). Having that in place, then we can start refactoring the code to remove templates. Before that, I don't feel that we have much to validate that we're not breaking things.

Fair enough, thanks for all this work. I don't want to block this PR on it.

csrwng · 2021-03-04T15:53:55Z

/test e2e-aws

openshift-ci · 2021-03-04T16:51:47Z

@csrwng: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws	`b6f3f6d`	link	`/test e2e-aws`

Full PR test history. Your PR dashboard.

Details

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.

csrwng · 2021-03-04T17:28:44Z

e2e is expected to fail

ironcladlou · 2021-03-04T20:01:47Z

/lgtm

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 24, 2021

openshift-ci-robot requested review from enxebre and sjenning February 24, 2021 03:24

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 24, 2021

sjenning reviewed Feb 24, 2021

View reviewed changes

enxebre reviewed Feb 24, 2021

View reviewed changes

csrwng force-pushed the byo_api_changes branch from 84ff173 to f48108d Compare February 25, 2021 02:53

ironcladlou reviewed Feb 25, 2021

View reviewed changes

csrwng force-pushed the byo_api_changes branch 4 times, most recently from 08ced39 to 024d958 Compare March 1, 2021 17:09

csrwng force-pushed the byo_api_changes branch from 024d958 to 9d381a7 Compare March 1, 2021 20:15

csrwng force-pushed the byo_api_changes branch from 9d381a7 to 78b66c3 Compare March 1, 2021 22:12

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 1, 2021

csrwng force-pushed the byo_api_changes branch from 78b66c3 to b82faa3 Compare March 1, 2021 22:16

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 1, 2021

csrwng changed the title ~~WIP: Implement BYO infra API changes~~ Implement BYO infra API changes Mar 4, 2021

openshift-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 Mar 4, 2021

Implement BYO infra API changes

162906e

csrwng added 2 commits March 4, 2021 10:53

Add destroy infra and destroy iam commands

c8e3617

Update documentation

b6f3f6d

csrwng force-pushed the byo_api_changes branch from f384fb6 to b6f3f6d Compare March 4, 2021 15:53

openshift-ci-robot assigned ironcladlou Mar 4, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2021

openshift-merge-robot merged commit 54b6538 into openshift:main Mar 4, 2021

csrwng deleted the byo_api_changes branch June 14, 2022 19:12

Uh oh!

Conversation

csrwng commented Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Feb 24, 2021

Uh oh!

sjenning Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

sjenning commented Feb 24, 2021

Uh oh!

enxebre Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

csrwng Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

enxebre Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csrwng Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

enxebre Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

csrwng Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

csrwng commented Feb 25, 2021

Uh oh!

ironcladlou Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

ironcladlou commented Feb 25, 2021

Uh oh!

ironcladlou commented Mar 1, 2021

Uh oh!

sjenning commented Mar 1, 2021

Uh oh!

csrwng commented Mar 1, 2021

Uh oh!

ironcladlou commented Mar 1, 2021

Uh oh!

csrwng commented Mar 4, 2021

Uh oh!

openshift-ci Bot commented Mar 4, 2021

Uh oh!

csrwng commented Mar 4, 2021

Uh oh!

ironcladlou commented Mar 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

csrwng commented Feb 24, 2021 •

edited

Loading

enxebre Feb 24, 2021 •

edited

Loading