WIP: Moved oVirt installconfig to externalprovider #4517

ghost · 2021-01-04T12:24:01Z

This is the first PR of many as detailed in openshift/enhancements/pull/566. This PR is WIP and intended as a basis for discussion and will be amended later.

openshift-ci-robot · 2021-01-04T12:24:30Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign sdodson after the PR has been reviewed.
You can assign the PR to them by writing /assign @sdodson in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

Gal-Zaidman

Code looks good

Gal-Zaidman · 2021-01-04T15:32:35Z

pkg/externalprovider/provider/installconfig.go

+
+// InstallConfigExternalProvider describes the methods required for the installconfig asset.
+type InstallConfigExternalProvider interface {
+	// AddToInstallConfigPlatform adds the current platform to the installconfig.


Maybe change to "adds the current provider to the installconfig Platform"

Gal-Zaidman · 2021-01-04T15:35:31Z

pkg/externalprovider/provider/installconfig.go

+)
+
+// InstallConfigExternalProvider describes the methods required for the installconfig asset.
+type InstallConfigExternalProvider interface {


The name is a bit confusing "InstallConfigExternalProvider", I understood that platforms are the "external providers", because of "ExternalProvider is a provider that is not maintained by the core installer team." [1] maybe you should use a different name? it seems like the InstallConfigExternalProvider is something that the installer team will want to maintain

[1]https://github.com/openshift/installer/pull/4517/files?file-filters%5B%5D=.go#diff-95c3eb6370ff093d7d65c61022511d7286d41e57f067925081077b6ded9fb55fR3

I'm up for ideas instead of "External Provider", I'd like to wait for some feedback from the installer team on this.

Gal-Zaidman · 2021-01-04T15:36:54Z

pkg/externalprovider/simpleapi.go

+package externalprovider
+
+var providerRegistry = NewRegistry()
+


can you just name the file "api" why simple?

Ideally, at some point this API should be removed, but that's up in the air.

Gal-Zaidman · 2021-01-04T15:50:19Z

pkg/externalprovider/simpleapi.go

+func ValidateInstallConfig(
+	ProviderName Name,
+	Config *types.InstallConfig,
+	File *asset.File,


Seems like most providers will not need File, AWS or Azure...
This is totally a design decision and I can go either way here, maybe a variadic function with the empty interface(a ...interface{}) can replace it?
I just hate to ignore them for all providers just because it is needed by some...

The API is designed to 1:1 replace the current switch-cases with minimal changes to the code. Adding parameters to existing APIs is harder than adding new API calls.

patrickdillon · 2021-01-04T16:20:56Z

pkg/externalprovider/provider/base.go

@@ -0,0 +1,10 @@
+package provider
+
+// ExternalProvider is a provider that is not maintained by the core installer team.


Perfect example of Conway's Law

I don't think the providers should be grouped based on installer team/non-installer team.

The API is designed to be consumable by all providers, but I'm pragmatic about the adoption.

patrickdillon · 2021-01-04T16:23:48Z

pkg/externalprovider/provider/installconfig.go

+		Config *types.InstallConfig,
+		File *asset.File,
+		AWS *aws.Metadata,
+		Azure *icazure.Metadata,


It would make more sense to just have this signature match the dependencies of the asset being genereated:
https://github.com/openshift/installer/blob/master/pkg/asset/installconfig/platformcredscheck.go#L37

oVirt should never have a function that takes *aws.Metadata as a parameter.

That is true, but the API is designed to be adopted by all providers, so I added all parameters that are used by any provider at this time. Adding new methods is easier than adding new parameters to existing methods.

IMHO my suggestion would make it easier and more ergonomic for this API to be adopted by all providers. Also, I meant this as a general suggestion. The one function I picked here is just an example, but this should be applied to all platform interface functions related to an asset. By mapping the parameters to the asset dependencies you ensure that every platform would have all needed resources. It would then be the responsibility of the provider to extract those resources from the dependencies.

Ideally, we would pass on the install config directly, but if we do that we have a circular dependency because the IC structure is in the same package as the code needing to call the new API.

Ah, I see how the circular dependency is a problem now. I think there should be a way around the circular dependency by having a common interface. The asset package should not really need to depend on the external providers, and I think that is a benefit of the approach. I think moving the definition of the interface to the asset package could resolve this, but it is a bit too much for me to map it all out now.

The main issue is this piece of code:

func init() { providerRegistry.Register(ovirt.NewOvirtProvider()) }

If we can call this from somewhere that is not in the installconfig flow (e.g. some main.go) then that would solve the whole problem as the registry and the API interface could be split into two packages resolving the circular dependency. However, I don't know what would be a good place to add this.

openshift-ci · 2021-01-14T16:28:50Z

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

Test name	Commit	Details	Rerun command
ci/prow/yaml-lint	`0091074`	link	`/test yaml-lint`
ci/prow/e2e-aws-fips	`0091074`	link	`/test e2e-aws-fips`
ci/prow/e2e-aws-upgrade	`0091074`	link	`/test e2e-aws-upgrade`
ci/prow/openstack-manifests	`0091074`	link	`/test openstack-manifests`

Full PR test history. Your PR dashboard.

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.

openshift-bot · 2021-04-15T00:59:53Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

mtnbikenc · 2021-04-22T15:32:04Z

/uncc

staebler · 2021-04-29T02:34:42Z

/lifecycle frozen

openshift-ci-robot · 2021-04-29T02:34:42Z

@staebler: The lifecycle/frozen label cannot be applied to Pull Requests.

In response to this:

/lifecycle frozen

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.

openshift-ci · 2021-04-29T02:34:48Z

@janoszen: PR needs rebase.

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.

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 Jan 4, 2021

openshift-ci-robot requested review from dougsland and mtnbikenc January 4, 2021 12:24

ghost mentioned this pull request Jan 4, 2021

Separating provider-specific code in the installer openshift/enhancements#566

Closed

Janos Pasztor added 7 commits January 4, 2021 14:39

Added provider registry and base API structure

11dc0bc

Added oVirt as external provider

21c2ec8

Implemented AddToInstallConfigPlatform API

8377ece

Implemented ValidateInstallConfig API

40df377

Implemented PlatformCredsCheck API

2253d86

Implemented PlatformPermsCheck API

507eaea

Implemented PlatformProvisionCheck API

0091074

Gal-Zaidman reviewed Jan 4, 2021

View reviewed changes

patrickdillon reviewed Jan 4, 2021

View reviewed changes

openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 15, 2021

openshift-ci-robot removed the request for review from mtnbikenc April 22, 2021 15:32

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

ghost closed this Apr 29, 2021

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Moved oVirt installconfig to externalprovider #4517

WIP: Moved oVirt installconfig to externalprovider #4517

ghost commented Jan 4, 2021 •

edited by ghost

Loading

openshift-ci-robot commented Jan 4, 2021

Gal-Zaidman left a comment

Gal-Zaidman Jan 4, 2021

Gal-Zaidman Jan 4, 2021

ghost Jan 4, 2021

Gal-Zaidman Jan 4, 2021

ghost Jan 4, 2021

Gal-Zaidman Jan 4, 2021

ghost Jan 4, 2021

patrickdillon Jan 4, 2021

ghost Jan 4, 2021

patrickdillon Jan 4, 2021

ghost Jan 4, 2021

patrickdillon Jan 4, 2021 •

edited

Loading

ghost Jan 5, 2021

patrickdillon Jan 5, 2021

ghost Jan 5, 2021

openshift-ci bot commented Jan 14, 2021

openshift-bot commented Apr 15, 2021

mtnbikenc commented Apr 22, 2021

staebler commented Apr 29, 2021

openshift-ci-robot commented Apr 29, 2021

openshift-ci bot commented Apr 29, 2021

		package externalprovider

		var providerRegistry = NewRegistry()

		@@ -0,0 +1,10 @@
		package provider

		// ExternalProvider is a provider that is not maintained by the core installer team.

WIP: Moved oVirt installconfig to externalprovider #4517

WIP: Moved oVirt installconfig to externalprovider #4517

Conversation

ghost commented Jan 4, 2021 • edited by ghost Loading

openshift-ci-robot commented Jan 4, 2021

Gal-Zaidman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickdillon Jan 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 14, 2021

openshift-bot commented Apr 15, 2021

mtnbikenc commented Apr 22, 2021

staebler commented Apr 29, 2021

openshift-ci-robot commented Apr 29, 2021

openshift-ci bot commented Apr 29, 2021

ghost commented Jan 4, 2021 •

edited by ghost

Loading

patrickdillon Jan 4, 2021 •

edited

Loading