baremetal: Disable network interfaces and BMH validations when needed #3794
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/romfreiman |
|
/retitle baremetal: Disable network interfaces and BMH validations when needed |
|
/label platform/baremetal |
openshift-ci-robot
changed the title
| // currently to validations skipping is supported | ||
| // "interfaces" - skip validation of hosts networking interfaces via libvirt | ||
| // "hosts" - skips validation of hosts existannce it they values | ||
| SkipValidations string `json:"skipValidations,omitemtpy"` |
There was a problem hiding this comment.
Did you look into alternatives that avoid this new boolean?
I think the main issue here is that the validations happen on create ignition-configs - I wonder if we can either ensure they only happen on create cluster or alternatively use some special value to LibvirtURI that indicates libvirt related validations should be disabled?
/cc @stbenjam any thoughts on the best interface here?
There was a problem hiding this comment.
I agree -- adding anything to the platform is a last resort. Changing when the validations run may be the best solution.
If we need to run some validations before create ignition to make sure we don't make bad ignition configs, I'd prefer to opt for some indication we're doing an assisted install. Disabling validations is probably not the only conditional thing we'll need to do for it.
There was a problem hiding this comment.
This is another example of why I think "assisted-baremetal" is a different platform in the installer, even if the platform in the cluster is still just "baremetal".
There was a problem hiding this comment.
@dhellmann if IPI was optional, those checks would not be required. Also, I would expect the runtime validations (libvirt socket) to be done during create cluster and not generate igntions.
There was a problem hiding this comment.
@hardys i wanted to disrupt the usual flow as little as possible, so for user that do not add the flag to the install-config, nothing would change. I had 2 addtional options that i thought about
- change the build tag of pkg/types/baremetal/validation/libvirt.go from baremetal to libvirt, and compile openshift-installer with TAG="baremetal" instead of TAG="baremetal libvirt". This would remove libvirt validation of network interfaces from baremetal installer.
- make Hosts field in pkg/types/baremetal/platform.go optional, and then execute all the validations for Hosts only in case this field is not empty
There was a problem hiding this comment.
suppose that for now we will leave BMH validation as is ( it does not really hurt us, we will just fill in dummy values, and we will use LibvirtURI field in baremetal platform to signal if it is assisted-installer setup. the code will look like:
if p.LibvirtURI == "assisted-installer" {
skip libvirt interface validation
}
There was a problem hiding this comment.
Magic string values aren't ok here -- I think the consensus from Steve and I are that the IPI-specific validations should be moved to be later, so you can do the UPI workflow with the baremetal platform.
There was a problem hiding this comment.
If you are just looking to iterate quickly, then I'd suggest doing it in a branch/fork. That worked well for us early on in baremetal IPI days, because we could iterate really quickly without the scrutiny that comes with merging to the installer master branch.
(That approach has its downsides too, since eventually you'll need it to make it to master and needs to get reviewed. We spent quite a long time addressing the installer team's feedback on our large PR to merge our fork.)
There was a problem hiding this comment.
I like the suggestion of reworking the validations such that runtime validations happen at
create clustertime.
@stbenjam do you think thats something someone external to the team can develop? I'm concerned this is not a trivial effort and implies only long term solution?
We are going to push back against any new field added to the platform. It's not that we're trying to be difficult, but because we're concerned about the user experience of an already complicated setup. Every field we add is another setting that needs documenting and users will see it and need to consider it (even if they don't need to do anything with it explicitly).
do we see a path for an internal only flag? e.g. not supported / documented outside this usage path?
There was a problem hiding this comment.
Yes, we can help - @dhellmann is going to put together a proposal to make sure we understand exactly what you need.
|
Commit message needs updating to meet https://github.com/openshift/installer/blob/master/CONTRIBUTING.md. Your text in the PR description should go in the commit message. |
|
We are using this ENV: OPENSHIFT_INSTALL_INVOKER="assisted-installer" |
The installer team generally doesn't allow new uses of environment variables, I don't think that will work. We left a few suggestions in the thread above about how to solve this without a specific validations knob. |
|
@yevgeny-shnaidman I guess we can set LibvirtURI='PIVOT' and and skipt :) |
|
we want it in the official release asap. we have fork already
…On Wed, Jun 24, 2020, 18:04 Stephen Benjamin ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/types/baremetal/platform.go
<#3794 (comment)>:
> @@ -106,4 +106,10 @@ type Platform struct {
//
// +optional
ClusterOSImage string `json:"clusterOSImage,omitempty" validate:"omitempty,osimageuri,urlexist"`
+
+ // SkipValidations is list of validations to skip during loading/fetching of install-config.yaml for baremetal platform
+ // currently to validations skipping is supported
+ // "interfaces" - skip validation of hosts networking interfaces via libvirt
+ // "hosts" - skips validation of hosts existannce it they values
+ SkipValidations string `json:"skipValidations,omitemtpy"`
If you are just looking to iterate quickly, then I'd suggest doing it in a
branch/fork. That worked well for us early on in baremetal IPI days,
because we could iterate really quickly without the scrutiny that comes
with merging to the installer master branch.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#3794 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABLPP44CTOXCPSMIP356K5DRYIIZLANCNFSM4OGTQDMQ>
.
|
|
can u send alternative pr? if u have already an agreement?
thanks
…On Wed, Jun 24, 2020, 18:10 Rom Freiman ***@***.***> wrote:
we want it in the official release asap. we have fork already
On Wed, Jun 24, 2020, 18:04 Stephen Benjamin ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In pkg/types/baremetal/platform.go
> <#3794 (comment)>:
>
> > @@ -106,4 +106,10 @@ type Platform struct {
> //
> // +optional
> ClusterOSImage string `json:"clusterOSImage,omitempty" validate:"omitempty,osimageuri,urlexist"`
> +
> + // SkipValidations is list of validations to skip during loading/fetching of install-config.yaml for baremetal platform
> + // currently to validations skipping is supported
> + // "interfaces" - skip validation of hosts networking interfaces via libvirt
> + // "hosts" - skips validation of hosts existannce it they values
> + SkipValidations string `json:"skipValidations,omitemtpy"`
>
> If you are just looking to iterate quickly, then I'd suggest doing it in
> a branch/fork. That worked well for us early on in baremetal IPI days,
> because we could iterate really quickly without the scrutiny that comes
> with merging to the installer master branch.
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <#3794 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABLPP44CTOXCPSMIP356K5DRYIIZLANCNFSM4OGTQDMQ>
> .
>
|
I'll spend some time putting together an enhancement proposal describing a way we could make this change. |
|
openshift/enhancements#388 describes in more detail what I think @hardys and @stbenjam were proposing. |
|
@yevgeny-shnaidman: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
|
Should we close this in favor of #3851 ? Similarly I think openshift/enhancements#388 supersedes #3788 ? |
|
/close |
|
@dhellmann: Closed this PR. In response to this:
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. |
7 participants
Currently openshift-installer.baremetal compiles with TAG="baremetal libvirt".Therefore it tries to validate baremetal network interfaces using libvirt. In addition it requires the presence of BMH definitions in the install-config.yaml.
In Assisted-Installer product we use openshift-installer to produce ignitions while running a job on a openshift cluster, so we need to disable libvirt validation. Also, we don't use BMH during 1st day installation, and would like to not configure it with dummy values in install-config.yaml
Proposed change:
Add a field called "skipValidations" to the baremetal platform configuration in the install-config.yaml. If this field is NOT present, openshift-installer will execute as usual. In case this field is present, it will skip the validations defined by it.
Currently 2 validations are defined: