validation changes for assisted bare metal deployments #388

dhellmann · 2020-06-24T20:59:12Z

The assisted installer is currently bypassing the installer validation
checks for the baremetal platform by using artificial data or
workarounds. This enhancement describes how we can modify the
installer to avoid those workarounds while retaining all of the checks
needed for the installer-provisioned infrastructure workflow.

stbenjam · 2020-06-25T02:07:40Z

Thanks for writing this up. I think it captures all the discussions and alternative proposals.

By doing the minimal set of validations at the InstallConfig level you can produce manifests and ignitions without needing the bootstrap infrastructure available. There’s probably some benefits there by separating the concerns even without the assisted install case.

enhancements/installer/assisted-installer-bare-metal-validations.md

yevgeny-shnaidman · 2020-06-25T07:06:20Z

@dhellmann doesn't it look a little bit strange that for some platforms we enforce validations during InstallConfig loading, and for some only during Cluster generation?

stbenjam · 2020-06-25T09:49:43Z

Why do you think it’s strange? The provision and platform checks that already exist for other platforms are a better fit for the checks we are doing that are causing you problems.

romfreiman · 2020-06-25T10:02:45Z

I'm fine with this approach (as I stated multiple times already). My only concern is whether we're not breaking API here.
I mean if now someone expect to fail during ignition phase (maybe there are even tests that are verifying this?) but will fail later on. Would it affect big customers?
Unfortunately I dont have this knowledge, hence raising the concern

stbenjam · 2020-06-25T10:16:26Z

I don't think any IPI user will care, and it's probably more correct and consistent with other platforms with this approach. You'd be able to run create manifests and ignition-configs from your laptop, and then bring the result to where you intend to provision, for example.

No one may even notice the change for the IPI workflow, a good percentage of users just run create cluster directly, and moving these validations later will be imperceptible (milliseconds).

enhancements/installer/assisted-installer-bare-metal-validations.md

stbenjam · 2020-06-25T10:18:13Z

enhancements/installer/assisted-installer-bare-metal-validations.md

+
+The assisted installer does not need the `bootstrapProvisioningIP`,
+`bootstrapProvisioningIP`, `provisioningNetworkCIDR`,
+`provisioningDHCPRange`, or `provisioningNetworkInterface` fields. The


