Default to OVNKubernetes NetworkType for E2E, enable API access for production by drewandersonnz · Pull Request #1571 · Azure/ARO-RP

drewandersonnz · 2021-07-01T14:46:45Z

Which issue this PR addresses:

What this PR does / why we need it:

Adds SDNProvider to the API
Sets OVNKubernetes as the default option for E2E
Maintains OpenShiftSDN as the default option for production

Test plan for issue:

Unit tests exists for API behavior (make test-go)
E2E tests will transition to OVNKubernetes

Is there any documentation that needs to be updated for this PR?

Auto-generated Swagger API client docs are available in PR.

drewandersonnz · 2021-07-01T14:49:10Z

Looks to be blocked by jewzaam/installer-aro#2

mjudeikis

Still needs wiring into Installer and maybe E2E tests. Good start :)

pkg/api/openshiftcluster.go

pkg/api/v20210131preview/openshiftcluster.go

drewandersonnz · 2021-07-02T08:42:55Z

Still needs wiring into Installer and maybe E2E tests

@mjudeikis what do you mean?

installer is already updated, this PR builds clusters with OVNKubernetes by default, by passing OVNKubernetes as the default parameter to the RP API, and passing that through to installer after validation.
E2E tests will build an OVNKubernetes cluster by default, are you requesting we double the E2E tests to do one set for OpenShiftSDN and another E2E for OVNKubernetes ?

mjudeikis · 2021-07-02T09:02:57Z

installer is already updated, this PR builds clusters with OVNKubernetes by default, by passing OVNKubernetes as the default parameter to the RP API, and passing that through to installer after validation.

Missed that one. Sorry.

E2E tests will build an OVNKubernetes cluster by default, are you requesting we double the E2E tests to do one set for OpenShiftSDN and another E2E for OVNKubernetes ?

No, I think lets leave one for now. I was thinking if we need something in E2E test suite to verify it works fine with our DNS changes. @Makdaam might be able to answer how much in the need we are here

pkg/api/v20210131preview/openshiftcluster_convert.go

pkg/api/v20210131preview/openshiftcluster_validatestatic.go

pkg/cluster/generateconfig.go

github-actions · 2021-07-07T09:22:32Z

Please rebase pull request.

mjudeikis

As mentioned by @m1kola this is not yet done as per 3 usecases.
In example if you create this using old API you get this now:

ERRO[2021-07-13T08:43:47+01:00]pkg/util/steps/runner.go:34 steps.Run() step [Action github.com/Azure/ARO-RP/pkg/cluster.(*manager).Install.func1] encountered error: networki
ng.networkType: Required value: network provider type required  client_principal_name= client_request_id=01ea3a82-e3ae-11eb-bf99-b46921811e1f component=backend correlation_i
d= request_id=bff09dab-7500-4c53-957f-2a3735a1152d resource_group=v4-westeurope resource_id=/subscriptions/225e02bc-43d0-43d1-a01a-17e584a4ef69/resourcegroups/v4-westeurope/
providers/microsoft.redhatopenshift/openshiftclusters/mjudeikis2 resource_name=mjudeikis2 subscription_id=225e02bc-43d0-43d1-a01a-17e584a4ef69 

ERRO[2021-07-13T08:43:48+01:00]pkg/backend/openshiftcluster.go:221 backend.(*openShiftClusterBackend).updateAsyncOperation.func1() networking.networkType: Required value: ne
twork provider type required  client_principal_name= client_request_id=01ea3a82-e3ae-11eb-bf99-b46921811e1f component=backend correlation_id= request_id=bff09dab-7500-4c53-9
57f-2a3735a1152d resource_group=v4-westeurope resource_id=/subscriptions/225e02bc-43d0-43d1-a01a-17e584a4ef69/resourcegroups/v4-westeurope/providers/microsoft.redhatopenshif
t/openshiftclusters/mjudeikis2 resource_name=mjudeikis2 subscription_id=225e02bc-43d0-43d1-a01a-17e584a4ef69

pkg/api/v20210131preview/openshiftcluster_validatestatic.go

github-actions · 2021-07-15T11:35:12Z

Please rebase pull request.

pkg/cluster/generateconfig.go

drewandersonnz · 2021-07-15T12:33:57Z

Rebased.

m1kola

Looks good, but we need to tidy up the tests before we merge.

pkg/api/v20210131preview/openshiftcluster_validatestatic_test.go

pkg/cluster/generateconfig.go

m1kola

I just realised that we are not defining constatns for the internal representnation in pkg/api/openshiftcluster.go.

I think the PR looks very clean and I hope it is the last round of review from me, becuase everything else looks good.

pkg/api/v20210131preview/openshiftcluster_validatestatic_test.go

pkg/cluster/generateconfig.go

pkg/api/admin/openshiftcluster_validatestatic_test.go

pkg/api/openshiftcluster.go

pkg/api/v20210131preview/openshiftcluster_validatestatic_test.go

