Skip to content

*: add InstallType to InstallConfig #4209

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

Closed
hexfusion wants to merge 1 commit into from
Closed

*: add InstallType to InstallConfig #4209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions data/data/install.openshift.io_installconfigs.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,6 +672,10 @@ spec:
- source
type: object
type: array
installType:
description: InstallType provides signal to the cluster that the installation
will not follow the default installer workflow.
type: string
kind:
description: 'Kind is a string value representing the REST resource this
object represents. Servers may infer this from the endpoint the client
Expand Down
17 changes: 17 additions & 0 deletions docs/user/customization.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ The following `install-config.yaml` properties are available:
* `noProxy` (optional string): A comma-separated list of domains and [CIDRs][cidr-notation] for which the proxy should not be used.
* `pullSecret` (required string): The secret to use when pulling images.
* `sshKey` (optional string): The public Secure Shell (SSH) key to provide access to instances.
* `installType` (optional string): The name of the non standard installation type used to bootstrap the cluster.

### IP networks

Expand Down Expand Up @@ -154,6 +155,22 @@ pullSecret: '{"auths": ...}'
sshKey: ssh-ed25519 AAAA...
```

### Custom installation type

An example install config with custom install type:

```yaml
apiVersion: v1
baseDomain: example.com
controlPlane:
name: master
replicas: 2
metadata:
name: test-cluster
platform: ...
installType: assisted
```

### Image content sources

An example install config with custom image content sources:
Expand Down
3 changes: 3 additions & 0 deletions pkg/explain/printer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ func Test_PrintFields(t *testing.T) {
ImageContentSources lists sources/repositories for the release-image content.
ImageContentSource defines a list of sources/repositories that can be used to pull content.

installType <string>
InstallType provides signal to the cluster that the installation will not follow the default installer workflow.

kind <string>
Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds

Expand Down
5 changes: 5 additions & 0 deletions pkg/types/installconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,11 @@ type InstallConfig struct {
// +optional
FIPS bool `json:"fips,omitempty"`

// InstallType provides signal to the cluster that the installation will not follow the default installer workflow.
//
// +optional
InstallType string `json:"installType,omitempty"`
Copy link
Contributor

@ironcladlou ironcladlou Sep 23, 2020

Choose a reason for hiding this comment

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

I feel like there's a more descriptive name to be found, and it should be an enum at least... seems like it will inevitably become mirror of PlatformType to some degree but with different lifecycle implications.

Is a raw enum going to be enough? (It wasn't for platform type.) What's the relationship between this type and and platform types? Which combinations are supported, and what are the clear semantics and cluster implications for each type and combination of type/platform? etc.

Would a vendor annotation on the installconfig make any sense as an alternative if we aren't able to more carefully specify these things right now?

Copy link
Contributor

@ironcladlou ironcladlou Sep 23, 2020

Choose a reason for hiding this comment

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

(see: openshift/enhancements#480 (comment))

Copy link
Contributor Author

@hexfusion hexfusion Sep 23, 2020

Choose a reason for hiding this comment

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

Is a raw enum going to be enough? (It wasn't for platform type.) What's the relationship between this type and and platform types? Which combinations are supported, and what are the clear semantics and cluster implications for each type and combination of type/platform? etc.

I agree with you in general but the idea is that the logic and relationships that each operator would conclude should be based on the value. We could certainly make this more strict and only define explicitly supported types. Then extended the list as necessary.

Would a vendor annotation on the installconfig make any sense as an alternative if we aren't able to more carefully specify these things right now?

If no agreement can be made I am open to any reasonable signal.

Copy link
Contributor Author

@hexfusion hexfusion Sep 23, 2020

Choose a reason for hiding this comment

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

(see: openshift/enhancements#480 (comment))

I agree that tying the install type to a platform doesn't make sense. The idea is to completely platform agnostic as the install type is not related to the platform it is performed on the platform. That was my intention in making the key at root level.

Copy link
Contributor Author

@hexfusion hexfusion Sep 23, 2020

Choose a reason for hiding this comment

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

I feel like there's a more descriptive name to be found, and it should be an enum at least

If you have a better name I am open to change.

Copy link
Contributor

@ironcladlou ironcladlou Sep 23, 2020

Choose a reason for hiding this comment

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

I'm most interested in clarity of semantics. Possible values, what they mean on their own, what they mean with respect to platform types.

For example, based on what I understand so far, here's a try:

// AssistedInstaller indicates the platform will assume an installation
// topology consisting of exactly 3 nodes which will be the only available
// nodes to complete cluster bootstrapping.
//
// The platform may relax certain high availability constraints to achieve
// bootstrapping under the constrained topology.
//
// This install type is only supported when the platform type is BareMetal.
type AssistedInstaller InstallType = "AssistedInstaller"

Copy link
Contributor

@ironcladlou ironcladlou Sep 23, 2020

Choose a reason for hiding this comment

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

Maybe it needs to be even more specific in terms of the machine pools. Isn't the requirement for there to be a single master pool with 3 replicas and an empty worker pool? This should be unambiguous.

Copy link

@romfreiman romfreiman Sep 23, 2020

Choose a reason for hiding this comment

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

why? we let the user choose how many workers he adds - among all the discovered hardware, we auto-select 3 masters and all the rest become workers

Copy link
Contributor

@ironcladlou ironcladlou Sep 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Good point. Is the absence of the dedicated bootstrap node the substantive difference? Again, we should figure out how to define it

Copy link
Contributor Author

@hexfusion hexfusion Sep 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Worker pools are not assumed but unlike compact clusters workers are possible. @romfreiman keep me honest here.


// CredentialsMode is used to explicitly set the mode with which CredentialRequests are satisfied.
//
// If this field is set, then the installer will not attempt to query the cloud permissions before attempting
Expand Down