What do you think about clusterOSImage and bootstrapOSImage? We could validate that if specified (they're not in the assisted install), that it's a valid URL in the installConfig, and only validate it exists in ProvisionCheck.

I added a paragraph about splitting those checks.

enhancements/installer/assisted-installer-bare-metal-validations.md

stbenjam · 2020-06-25T14:48:04Z

Thanks, I'm happy with this.

@hardys is out for a couple days, @crawford could you have a look at the proposal?

/assign @crawford

kirankt · 2020-06-25T15:09:32Z

enhancements/installer/assisted-installer-bare-metal-validations.md

+which validations should apply in each case, so there is no need to
+make the validations configurable.
+
+### Add an environment variable


Pretty sure this is a non-starter. The installer team will reject this idea based on previous experience.

I agree, that's why it's down here in the alternatives list.

markmc · 2020-06-25T16:14:37Z

I think we're missing a description of how exactly openshift-install will be used in an assisted installation. It's touched on in this enhancement and #376, but I think anyone not close to the subject would come away quite confused.

Like, firstly an assisted install only ever uses "create ignition-configs" and not "create cluster". (I guess some would say this implies it's a UPI use case)

But rather than the typical platform: none for UPI, we want to use platform: baremetal to express some intent about how the cluster should be installed, and its final configuration.

My understanding of that intent is: (a) configuring the keepalived/haproxy API load balancer (which is IPI-ish), and (b) enable the baremetal-operator for day 2 machine API operations (usually only available for IPI). And not (c) "provision a libvirt VM with the bootstrap configuration, and a bunch of IPMI-controlled physical machines as control plane nodes using PXE" (definitely IPI).

For each baremetal.Platform field, can we make it clear which of those 3 capabilities the field relates to?

If we did, then this proposal becomes "a UPI-like use case for 2 of these 3 capabilities" and it would be more obvious which fields are required for that.

Related, AIUI the "configure the baremetal-operator" intent currently requires all of the fields in the metal3.io Provisioning? But we desire that to be a day 2 "set up PXE-based machine provisioning" thing? (That topic is touched on in openshift/machine-api-operator#560)

Describing this in terms of which validations run at which phase doesn't really resonate with me - it's more which platform: baremetal capabilities (of the three) are being requested, and which validations apply to those capabilities?

Maybe this enhancement could become more "allowing bare-metal UPI installs to use some baremetal IPI capabilities"?

markmc · 2020-06-25T16:19:44Z

My only concern is whether we're not breaking API here.

See my comment. If we talk about this in terms of relaxing the "you may only use these 3 capabilities together" to "you may use 2 of these capabilities, without the other" ... it becomes more obvious to me that this isn't an API-breaking change. (i.e. changing foo(bar, blaa, baz) to foo(bar, blaa, baz=None) isn't an API break)

stbenjam · 2020-06-25T16:39:57Z

I think we're missing a description of how exactly openshift-install will be used in an assisted installation. It's touched on in this enhancement and #376, but I think anyone not close to the subject would come away quite confused.

Like, firstly an assisted install only ever uses "create ignition-configs" and not "create cluster". (I guess some would say this implies it's a UPI use case)

Perhaps we should explicitly call this out in this enhancement. The implied point of this enhancement is to facilitate the use of the baremetal platform for a UPI-like workflow. That is, allow you to generate manifests and ignitions without us actually requiring the fields needed for the automated bootstrap and master provisioning parts.

Deferring checking those fields is totally consistent with how other platforms do things. Even if we're doing IPI, Provisioning and Platform checks are clearly the right place for those validations; we made a mistake by validating everything in the installconfig validations.

This is a bit of a contrived example but imagine a user wanting to do an IPI install, but she has a bunch of manifests and ignition customizations. It makes sense to let her do that on a development laptop, and then bring that output to the provisioning host to run create cluster. That works if we rearrange the validations to be run only where needed. It also allows the UPI-workflow assisted installer wants to do.

My understanding of that intent is: (a) configuring the keepalived/haproxy API load balancer (which is IPI-ish), and (b) enable the baremetal-operator for day 2 machine API operations (usually only available for IPI). And not (c) "provision a libvirt VM with the bootstrap configuration, and a bunch of IPMI-controlled physical machines as control plane nodes using PXE" (definitely IPI).

Why are a and b separate? The way I understand things, we're not supporting separation: if you specify baremetal platform you get (a) and (b). We've deferred the epic to make Metal3 entirely optional.

Related, AIUI the "configure the baremetal-operator" intent currently requires all of the fields in the metal3.io Provisioning? But we desire that to be a day 2 "set up PXE-based machine provisioning" thing? (That topic is touched on in openshift/machine-api-operator#560)

Well, that's changing. #386 covers the optional provisioning network. It's sort of orthogonal to the assisted install case, even if they'll use it. It's valid even in IPI.

Although one note: all provisioning decisions on day 1 are permanent. We don't intend to make it possible to turn on a provisioning network later. If you install without it (either in assisted or IPI), you get Metal3 but can only do virtual media based provisioning and all the power management things. Trying to change the Provisioning configuration on day 2 is a separate epic and set of stories we could visit in the 4.7 time frame.

stbenjam · 2020-06-25T16:55:17Z

@markmc If we instead went to a capabilities-based approach, what would the interface look like?

markmc · 2020-06-25T17:08:05Z

@markmc If we instead went to a capabilities-based approach, what would the interface look like?

Something like "the presence of field A implies capabilities 1 and 2, and field B implies capability 3" ?

(i.e. we want to make field B optional if you just want capability 1 and 2)

Maybe the key difference is the docs for this would describe how to use those capabilities individually rather than talking about which fields are validated by which phases

markmc · 2020-06-25T17:12:11Z

My understanding of that intent is: (a) configuring the keepalived/haproxy API load balancer (which is IPI-ish), and (b) enable the baremetal-operator for day 2 machine API operations (usually only available for IPI). And not (c) "provision a libvirt VM with the bootstrap configuration, and a bunch of IPMI-controlled physical machines as control plane nodes using PXE" (definitely IPI).

Why are a and b separate? The way I understand things, we're not supporting separation: if you specify baremetal platform you get (a) and (b). We've deferred the epic to make Metal3 entirely optional.

Right, but I could imagine a future enhancement making it possible to use (a) alone in UPI installs ... and then couldn't rely on the fields/phases association, so we'd have to switch then to thinking in terms of individual capabilities. Why not make that switch in approach now?

(I think I'm advocating for more of a change in documentation - this enhancement and user docs - rather than changing the mechanism)

dhellmann · 2020-06-25T17:40:08Z

My understanding of that intent is: (a) configuring the keepalived/haproxy API load balancer (which is IPI-ish), and (b) enable the baremetal-operator for day 2 machine API operations (usually only available for IPI). And not (c) "provision a libvirt VM with the bootstrap configuration, and a bunch of IPMI-controlled physical machines as control plane nodes using PXE" (definitely IPI).

Why are a and b separate? The way I understand things, we're not supporting separation: if you specify baremetal platform you get (a) and (b). We've deferred the epic to make Metal3 entirely optional.

Right, but I could imagine a future enhancement making it possible to use (a) alone in UPI installs ... and then couldn't rely on the fields/phases association, so we'd have to switch then to thinking in terms of individual capabilities. Why not make that switch in approach now?

(I think I'm advocating for more of a change in documentation - this enhancement and user docs - rather than changing the mechanism)

How would the user express which capabilities they want?

kirankt · 2020-06-26T01:54:58Z

My understanding of that intent is: (a) configuring the keepalived/haproxy API load balancer (which is IPI-ish), and (b) enable the baremetal-operator for day 2 machine API operations (usually only available for IPI). And not (c) "provision a libvirt VM with the bootstrap configuration, and a bunch of IPMI-controlled physical machines as control plane nodes using PXE" (definitely IPI).

Why are a and b separate? The way I understand things, we're not supporting separation: if you specify baremetal platform you get (a) and (b). We've deferred the epic to make Metal3 entirely optional.

Right, but I could imagine a future enhancement making it possible to use (a) alone in UPI installs ... and then couldn't rely on the fields/phases association, so we'd have to switch then to thinking in terms of individual capabilities. Why not make that switch in approach now?
(I think I'm advocating for more of a change in documentation - this enhancement and user docs - rather than changing the mechanism)

How would the user express which capabilities they want?

My guess is something similar to what @stbenjam proposed in #386. Sort of choice in a IPI platform-wide profile, which enables/disables a whole set of related-features based on a said profile. Not sure about what features need to be turned on/off but couple things come to mind right away: validations (this proposal), provisioning network (@stbenjam 's) proposal, etc.

Profiles could be:
Full deploy of IPI (default)
Day 2 deploy only (UPI, Assisted, ISO-based installs, etc)
...

Signed-off-by: Doug Hellmann <[email protected]>

dhellmann · 2020-06-26T15:38:33Z

@markmc I have incorporated the feedback you gave me earlier in the latest draft.

dhellmann · 2020-06-26T15:39:41Z

I haven't changed the title. Should I rename this to incorporate "user-provisioned" instead of focusing on the assisted installer?

stbenjam · 2020-06-26T15:53:03Z

My guess is something similar to what @stbenjam proposed in #386. Sort of choice in a IPI platform-wide profile, which enables/disables a whole set of related-features based on a said profile.

At this point, I'd prefer not to combine the provisioning network profiles with other settings -- we may need some explicit indication later that an install as assisted, but I think this proposal covers what we need now.

dhellmann · 2020-06-26T16:33:08Z

My guess is something similar to what @stbenjam proposed in #386. Sort of choice in a IPI platform-wide profile, which enables/disables a whole set of related-features based on a said profile.

At this point, I'd prefer not to combine the provisioning network profiles with other settings -- we may need some explicit indication later that an install as assisted, but I think this proposal covers what we need now.

Yeah, I misunderstood what Mark was suggesting. After chatting with him it was clear that he meant the framing for this proposal was focused on the solution rather than the problem, and that thinking about the problem more broadly would result in a more useful problem statement and better context for reviewers not already familiar with the assisted installer. I've tried to incorporate his feedback into the latest draft (including stealing directly from some sample text he provided).

markmc · 2020-06-26T17:08:11Z

My guess is something similar to what @stbenjam proposed in #386. Sort of choice in a IPI platform-wide profile, which enables/disables a whole set of related-features based on a said profile.

At this point, I'd prefer not to combine the provisioning network profiles with other settings -- we may need some explicit indication later that an install as assisted, but I think this proposal covers what we need now.

Yeah, I misunderstood what Mark was suggesting. After chatting with him it was clear that he meant the framing for this proposal was focused on the solution rather than the problem, and that thinking about the problem more broadly would result in a more useful problem statement and better context for reviewers not already familiar with the assisted installer. I've tried to incorporate his feedback into the latest draft (including stealing directly from some sample text he provided).

Thanks for your patience with my feedback. I'm happy with the proposal, and I hope the new framing of the problem helps capture where it's coming from at a higher level. 👍

stbenjam · 2020-06-26T17:09:41Z

Are we all happy with this proposal then? Feel free to remove the hold when ready.

/lgtm
/hold

kirankt · 2020-06-26T18:12:28Z

/approve

openshift-ci-robot · 2020-06-26T18:12:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhellmann, kirankt, stbenjam

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:

~~OWNERS~~ [dhellmann,stbenjam]

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

romfreiman · 2020-06-27T17:34:47Z

enhancements/installer/assisted-installer-bare-metal-validations.md

+The [connected-assisted-installer
+enhancement](./connected-assisted-installer.md) describes how an agent
+running on several bare-metal hosts receive a command from the
+assisted installer application to begin the installation of that


Personally, I dont like the term application. I would call it either assisted installer or assisted installer service

romfreiman · 2020-06-27T17:35:54Z

enhancements/installer/assisted-installer-bare-metal-validations.md

+This installer workflow mirrors the [bare-metal user-provisioned
+infrastructure
+workflow](https://github.com/openshift/installer/blob/master/docs/user/metal/install_upi.md)
+in that the the OpenShift installer is used to generate assets for the


romfreiman · 2020-06-27T17:40:39Z

enhancements/installer/assisted-installer-bare-metal-validations.md

+`pkg/types/baremetal/validation` package that can be invoked from
+[PlatformProvisionCheck.Generate()](https://github.com/openshift/installer/blob/master/pkg/asset/installconfig/platformprovisioncheck.go#L37).
+
+There are two validation rules for the `clusterOSImage` and


what are those used for? just for general knowledge

They are used to optionally specify a URL for the two RHCOS images needed for an IPI baremetal deployment - these are large and by default are downloaded every deploy from the internet, so this interface enables faster repeated testing and also deployment in disconnected environments.

romfreiman · 2020-06-27T17:43:13Z

Looks good. I think we also discussed BMH annotations, but not 100% it's relevant to this enhancement

n1r1 · 2020-06-28T10:51:49Z

enhancements/installer/assisted-installer-bare-metal-validations.md

+  installer-provisioned infrastructure installer.
+- Recapitulate the test plans for the assisted installer.
+- Recapitulate the [approach for making parts of the bare metal
+  infrastructure automation optional](./baremetal-provisioning-optional.md).


broken link? couldn't find that doc

Found it. I guess the link will work once the enhancement will be merged.

hardys · 2020-06-29T08:45:32Z

enhancements/installer/assisted-installer-bare-metal-validations.md

+
+**For non-optional features moving to GA, the graduation criteria must include
+end to end tests.**
+


Some of the sections about graduation etc don't really apply here, can we just remove them?

hardys · 2020-06-29T09:06:08Z

Thanks for this - it lgtm and aligns well with the ideas I'd previously discussed and mentioned on openshift/installer#3794

dhellmann · 2020-06-29T17:44:08Z

I think this is ready to merge. We can address any other issues as follow-up patches.

/hold cancel

fix some review comments missed before openshift#388 merged Signed-off-by: Doug Hellmann <[email protected]>

openshift-ci-robot requested review from derekwaynecarr and hardys June 24, 2020 20:59

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

dhellmann mentioned this pull request Jun 24, 2020

baremetal: Disable network interfaces and BMH validations when needed openshift/installer#3794

Closed

stbenjam reviewed Jun 25, 2020

View reviewed changes

enhancements/installer/assisted-installer-bare-metal-validations.md Outdated Show resolved Hide resolved

stbenjam reviewed Jun 25, 2020

View reviewed changes

dhellmann force-pushed the assisted-installer-validations branch from 01c51dd to 2f6703a Compare June 25, 2020 13:08

stbenjam reviewed Jun 25, 2020

View reviewed changes

enhancements/installer/assisted-installer-bare-metal-validations.md Outdated Show resolved Hide resolved

openshift-ci-robot assigned crawford Jun 25, 2020

kirankt reviewed Jun 25, 2020

View reviewed changes

dhellmann force-pushed the assisted-installer-validations branch from 2f6703a to e06603e Compare June 25, 2020 15:11

zaneb mentioned this pull request Jun 25, 2020

Make BMCDetails optional metal3-io/baremetal-operator#562

Closed

validation changes for assisted bare metal deployments

461a137

Signed-off-by: Doug Hellmann <[email protected]>

dhellmann force-pushed the assisted-installer-validations branch from e06603e to 461a137 Compare June 26, 2020 15:37

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 26, 2020

openshift-ci-robot assigned stbenjam Jun 26, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2020

romfreiman reviewed Jun 27, 2020

View reviewed changes

n1r1 reviewed Jun 28, 2020

View reviewed changes

hardys reviewed Jun 29, 2020

View reviewed changes

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 29, 2020

openshift-merge-robot merged commit c6daad5 into openshift:master Jun 29, 2020

dhellmann added a commit to dhellmann/openshift-enhancements that referenced this pull request Jun 29, 2020

cleanup assisted installer bare metal validations enhancement

4c4d803

fix some review comments missed before openshift#388 merged Signed-off-by: Doug Hellmann <[email protected]>

dhellmann mentioned this pull request Jun 29, 2020

cleanup assisted installer bare metal validations enhancement #393

Merged

stbenjam mentioned this pull request Jul 7, 2020

Baremetal: Re-arrange validations for provisioning network at cluster creation time openshift/installer#3851

Merged

dhellmann mentioned this pull request Jul 9, 2020

baremetal: relax network interface and BMH validations when generating ignition-configs openshift/installer#3788

Closed

dhellmann added a commit to dhellmann/openshift-enhancements that referenced this pull request Sep 1, 2020

cleanup assisted installer bare metal validations enhancement

a2d893f

fix some review comments missed before openshift#388 merged Signed-off-by: Doug Hellmann <[email protected]>


		**For non-optional features moving to GA, the graduation criteria must include
		end to end tests.**

validation changes for assisted bare metal deployments #388

validation changes for assisted bare metal deployments #388

Uh oh!

Conversation

dhellmann commented Jun 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stbenjam commented Jun 25, 2020

Uh oh!

Uh oh!

yevgeny-shnaidman commented Jun 25, 2020

Uh oh!

stbenjam commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romfreiman commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stbenjam commented Jun 25, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stbenjam commented Jun 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markmc commented Jun 25, 2020

Uh oh!

markmc commented Jun 25, 2020

Uh oh!

stbenjam commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stbenjam commented Jun 25, 2020

Uh oh!

markmc commented Jun 25, 2020

Uh oh!

markmc commented Jun 25, 2020

Uh oh!

dhellmann commented Jun 25, 2020

Uh oh!

kirankt commented Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhellmann commented Jun 26, 2020

Uh oh!

dhellmann commented Jun 26, 2020

Uh oh!

stbenjam commented Jun 26, 2020

Uh oh!

dhellmann commented Jun 26, 2020

Uh oh!

markmc commented Jun 26, 2020

Uh oh!

stbenjam commented Jun 26, 2020

Uh oh!

kirankt commented Jun 26, 2020

Uh oh!

openshift-ci-robot commented Jun 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romfreiman commented Jun 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhellmann commented Jun 24, 2020 •

edited

Loading

stbenjam commented Jun 25, 2020 •

edited

Loading

romfreiman commented Jun 25, 2020 •

edited

Loading

stbenjam commented Jun 25, 2020 •

edited

Loading

kirankt commented Jun 26, 2020 •

edited

Loading