pkg/util/cluster/cluster.go

…tworkProfile

drewandersonnz · 2021-07-19T07:42:56Z

Rebased (again)

m1kola

Looks good to me. Let's wait for e2e

m1kola · 2021-07-19T12:14:29Z

E2E failed on vm redeployment. It is probably a known flake. Restarted.

drewandersonnz requested review from jewzaam, m1kola and mjudeikis as code owners July 1, 2021 14:46

mjudeikis suggested changes Jul 2, 2021

View reviewed changes

pkg/api/openshiftcluster.go Outdated Show resolved Hide resolved

pkg/api/v20210131preview/openshiftcluster.go Outdated Show resolved Hide resolved

m1kola suggested changes Jul 2, 2021

View reviewed changes

drewandersonnz force-pushed the 9823641 branch from 9c77fd3 to e2fe8a6 Compare July 2, 2021 10:15

github-actions bot added the needs-rebase branch needs a rebase label Jul 7, 2021

drewandersonnz force-pushed the 9823641 branch from 0f52c1a to 32d985a Compare July 7, 2021 12:44

github-actions bot removed the needs-rebase branch needs a rebase label Jul 7, 2021

drewandersonnz force-pushed the 9823641 branch from 32d985a to d572cd8 Compare July 7, 2021 13:09

mjudeikis suggested changes Jul 13, 2021

View reviewed changes

pkg/api/v20210131preview/openshiftcluster_validatestatic.go Outdated Show resolved Hide resolved

pkg/api/v20210131preview/openshiftcluster_validatestatic.go Outdated Show resolved Hide resolved

pkg/api/v20210131preview/openshiftcluster_validatestatic.go Outdated Show resolved Hide resolved

github-actions bot added the needs-rebase branch needs a rebase label Jul 15, 2021

m1kola reviewed Jul 15, 2021

View reviewed changes

pkg/cluster/generateconfig.go Outdated Show resolved Hide resolved

drewandersonnz force-pushed the 9823641 branch from b994e62 to f6ace59 Compare July 15, 2021 12:33

github-actions bot removed the needs-rebase branch needs a rebase label Jul 15, 2021

m1kola suggested changes Jul 15, 2021

View reviewed changes

drewandersonnz force-pushed the 9823641 branch from 6965eca to 6247c04 Compare July 16, 2021 12:09

m1kola suggested changes Jul 16, 2021

View reviewed changes

m1kola added the next-up label Jul 16, 2021

drewandersonnz added 5 commits July 19, 2021 09:41

create NetworkType to suport OVNKubernetes

bfa19cc

make client; make test-go

016aab3

Switch to OVNKubernetes on create

0d2b2a9

SDNProvider instead of NetworkType

82159bf

make client; make test-go;

2f61391

drewandersonnz added 6 commits July 19, 2021 09:41

use switch as per feedback

da19a8b

remove isCreate

118da75

Simplified error message

26cc634

enable default SDNProvider when not specified

d1f87df

Admin API for sdnProvider

7f91e94

de-duplicate and move tests into TestOpenShiftClusterStaticValidateNe…

f2b4595

…tworkProfile

drewandersonnz force-pushed the 9823641 branch from 6247c04 to f2b4595 Compare July 19, 2021 07:41

Changes related to feedback

7cf26fe

drewandersonnz force-pushed the 9823641 branch from 12837d3 to 7cf26fe Compare July 19, 2021 08:02

default to OpenShiftSDN provider in production

65a2a9d

drewandersonnz changed the title ~~Default to OVNKubernetes NetworkType~~ Default to OVNKubernetes NetworkType for E2E, enable API access for production Jul 19, 2021

m1kola approved these changes Jul 19, 2021

View reviewed changes

mjudeikis approved these changes Jul 20, 2021

View reviewed changes

mjudeikis merged commit add7bc3 into Azure:master Jul 20, 2021

drewandersonnz deleted the 9823641 branch October 1, 2021 08:14

Conversation

drewandersonnz commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue this PR addresses:

What this PR does / why we need it:

Test plan for issue:

Is there any documentation that needs to be updated for this PR?

Uh oh!

drewandersonnz commented Jul 1, 2021

Uh oh!

mjudeikis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

drewandersonnz commented Jul 2, 2021

Uh oh!

mjudeikis commented Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jul 7, 2021

Uh oh!

mjudeikis left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jul 15, 2021

Uh oh!

Uh oh!

drewandersonnz commented Jul 15, 2021

Uh oh!

m1kola left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

m1kola left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

drewandersonnz commented Jul 19, 2021

Uh oh!

m1kola left a comment

Choose a reason for hiding this comment

Uh oh!

m1kola commented Jul 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

drewandersonnz commented Jul 1, 2021 •

edited

Loading

mjudeikis commented Jul 2, 2021 •

edited

Loading

mjudeikis left a comment •

edited

Loading