Skip to content

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

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
wants to merge 1 commit into from
Closed

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

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
6 changes: 6 additions & 0 deletions pkg/types/baremetal/platform.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Copy link

@hardys hardys Jun 24, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Member

@stbenjam stbenjam Jun 24, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@dhellmann dhellmann Jun 24, 2020

Choose a reason for hiding this comment

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

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".

Copy link

@romfreiman romfreiman Jun 24, 2020

Choose a reason for hiding this comment

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

@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.

Copy link
Author

@yevgeny-shnaidman yevgeny-shnaidman Jun 24, 2020

Choose a reason for hiding this comment

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

@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

  1. 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.
  2. 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

Copy link
Author

@yevgeny-shnaidman yevgeny-shnaidman Jun 24, 2020

Choose a reason for hiding this comment

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

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
}

@stbenjam @hardys

Copy link
Member

@stbenjam stbenjam Jun 24, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member

@stbenjam stbenjam Jun 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.)

Copy link

@ohadlevy ohadlevy Jun 24, 2020

Choose a reason for hiding this comment

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

I like the suggestion of reworking the validations such that runtime validations happen at create cluster time.

@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?

Copy link
Member

@stbenjam stbenjam Jun 24, 2020

Choose a reason for hiding this comment

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

Yes, we can help - @dhellmann is going to put together a proposal to make sure we understand exactly what you need.

#3794 (comment)

}
4 changes: 4 additions & 0 deletions pkg/types/baremetal/validation/libvirt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ func init() {
func validateInterfaces(p *baremetal.Platform, fldPath *field.Path) field.ErrorList {
errorList := field.ErrorList{}

if strings.Contains(p.SkipValidations, "interfaces") {
return errorList
}

findInterface, err := interfaceValidator(p.LibvirtURI)
if err != nil {
errorList = append(errorList, field.InternalError(fldPath.Child("libvirtURI"), err))
Expand Down
11 changes: 7 additions & 4 deletions pkg/types/baremetal/validation/platform.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,11 @@ func validateOSImages(p *baremetal.Platform, fldPath *field.Path) field.ErrorLis
return platformErrs
}

func validateHostsCount(hosts []*baremetal.Host, installConfig *types.InstallConfig) error {

func validateHostsCount(p *baremetal.Platform, installConfig *types.InstallConfig) error {
if strings.Contains(p.SkipValidations, "hosts") {
return nil
}
hosts := p.Hosts
hostsNum := int64(len(hosts))
counter := int64(0)

Expand Down Expand Up @@ -270,7 +273,7 @@ func ValidatePlatform(p *baremetal.Platform, n *types.Networking, fldPath *field
allErrs = append(allErrs, field.Invalid(fldPath.Child("provisioningNetworkInterface"), p.ProvisioningNetworkInterface, "no provisioning network interface is configured, please set this value to be the interface on the provisioning network on your cluster's baremetal hosts"))
}

if p.Hosts == nil {
if p.Hosts == nil && !strings.Contains(p.SkipValidations, "hosts") {
allErrs = append(allErrs, field.Invalid(fldPath.Child("hosts"), p.Hosts, "bare metal hosts are missing"))
}

Expand Down Expand Up @@ -301,7 +304,7 @@ func ValidatePlatform(p *baremetal.Platform, n *types.Networking, fldPath *field
allErrs = append(allErrs, field.Invalid(fldPath.Child("bootstrapHostIP"), p.BootstrapProvisioningIP, err.Error()))
}

if err := validateHostsCount(p.Hosts, c); err != nil {
if err := validateHostsCount(p, c); err != nil {
allErrs = append(allErrs, field.Required(fldPath.Child("Hosts"), err.Error()))
}

Expand Down