baremetal: validate hosts number

[andfasano]

This PR adds a new Baremetal platform validation rule to verify that the configured hosts are equal or greater than the sum of the configured ControlPlane and Compute replicas.

In addition, it contains refactoring steps to:

• have a single generic function for defining and applying tags based validation
• support future splitting of rules definition and their application by the use of a validation context

[stbenjam]

I'm a little bit hesitant to say we should use annotations for all validations.

This isn't reusable for anything else, unlike other validations. TBH, I think it makes sense for this just to be a normal validation with a very simple check in the body of the ValidatePlatform function. I would find that code easier to read.

What do you think?

[stbenjam]

Also: what does 'dive' mean?

Nevermind, I see: https://godoc.org/gopkg.in/go-playground/validator.v9#hdr-Dive

[andfasano]

Well, I also had some doubts about the reusability of this rule and agreed that very likely it will not be reusable elsewhere. I followed such approach for a matter of maintenance/readability (it is sufficient to look at the struct tags to understand all the validations applied to a given field) and because the boilerplate code required to add a new validation rule was essentially the same in this case (types.InstallConfig must always be passed as a contextual info to apply the cross-struct validation). I can revert it back to a normal validation to see how it looks like.

[abhinavdahiya]

i think this is mis-use of the context.

[andfasano]

config and uniqueValues are required contextual information to apply correctly the rules. customErrs is a necessary workaround required to build as much as possible a generic messaging layer that will adapt to the current one and will allow to reuse the pre-existing rules (and it could be removed once the issue go-playground/validator#506 will be fixed). The usage of context in this case will allow more decoupling between the definition and calling of validation rules, that in theory could be pretty far each other (instead of closures)

[andfasano]

/test pj-rehearse

[stbenjam]

pj-rehearse is only a job on openshift/release

You can do this to retest:

/retest

[andfasano]

/retest

[andfasano]

/retest

[andfasano]

/test e2e-metal-ipi

[stbenjam]

Thanks this looks great.

Would it be difficult to separate out the hosts count validation from the refactor into a separate PR? I want to get the hosts count validation merged now, but then look closer at the refactor with the context stuff.

I think it doesn't make sense now to combine the refactor if the count validation is by itself.

[andfasano]

Thanks this looks great.

Would it be difficult to separate out the hosts count validation from the refactor into a separate PR? I want to get the hosts count validation merged now, but then look closer at the refactor with the context stuff.

I think it doesn't make sense now to combine the refactor if the count validation is by itself.

Sure, I'll rework the current PR just to keep the validation as it is without the refactoring, an move it eventually to another PR; note that until we'll want to share the validations across the code the refactoring is not yet required, let me know what you think about it.

[andfasano]

Reworked PR so that it contains just the plain validation rule without any refactoring

[stbenjam]

With really nested values, the excessive calls to build are distracting. Is it avoidable? Maybe by using a builder interface, you could have Compute's build call it on the passed arguments, for example?

[andfasano]

Sounds a good idea, the syntax looks less heavy, pushed it (version with no interface) (could be also extended directly to the test struct).

[stbenjam]

/lgtm
/approve

[stbenjam]

Please have a look @abhinavdahiya, as this modifies files in pkg/types/validation, I can't approve it.

[stbenjam]

/assign @abhinavdahiya

[stbenjam]

/test e2e-metal-ipi

[abhinavdahiya]

https://github.com/openshift/installer/pull/3392/files#diff-02200a3eac2ac223b4117e690cf2d0c7
https://github.com/openshift/installer/pull/3392/files#diff-9f95494109d2a540a4a321c151e20c63

/approve

personally the commit message in the PR is missing any details on what new validation is being added, also sending in the entire install-config to a function validatePlatform seems wrong to me.

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, stbenjam

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [abhinavdahiya]

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

[stbenjam]

Thanks for updating the commit message. I think this looks ok, will wait to see results of e2e-metal-ipi first, though.

[stbenjam]

/lgtm

[openshift-ci-robot]

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

Test name	Commit	Details	Rerun command
ci/prow/e2e-libvirt	c6f4c29	link	/test e2e-libvirt
ci/prow/e2e-aws-fips	c6f4c29	link	/test e2e-aws-fips
ci/prow/e2e-openstack	c6f4c29	link	/test e2e-openstack
ci/prow/e2e-aws-scaleup-rhel7	c6f4c29	link	/test e2e-aws-scaleup-rhel7

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.

Details

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.